[RFC] Add BOLT 12 payer proof primitives#4297
[RFC] Add BOLT 12 payer proof primitives#4297vincenzopalazzo wants to merge 7 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4297 +/- ##
==========================================
+ Coverage 87.15% 87.18% +0.03%
==========================================
Files 161 162 +1
Lines 109251 110775 +1524
Branches 109251 110775 +1524
==========================================
+ Hits 95215 96583 +1368
- Misses 11560 11686 +126
- Partials 2476 2506 +30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
A few notes, though I didn't dig into the code at a particularly low level.
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
2 times, most recently
from
2324361 to
9f84e19
Compare
Add a Rust CLI tool that generates and verifies test vectors for BOLT 12 payer proofs as specified in lightning/bolts#1295. The tool uses the rust-lightning implementation from lightningdevkit/rust-lightning#4297. Features: - Generate deterministic test vectors with configurable seed - Verify test vectors from JSON files - Support for basic proofs, proofs with notes, and invalid test cases - Uses refund flow for explicit payer key control Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Some API comments. I'll review the actual code somewhat later (are we locked on on the spec or is it still in flux at all?), but would be nice to reduce allocations in it first anyway.
|
🔔 2nd Reminder Hey @valentinewallace! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @jkczyz! This PR has been waiting for your review. |
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
5 times, most recently
from
fb8c68c to
9ad5c35
Compare
|
Some of the commits mixup changes. And the tests don't compile in two of the commits. Not sure why PR 4297 — commit split reviewAnalysis of which changes in commit 4 logically belong in earlier commits on the Branch commits
Commits 2 and 3 fail to compile ( Belongs in commit 2 (
|
|
🔔 1st Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
6f58300 to
af17a09
Compare
Per Rusty's and Jeffrey's preference (see [1] and [2]), a payer proof must reject any unknown even TLV in the stream: even means "MUST NOT CONTINUE IF YOU DO NOT UNDERSTAND THIS", and including an unknown even TLV in a proof implies the verifier needs to check something about it that it cannot. The parser already enforces this: `ParsedMessage::<FullPayerProofTlvStream>` routes bytes through every sub-stream (offer, invoice request, invoice, payer-proof/signature, and the three experimental ranges) and each `tlv_stream!`-generated sub-parser rejects unknown even TLVs inside its range with `UnknownRequiredFeature`. Types that fall into the unused gap between the signature range (`..=1000`) and the experimental offer range (`1_000_000_000..`) are left unconsumed by every sub-stream and rejected by `ParsedMessage`'s all-bytes-consumed check with `InvalidValue`. Fold `test_parsing_even_type_handling_by_range` and `test_parsing_rejects_unknown_even_signature_range_types` (which together covered only two of the seven sub-stream ranges) into a single `test_parsing_rejects_unknown_even_tlvs_in_every_range` that drives an `assert_rejected` helper across all seven sub-stream ranges plus the gap between the signature and experimental ranges. Also expand the comment on `tlv_stream_iter` to spell out the rejection policy it relies on. No behavior change; this commit only adds test coverage and documentation. [1] lightningdevkit#4297 (comment) [2] lightningdevkit#4297 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
fba9fc5 to
cdac67d
Compare
|
🔔 2nd Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
|
Thanks for the review, now the commit should be better divided and pass the check commits checks. In addition to that, I added the unknown fields rejection to the tests to validate the code. Should be good for another round! |
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
Still got a bit to review. I'm fine if you just make the changes in the relevant commits without using fixups.
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
cdac67d to
865df15
Compare
|
🔔 3rd Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review. |
| /// Test that invalid hash lengths (not multiple of 32) are rejected. | ||
| #[test] | ||
| fn test_parsing_rejects_invalid_hash_length() { | ||
| use core::convert::TryFrom; | ||
|
|
||
| // Create a TLV stream with missing_hashes (type 246) that has invalid length | ||
| // BigSize encoding: values 0-252 are single byte, 253-65535 use 0xFD prefix | ||
| let mut bytes = Vec::new(); | ||
| // TLV type 246 (missing_hashes) - 246 < 253 so single byte | ||
| bytes.push(0xf6); // type 246 | ||
| bytes.push(0x21); // length 33 (not multiple of 32!) | ||
| bytes.extend_from_slice(&[0x00; 33]); // 33 bytes of zeros | ||
|
|
||
| let result = PayerProof::try_from(bytes); | ||
| assert!(result.is_err()); | ||
| } |
There was a problem hiding this comment.
Bug: Stale TLV type numbers after layout restructuring. This test uses type 246 (0xf6) which was the old missing_hashes type. The new type is PAYER_PROOF_MISSING_HASHES_TYPE = 1003. Since 246 is an unknown even type in the SIGNATURE_TYPES range (240..=1000), ParsedMessage rejects it with UnknownRequiredFeature — not because of invalid hash length, but because the type is unrecognized. The comment "invalid hash lengths (not multiple of 32) are rejected" is incorrect for the current code; the actual hash-length validation for type 1003 is now untested.
The same issue applies to test_parsing_rejects_invalid_leaf_hashes_length (type 248, should be 1004) and test_parsing_rejects_short_payer_signature (type 250, should be 241).
To actually test what the comments describe, use the new type constants:
// For missing_hashes: use BigSize encoding for 1003
// 1003 > 252 so requires 0xFD prefix: [0xFD, 0x03, 0xEB]
let mut bytes = Vec::new();
BigSize(PAYER_PROOF_MISSING_HASHES_TYPE).write(&mut bytes).unwrap();
BigSize(33).write(&mut bytes).unwrap(); // not multiple of 32
bytes.extend_from_slice(&[0x00; 33]);
Move the invoice/refund payer key derivation logic into reusable helpers so payer proofs can derive the same signing keys without duplicating the metadata and signer flow.
Add the payer proof types, selective disclosure merkle support, parsing, and tests for constructing and validating BOLT 12 payer proofs from invoices. This implements the payer proof extension to BOLT 12 as specified in lightning/bolts#1295. Missing hashes in a proof are emitted in the DFS traversal order defined by the spec. The BOLT 12 payer proof spec test vectors from bolt12/payer-proof-test.json (full disclosure, minimal disclosure, with payer note, and left-subtree omitted) validate the end-to-end output. The parser rejects unknown even TLVs in every sub-stream range (offer, invoice request, invoice, payer-proof/signature, and the three experimental ranges) via the `tlv_stream!` macro's unknown-even fallback, and rejects types in the unused gap between the signature range and the experimental ranges via the all-bytes-consumed check in `ParsedMessage::try_from`. Co-Authored-By: Rusty Russell <rusty@rustcorp.com.au> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rename the old PaidBolt12Invoice enum to Bolt12InvoiceType, move it out of events, and update outbound payment plumbing to store the renamed invoice type directly.
Encapsulate the paid invoice, preimage, and payer nonce in the PaidBolt12Invoice struct and surface it through Event::PaymentSent::bolt12_invoice. To support the nonce round-trip, plumb payment_nonce through HTLCSource::OutboundRoute, SendAlongPathArgs, PendingOutboundPayment::Retryable and the outbound payment internals, and extract it from the OffersContext variants so payers can later re-derive the payer signing key from the same nonce used for the invoice request. Update expect_payment_sent, claim_payment, claim_payment_along_route and the async-payments test assertions to surface and consume the PaidBolt12Invoice. Also add Writeable/Readable impls for sha256::Hash in util::ser so PaidBolt12Invoice serialization compiles. Co-Authored-By: Jeffrey Czyz <jkczyz@gmail.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
86f5b71 to
3d080ad
Compare
|
Ok I rebased to fix the conflict (hope it was real and not just a github bug) and I pushed as a fixup commit the last iteration done on the specification lightning/bolts#1295 (comment) I would like to give another pass myself, but I think it is good to put it out there for early review too |
| //! | ||
| //! # Proposed authentication change (full payer-proof merkle root) | ||
| //! | ||
| //! The current spec (BOLT 12 PR 1295) signs only `SHA256(payer_signature.note || | ||
| //! invoice_merkle_root)` with `invreq_payer_id`. Tamper-resistance for the | ||
| //! remaining payer-proof TLVs (`preimage`, `omitted_tlvs`, `missing_hashes`, | ||
| //! `leaf_hashes`) is *transitive*: each one has a separate binding (the | ||
| //! preimage hashes to `invoice_payment_hash`; the rest reconstruct the invoice | ||
| //! merkle root that the issuer's `signature` covers). It works only because | ||
| //! every payer_proof TLV today is either part of the already-signed invoice or | ||
| //! has an out-of-band binding to it. Any future payer-side TLV outside both | ||
| //! categories silently loses authentication — see | ||
| //! <https://github.com/lightning/bolts/pull/1295#discussion_r3160114065>. | ||
| //! | ||
| //! T-bast's proposal is to sign the merkle root of *all* payer_proof TLVs and | ||
| //! to extract `note` into its own TLV (a normal merkle leaf instead of being | ||
| //! bundled inside the `payer_signature` TLV). Under that scheme: | ||
| //! | ||
| //! 1. `payer_signature` becomes a plain `bip340sig` like every other signature | ||
| //! TLV in BOLT 12. | ||
| //! 2. `note` becomes a dedicated TLV (`PAYER_PROOF_PROOF_NOTE_TYPE`, see | ||
| //! below); the type number is provisional and may move when the spec | ||
| //! reallocates payer-proof data TLVs out of the `SIGNATURE_TYPES` range. | ||
| //! 3. The `payer_signature` is a tagged signature over the merkle root of all | ||
| //! payer_proof TLVs except the `payer_signature` TLV itself, computed | ||
| //! exactly as `signature` is computed for invoices/offers/invoice requests. | ||
| //! | ||
| //! Verifier flow under the new scheme is two signature checks (instead of the | ||
| //! current implicit chain of three): | ||
| //! | ||
| //! 1. Verify `payer_signature` against the payer-proof merkle root using | ||
| //! `invreq_payer_id`. | ||
| //! 2. Reconstruct the invoice merkle root from `leaf_hashes`, | ||
| //! `missing_hashes`, `omitted_tlvs`, and the disclosed invoice TLVs, then | ||
| //! verify the issuer's `signature` against it using `invoice_node_id`. | ||
| //! | ||
| //! `SHA256(preimage) == invoice_payment_hash` remains a separate check. | ||
| //! | ||
| //! This branch carries the design and a placeholder TLV constant for the new | ||
| //! `payer_note` TLV; the structural code change to switch over has been | ||
| //! deferred until the spec settles on the final TLV layout (in particular | ||
| //! whether the data-bearing payer-proof TLVs `preimage`/`omitted_tlvs`/ | ||
| //! `missing_hashes`/`leaf_hashes` move out of the `SIGNATURE_TYPES` range so | ||
| //! they can be merkle leaves under the standard BOLT 12 signing convention). | ||
| //! Tracking issue: <https://github.com/lightning/bolts/issues/1332>. |
There was a problem hiding this comment.
We may want to remove after the spec meeting if this change is confirmed
| /// docs for the authentication scheme. | ||
| payer_signature: Signature, | ||
| /// Optional payer-supplied note. Its own TLV (a regular merkle leaf) | ||
| /// per BOLT 12 PR 1295 commit `0f2b026`. |
There was a problem hiding this comment.
pushed the code before Rusty updated the spec, so this needs to be removed if the change is confimed
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
2521206 to
5b1d61f
Compare
6 participants
This is a first draft implementation of the payer proof extension to BOLT 12 as proposed in lightning/bolts#1295. The goal is to get early feedback on the API design before the spec is finalized.
Payer proofs allow proving that a BOLT 12 invoice was paid by demonstrating possession of:
This PR adds the core building blocks:
This is explicitly a PoC to validate the API surface - the spec itself is still being refined. Looking for feedback on:
cc @TheBlueMatt @jkczyz