[RFC] Add BOLT 12 payer proof primitives#4297
[RFC] Add BOLT 12 payer proof primitives#4297vincenzopalazzo wants to merge 4 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 86.99% 87.17% +0.17%
==========================================
Files 163 162 -1
Lines 108706 110768 +2062
Branches 108706 110768 +2062
==========================================
+ Hits 94571 96563 +1992
- Misses 11655 11682 +27
- Partials 2480 2523 +43
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
There was a problem hiding this comment.
A few notes, though I didn't dig into the code at a particularly low level.
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
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. |
fb8c68c to
9ad5c35
Compare
56efb64 to
1ee2f1a
Compare
|
|
||
| impl PayerProof { | ||
| /// The payment preimage proving the invoice was paid. | ||
| pub fn preimage(&self) -> PaymentPreimage { |
| /// The payer's note, if any. | ||
| pub fn payer_note(&self) -> Option<PrintableString<'_>> { | ||
| self.contents.payer_note.as_deref().map(PrintableString) | ||
| } |
26648f3 to
6f58300
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. |
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>
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
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.
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>
cdac67d to
865df15
Compare
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