[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.12% +0.13%
==========================================
Files 163 162 -1
Lines 108706 110488 +1782
Branches 108706 110488 +1782
==========================================
+ Hits 94571 96268 +1697
- Misses 11655 11689 +34
- Partials 2480 2531 +51
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
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
6 times, most recently
from
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) | ||
| } |
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
3 times, most recently
from
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. |
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>
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
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.
| /// | ||
| /// The `nonce` and `payment_id` must be the same ones used when creating the original | ||
| /// invoice request. In the common proof-of-payment flow, callers can instead use | ||
| /// `PaidBolt12Invoice::prove_payer_derived` together with the `payment_id` from |
There was a problem hiding this comment.
This should be a link but the type isn't introduced to later. This sentence could go in the later commit instead.
| /// This re-performs the same key derivation that occurs during invoice request creation with | ||
| /// [`InvoiceRequestBuilder::deriving_signing_pubkey`], allowing the payer to recover their | ||
| /// signing keypair for creating payer proofs. |
There was a problem hiding this comment.
I'd rather not be so specific here in case we have other use cases. In fact, this could be used for a Refund's keys.
We should instead frame this in terms keys created by Metadata::derive_from when using Metadata::DerivedSigningPubkey with a MetadataMaterial created with a payment_id.
| /// The `tlv_stream` must contain the offer and invoice request TLV records (excluding | ||
| /// payer metadata type 0 and payer_id type 88), matching what was used during | ||
| /// the original key derivation. |
There was a problem hiding this comment.
Similarly, let's shorten this to:
"The tlv_stream must contain the records matching what was used during the original key derivation."
| use bitcoin::constants::ChainHash; | ||
| use bitcoin::hash_types::{BlockHash, Txid}; | ||
| use bitcoin::hashes::hmac::Hmac; | ||
| use bitcoin::hashes::sha256; |
There was a problem hiding this comment.
This import isn't needed. Use Sha256 instead of sha256::Hash.
| let nonce = self.nonce.ok_or(PayerProofError::KeyDerivationFailed)?; | ||
| let invoice = self.bolt12_invoice().ok_or(PayerProofError::IncompatibleInvoice)?; |
There was a problem hiding this comment.
Shouldn't we switch the order of these? IIUC, a StaticInvoice will never have a Nonce, so we'd never reach the second check, giving a misleading error.
| /// Error during signing. | ||
| SigningError, | ||
| /// The invreq_metadata field cannot be included (per spec). | ||
| InvreqMetadataNotAllowed, |
There was a problem hiding this comment.
s/InvreqMetadataNotAllowed/PayerMetadataNotAllowed
| SigningError, | ||
| /// The invreq_metadata field cannot be included (per spec). | ||
| InvreqMetadataNotAllowed, | ||
| /// TLV types >= 240 cannot be included — they are in the |
There was a problem hiding this comment.
This comment is inaccurate since the experimental types are allowed.
| impl<'a, S: SigningStrategy> PayerProofBuilder<'a, S> { | ||
| /// Include a specific TLV type in the proof. | ||
| /// | ||
| /// Returns an error if the type is not allowed (e.g., invreq_metadata or |
There was a problem hiding this comment.
We should use the names are types use rather than what's in the spec.
| SignError::Signing => PayerProofError::SigningError, | ||
| SignError::Verification(_) => PayerProofError::InvalidPayerSignature, |
There was a problem hiding this comment.
Let's have the method use SignError for the error type like we do for UnsignedBolt12Invoice. Likewise, in build_and_sign we can unwrap since we know we shouldn't error. Our signing function doesn't error and we derived the keys, so verification should pass. We probably don't need those PayerProofError variants.
| PayerSignatureWithNote { signature: payer_signature, note: self.payer_note.take() }; | ||
|
|
||
| let bytes = | ||
| self.serialize_payer_proof(&payer_signature_tlv).expect("Vec write should not fail"); |
There was a problem hiding this comment.
This expect should be in serialize_payer_proof. No need to return a Result there.
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