[RFC] Add BOLT 12 payer proof primitives#4297
[RFC] Add BOLT 12 payer proof primitives#4297vincenzopalazzo wants to merge 5 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4297 +/- ##
==========================================
+ Coverage 86.99% 87.08% +0.08%
==========================================
Files 163 164 +1
Lines 108706 110509 +1803
Branches 108706 110509 +1803
==========================================
+ Hits 94571 96235 +1664
- Misses 11655 11735 +80
- Partials 2480 2539 +59
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
from
3a500c8 to
564682f
Compare
|
Thanks for the catch @jkczyz need to remind myself not to work with git magic command late at night, so I should have restored the old history with the changes requested in the last review from you and Matt. Thanks! |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
| /// values or out-of-bounds lengths. This function validates the framing first, | ||
| /// returning an error instead of panicking on untrusted input. It also checks | ||
| /// strict ascending TLV type ordering (which covers duplicates). | ||
| fn validate_tlv_framing(bytes: &[u8]) -> Result<(), DecodeError> { |
There was a problem hiding this comment.
Why do we need this and the manual TLV record parsing in impl TryFrom<Vec<u8>> for PayerProof {? Can't we use the tlv_stream macros that we use in the other offer types?
There was a problem hiding this comment.
I think it comes down to needing the bytes after parsing to pass to reconstruct_merkle_root, so tlv_stream alone is not sufficient unless we want to re-serialize there. See #4297 (comment).
There was a problem hiding this comment.
I think we still need the manual path here because reconstruct_merkle_root needs the original disclosed TLV record bytes, not just parsed field values. tlv_stream! / ParsedMessage works well for the other BOLT 12 types because they parse a fixed known set of TLVs, but here we also need to carry through an arbitrary subset of invoice TLVs as opaque records for merkle reconstruction. Re-serializing after parsing felt wrong since the merkle logic is defined over the original record bytes, not a normalized encoding. I think we could factor this better if we taught the TLV helpers to preserve/filter raw records alongside typed fields, but that seemed like a larger abstraction change than I wanted to take on in this PR. Happy to go that route if you think it is worth it.
There was a problem hiding this comment.
Re-serializing after parsing felt wrong since the merkle logic is defined over the original record bytes, not a normalized encoding.
We shouldn't need to re-serialize though as we retain the bytes to include in PayerProof. So we can just wrap that in TlvStream and iterate over its TlvRecords as you are currently doing. It's just we wouldn't need to do the parsing in that loop anymore. It would require an additional pass over the bytes, though.
There was a problem hiding this comment.
I vibe some experimentation on this based on our offline discussion, and I added a fixup commit 664617f
I am kind of unsure regarding the impl CursorReadable for FullPayerProofTlvStream if this is the correct approach tho
| /// values or out-of-bounds lengths. This function validates the framing first, | ||
| /// returning an error instead of panicking on untrusted input. It also checks | ||
| /// strict ascending TLV type ordering (which covers duplicates). | ||
| fn validate_tlv_framing(bytes: &[u8]) -> Result<(), DecodeError> { |
There was a problem hiding this comment.
I think it comes down to needing the bytes after parsing to pass to reconstruct_merkle_root, so tlv_stream alone is not sufficient unless we want to re-serialize there. See #4297 (comment).
| if !SIGNATURE_TYPES.contains(&tlv_type) { | ||
| // Included invoice TLV record (passthrough for merkle | ||
| // reconstruction). | ||
| included_types.insert(tlv_type); | ||
| included_records.push(record); | ||
| } else if tlv_type % 2 == 0 { | ||
| // Unknown even types are mandatory-to-understand per | ||
| // BOLT convention — reject them. | ||
| return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)); | ||
| } |
There was a problem hiding this comment.
What I mean is why does this check only apply to SIGNATURE_TYPES? Is this mentioned in the spec somewhere?
| _ => { | ||
| if tlv_type == TLV_INVREQ_METADATA { | ||
| return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)); | ||
| } | ||
| if !SIGNATURE_TYPES.contains(&tlv_type) { | ||
| included_types.insert(tlv_type); | ||
| included_records.push(( | ||
| tlv_type, | ||
| record.end - record.record_bytes.len(), | ||
| record.end, | ||
| )); | ||
| } | ||
| }, |
There was a problem hiding this comment.
I can't quite tell from the spec where it defines these distinctions. Why isn't any unknown even TLV rejected? Seems we are only reject unknown even signature TLV types.
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.
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
d3803f4 to
e0f843a
Compare
| let offer_records = TlvStream::new(bytes).range(OFFER_TYPES); | ||
| let invoice_request_records = TlvStream::new(bytes) | ||
| .range(INVOICE_REQUEST_TYPES) | ||
| .filter(|record| record.r#type != PAYER_METADATA_TYPE); |
There was a problem hiding this comment.
Nit: This filter is dead code. PAYER_METADATA_TYPE is 0, and INVOICE_REQUEST_TYPES is 80..160. Type 0 can never appear in range 80..160, so the filter never matches. Additionally, the producer (serialize_payer_proof) never emits type 0 since include_type rejects it, so even the OFFER_TYPES range (1..80) wouldn't contain it.
| .filter(|record| record.r#type != PAYER_METADATA_TYPE); | |
| let invoice_request_records = TlvStream::new(bytes) | |
| .range(INVOICE_REQUEST_TYPES); |
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
664617f to
dbfd2a5
Compare
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
5 times, most recently
from
3e056b2 to
a1064e2
Compare
Add the payer proof types, selective disclosure merkle support, parsing, and tests for constructing and validating BOLT 12 payer proofs from invoices.
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 invoice, preimage, and nonce in PaidBolt12Invoice and surface it in PaymentSent. Rework builder to return UnsignedPayerProof with SignFn/sign_message integration, use encode_tlv_stream! for serialization, move helpers to DisclosedFields methods, and address naming conventions and TLV validation feedback. Co-Authored-By: Jeffrey Czyz <jkczyz@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Don't require payer proof missing hashes to remain TLV-sorted now that the spec uses DFS traversal order. (cherry picked from commit 334bb05)
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
56efb64 to
1ee2f1a
Compare
| /// Compound value for the payer signature TLV (type 250): a schnorr signature | ||
| /// followed by optional UTF-8 note bytes. | ||
| struct PayerSignatureWithNoteRef<'a> { | ||
| signature: &'a Signature, | ||
| note: Option<&'a str>, | ||
| } |
|
|
||
| impl PayerProof { | ||
| /// The payment preimage proving the invoice was paid. | ||
| pub fn preimage(&self) -> PaymentPreimage { |
| /// The payer's public key (who paid). | ||
| pub fn payer_id(&self) -> PublicKey { | ||
| self.contents.payer_id | ||
| } |
There was a problem hiding this comment.
In InvoiceRequest, we use payer_signing_pubkey so we should do the same here.
| /// The payer's note, if any. | ||
| pub fn payer_note(&self) -> Option<PrintableString<'_>> { | ||
| self.contents.payer_note.as_deref().map(PrintableString) | ||
| } |
| fn test_parsing_rejects_payer_metadata() { | ||
| let proof = build_round_trip_proof_with_multiple_trailing_omitted_tlvs(); | ||
| let mut bytes = Vec::new(); | ||
| write_tlv_record_bytes(&mut bytes, PAYER_METADATA_TYPE, &[1; 32]); | ||
| bytes.extend_from_slice(proof.bytes()); | ||
|
|
||
| let result = PayerProof::try_from(bytes); | ||
| assert!(matches!(result, Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)))); | ||
| } |
There was a problem hiding this comment.
Bug: This test asserts the wrong error variant and should fail at runtime.
FullPayerProofTlvStream starts with OfferTlvStream (range OFFER_TYPES = 1..80). Since type 0 falls below this range, the CursorReadable for OfferTlvStream reads the type, finds it out of range, rewinds, and breaks. All 7 CursorReadable components do the same — they all skip type 0. The cursor stays at position 0 forever.
Result: all TLV fields are None. ParsedPayerProofFields::try_from then fails at:
let payer_id = payer_id.ok_or(Bolt12ParseError::InvalidSemantics(
Bolt12SemanticError::MissingPayerSigningPubkey,
))?;This produces Bolt12ParseError::InvalidSemantics(MissingPayerSigningPubkey), not Bolt12ParseError::Decode(DecodeError::InvalidValue) as asserted here.
Fix: either change the assertion to match the actual error, or (better) add an explicit type-0 rejection in the parser so the error is clear:
| fn test_parsing_rejects_payer_metadata() { | |
| let proof = build_round_trip_proof_with_multiple_trailing_omitted_tlvs(); | |
| let mut bytes = Vec::new(); | |
| write_tlv_record_bytes(&mut bytes, PAYER_METADATA_TYPE, &[1; 32]); | |
| bytes.extend_from_slice(proof.bytes()); | |
| let result = PayerProof::try_from(bytes); | |
| assert!(matches!(result, Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)))); | |
| } | |
| fn test_parsing_rejects_payer_metadata() { | |
| let proof = build_round_trip_proof_with_multiple_trailing_omitted_tlvs(); | |
| let mut bytes = Vec::new(); | |
| write_tlv_record_bytes(&mut bytes, PAYER_METADATA_TYPE, &[1; 32]); | |
| bytes.extend_from_slice(proof.bytes()); | |
| let result = PayerProof::try_from(bytes); | |
| assert!(result.is_err(), "Payer metadata (type 0) should be rejected"); | |
| } |
There was a problem hiding this comment.
ParsedMessage checks that all bytes are consumed, so it should fail before then.
| fn try_from(bytes: Vec<u8>) -> Result<Self, Self::Error> { | ||
| let parsed_proof = ParsedMessage::<FullPayerProofTlvStream>::try_from(bytes)?; | ||
| let ParsedMessage { bytes, tlv_stream } = parsed_proof; | ||
| let ParsedPayerProofFields { contents, omitted_markers, missing_hashes, leaf_hashes } = | ||
| ParsedPayerProofFields::try_from(tlv_stream)?; | ||
| let included_records: Vec<_> = tlv_stream_iter(&bytes).collect(); |
There was a problem hiding this comment.
Residual panic on crafted input (extends prior TlvStream issue):
ParsedMessage<FullPayerProofTlvStream> validates TLV framing only for types in covered ranges. There's a type gap at 1001–999,999,999 (between PayerProofTlvStream range 240..=1000 and ExperimentalOfferTlvStream range 1,000,000,000+). The CursorReadable for each component reads the type BigSize, finds it out of range, rewinds, and breaks — without reading the length or value bytes.
If a crafted proof contains a TLV with a type in this gap and a malformed BigSize length (e.g., 0xFD prefix followed by only 1 byte), ParsedMessage::try_from succeeds (cursor stuck at the gap TLV), but tlv_stream_iter(&bytes) here calls TlvStream::new(&bytes) which iterates over the full bytes. When it reaches the gap TLV and tries to read the truncated length, BigSize::read().unwrap() panics.
This is a DoS vector on untrusted input. Fix options:
- Check that
ParsedMessageconsumed all bytes (cursor at end) and reject if not - Make
TlvStream::next()returnNoneinstead of panicking on malformed BigSize - Add a
validate_tlv_framing(&bytes)pass beforetlv_stream_iter
Option 1 is simplest and also catches other gap issues (e.g., type 0 from the test above):
let parsed_proof = ParsedMessage::<FullPayerProofTlvStream>::try_from(bytes)?;
// Reject if ParsedMessage didn't consume all bytes (gap TLVs, type 0, etc.)
// (would need ParsedMessage to expose cursor position or a "fully consumed" check)There was a problem hiding this comment.
I believe (1) is exactly what ParsedMessage does, but wouldn't hurt to add a test.
| type FullPayerProofTlvStream = ( | ||
| OfferTlvStream, | ||
| InvoiceRequestTlvStream, | ||
| InvoiceTlvStream, | ||
| PayerProofTlvStream, | ||
| ExperimentalOfferTlvStream, | ||
| ExperimentalInvoiceRequestTlvStream, | ||
| ExperimentalInvoiceTlvStream, | ||
| ); |
There was a problem hiding this comment.
Design concern: FullPayerProofTlvStream omits PayerTlvStream (type 0) from its component list. This means type 0 TLVs in untrusted input are silently skipped rather than explicitly rejected, causing confusing downstream errors (MissingPayerSigningPubkey instead of a clear "payer metadata not allowed" error).
Consider either:
- Adding
PayerTlvStreamas the first component (then checkingpayer_metadata.is_none()inParsedPayerProofFields::try_from) - Or adding a post-parse check that no types below
OFFER_TYPES.startexist in the bytes
This would also fix the test_parsing_rejects_payer_metadata test which currently asserts the wrong error variant.
There was a problem hiding this comment.
I don't believe this is the case just as it isn't for FullOfferTlvStream. @vincenzopalazzo Could you write a test that confirms this?
There was a problem hiding this comment.
Specifically, when using PayerProof::try_from, which uses ParsedMessage::<FullPayerProofTlvStream>::try_from.
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