[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 85.86% 85.63% -0.23%
==========================================
Files 159 160 +1
Lines 104302 105267 +965
Branches 104302 105267 +965
==========================================
+ Hits 89558 90149 +591
- Misses 12246 12599 +353
- Partials 2498 2519 +21
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
| pub(super) struct SelectiveDisclosure { | ||
| /// Nonce hashes for included TLVs (in TLV type order). | ||
| pub(super) leaf_hashes: Vec<sha256::Hash>, |
There was a problem hiding this comment.
Naming: the field is called leaf_hashes but the doc comment says "Nonce hashes" and the values stored are nonce hashes (computed from tagged_hash_from_engine(nonce_tag, record.type_bytes)), not leaf hashes. In reconstruct_merkle_root, the variable is used as nonce_hash = leaf_hashes[inc_idx]. This naming inconsistency could lead to confusion during maintenance. Consider renaming to nonce_hashes.
There was a problem hiding this comment.
Fair point on the naming — the doc comment says "Nonce hashes" but the field is leaf_hashes. Looking at the spec though, the TLV field is literally called leaf_hashes (type 248), so the field name matches the spec. The doc comment could be more precise but the naming follows the wire format.
|
|
Implement payer proofs for BOLT 12 invoices as specified in lightning/bolts#1295. A payer proof cryptographically demonstrates that a BOLT 12 invoice was paid using selective disclosure of invoice fields, the payment preimage, and signatures from both the invoice issuer and the payer. The selective disclosure mechanism uses a merkle tree over the invoice's TLV fields, allowing the payer to reveal only chosen fields while proving the full invoice was signed by the issuer. Privacy-preserving omitted markers hide the actual TLV type numbers of undisclosed fields. PayerProofBuilder provides two signing modes: an explicit signing function for callers with direct key access, and automatic key re-derivation from ExpandedKey + Nonce + PaymentId for the common case where invoice requests used deriving_signing_pubkey. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove nonce_hash from TlvMerkleData, saving 32 bytes per TLV record. The nonce hash was stored for every TLV but only consumed by included TLVs when building leaf_hashes, so collect those directly during the TlvStream iteration instead. Inline reconstruct_positions_from_records into reconstruct_merkle_root so that TreeNodes are built directly during position reconstruction, eliminating the intermediate Vec<bool>. Pre-allocate missing_with_types and needs_hash with with_capacity(num_omitted) to avoid repeated reallocations during tree traversal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
1c98598 to
af080fe
Compare
| impl Bech32Encode for PayerProof { | ||
| const BECH32_HRP: &'static str = PAYER_PROOF_HRP; | ||
| } |
There was a problem hiding this comment.
Missing FromStr impl. Both Offer and Refund implement FromStr (delegating to from_bech32_str), which is the standard Rust way to parse from a string. Without this, users can't do encoded.parse::<PayerProof>(). The bech32 decode path (from_bech32_str → TryFrom<Vec<u8>>) is also not exercised by any test — the tests only check the bech32 prefix and then round-trip through raw TLV bytes.
| impl Bech32Encode for PayerProof { | |
| const BECH32_HRP: &'static str = PAYER_PROOF_HRP; | |
| } | |
| impl Bech32Encode for PayerProof { | |
| const BECH32_HRP: &'static str = PAYER_PROOF_HRP; | |
| } | |
| impl core::str::FromStr for PayerProof { | |
| type Err = crate::offers::parse::Bolt12ParseError; | |
| fn from_str(s: &str) -> Result<Self, <Self as core::str::FromStr>::Err> { | |
| Self::from_bech32_str(s) | |
| } | |
| } |
There was a problem hiding this comment.
Intentionally deferred — the bech32 decode path can be added later when there's a concrete use case. Right now we have Display for encoding and TryFrom<Vec<u8>> for raw bytes. Adding FromStr is a nice-to-have but not blocking for the initial implementation.
| nodes.push(TreeNode { hash: None, included: false, min_type: 0 }); | ||
|
|
||
| let mut inc_idx = 0; | ||
| let mut mrk_idx = 0; | ||
| let mut prev_marker: u64 = 0; | ||
| let mut node_idx: u64 = 1; |
There was a problem hiding this comment.
The min_type field is assigned node_idx (a sequential counter 0, 1, 2, ...) rather than the actual TLV type. On the producer side (build_tree_with_disclosure), min_type is the real TLV type (e.g., 0, 10, 20, ...). Both needs_hash and missing_with_types are sorted by min_type to establish a bijection between the two sides, so this only works because sequential indices and TLV types are both monotonically increasing left-to-right, preserving the relative sort order.
This implicit invariant is fragile and worth documenting with a comment, since a reader comparing the two functions would naturally expect the min_type values to match.
There was a problem hiding this comment.
Not a bug — the sequential node_idx preserves the same relative ordering as the actual TLV types because TLVs are always in ascending type order. Since min_type is only used for sorting needs_hash entries, and sorted sequential indices produce the same order as sorted TLV types, the result is equivalent. The producer uses real types, the consumer uses indices, but both sort to the same permutation.
| // We should have 4 missing hashes for omitted types: | ||
| // - type 0 (single leaf) | ||
| // - types 20+30 (combined branch, min_type=20) | ||
| // - type 50 (single leaf) | ||
| // - type 60 (single leaf) | ||
| // | ||
| // The spec example only shows 3, but that appears to be incomplete | ||
| // (missing hash for type 60). Our implementation should produce 4. | ||
| assert_eq!( | ||
| disclosure.missing_hashes.len(), | ||
| 4, | ||
| "Expected 4 missing hashes for omitted types [0, 20+30, 50, 60]" |
There was a problem hiding this comment.
This comment acknowledges a disagreement with the spec ("The spec example only shows 3, but that appears to be incomplete ... Our implementation should produce 4"). For the tree structure with 7 nodes where the last node (type 60) is an odd-one-out at level 0, it correctly needs its own missing_hash at level 1 when paired with the (40,50) subtree. So this implementation produces 4 missing hashes.
This should be resolved with the spec authors before merging — if other implementations follow the spec's "3 missing hashes" example, payer proofs generated by this code won't interoperate.
There was a problem hiding this comment.
This is a known open question with the spec, already noted in the test comment. The spec example shows 3 missing hashes but for 7 nodes with 2 included, the correct count is 4 (type 0, types 20+30 as branch, type 50, type 60). The round-trip test proves our implementation is self-consistent. Once the spec adds test vectors (there's a FIXME placeholder for that), we'll align.
| .build_with_derived_key(&expanded_key, wrong_nonce, payment_id, None); | ||
| assert!(matches!(result, Err(PayerProofError::KeyDerivationFailed))); | ||
|
|
||
| // --- Test 4: Minimal proof (only required fields) --- | ||
| let minimal_proof = invoice.payer_proof_builder(payment_preimage).unwrap() | ||
| .build_with_derived_key(&expanded_key, payer_nonce, payment_id, None) | ||
| .unwrap(); | ||
| // --- Test 5: Proof with selective disclosure and payer note --- | ||
| let proof_with_note = invoice.payer_proof_builder(payment_preimage).unwrap() |
There was a problem hiding this comment.
The test claims "Serialization Round-Trip" and encodes to bech32, but only checks the prefix and then round-trips through raw TLV bytes via PayerProof::try_from(proof.bytes().to_vec()). The actual bech32 decode path (from_bech32_str → TryFrom<Vec<u8>>) is never tested. You should decode from the bech32 string too:
let decoded_from_bech32 = PayerProof::from_bech32_str(&encoded).unwrap();
assert_eq!(decoded_from_bech32.preimage(), proof.preimage());(Same issue in the second test at creates_payer_proof_with_note_and_selective_disclosure.)
There was a problem hiding this comment.
The test intentionally round-trips through raw TLV bytes (PayerProof::try_from(proof.bytes().to_vec())) which exercises the full verification pipeline (preimage, invoice sig, payer sig, merkle reconstruction). The bech32 encode is tested via to_string() prefix check. Full bech32 decode requires FromStr which is deferred — see the other comment.
| let marker = if let Some(prev_type) = prev_included_type { | ||
| prev_type + 1 | ||
| } else if let Some(last_marker) = prev_marker { | ||
| last_marker + 1 |
There was a problem hiding this comment.
prev_type + 1 and last_marker + 1 can overflow if the value is u64::MAX. While extreme TLV types are impractical, this would panic in debug builds. The same + 1 overflow exists in reconstruct_merkle_root at line 561 (prev_marker + 1). Consider using checked_add or saturating_add and returning an error on overflow.
There was a problem hiding this comment.
Theoretical but not practical — TLV types near u64::MAX don't exist in any BOLT 12 context. The marker algorithm operates on types 0-239 (non-signature invoice TLVs). That said, if we want to be paranoid we could use saturating_add, but it would change semantics in an impossible edge case. Not worth the complexity IMO.
| fn serialize_payer_proof(&self, payer_signature: &Signature, note: Option<&str>) -> Vec<u8> { | ||
| let mut bytes = Vec::new(); | ||
|
|
||
| for record in | ||
| TlvStream::new(&self.invoice_bytes).filter(|r| self.included_types.contains(&r.r#type)) | ||
| { | ||
| bytes.extend_from_slice(record.record_bytes); | ||
| } |
There was a problem hiding this comment.
Bug (defense-in-depth): This iterates over self.invoice_bytes which includes the invoice's own signature record (type 240, same value as TLV_SIGNATURE). If included_types ever contains a signature-range type (e.g., 240), this loop would write the invoice signature as a "passthrough" included record, and then lines 327-329 would write it again as TLV_SIGNATURE, producing duplicate type-240 entries that fail the parser's ordering check.
The prior review flagged include_type() not rejecting types >= 240 as the root cause. But even if that's fixed, this loop should defensively filter signature types to match what build_unsigned does for bytes_without_sig:
| fn serialize_payer_proof(&self, payer_signature: &Signature, note: Option<&str>) -> Vec<u8> { | |
| let mut bytes = Vec::new(); | |
| for record in | |
| TlvStream::new(&self.invoice_bytes).filter(|r| self.included_types.contains(&r.r#type)) | |
| { | |
| bytes.extend_from_slice(record.record_bytes); | |
| } | |
| for record in | |
| TlvStream::new(&self.invoice_bytes).filter(|r| self.included_types.contains(&r.r#type) && !SIGNATURE_TYPES.contains(&r.r#type)) | |
| { | |
| bytes.extend_from_slice(record.record_bytes); | |
| } |
There was a problem hiding this comment.
Good catch, fixed in f78efc0 together with the include_type guard. The serialization loop now defensively filters SIGNATURE_TYPES from the invoice bytes iteration as belt-and-suspenders.
| for record in TlvStream::new(&bytes) { | ||
| let tlv_type = record.r#type; | ||
|
|
||
| // Strict ascending order check covers both ordering and duplicates. | ||
| if tlv_type <= prev_tlv_type && prev_tlv_type != 0 { | ||
| return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)); | ||
| } | ||
| prev_tlv_type = tlv_type; |
There was a problem hiding this comment.
Minor bug in ordering logic: prev_tlv_type is initialized to 0, and the guard prev_tlv_type != 0 means the check never fires when prev_tlv_type == 0. This is intentional for the first iteration (to allow any first TLV type), but it also means that if two consecutive TLV records both have type 0, the second would not be caught by this check.
Currently mitigated because type 0 (PAYER_METADATA_TYPE) is rejected at line 588, but the ordering logic itself is unsound. A cleaner approach would be:
| for record in TlvStream::new(&bytes) { | |
| let tlv_type = record.r#type; | |
| // Strict ascending order check covers both ordering and duplicates. | |
| if tlv_type <= prev_tlv_type && prev_tlv_type != 0 { | |
| return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)); | |
| } | |
| prev_tlv_type = tlv_type; | |
| // Strict ascending order check covers both ordering and duplicates. | |
| // Use a separate flag for the first iteration rather than special-casing 0. | |
| if prev_tlv_type > 0 && tlv_type <= prev_tlv_type { | |
| return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)); | |
| } | |
| prev_tlv_type = tlv_type; |
(Note: this changes behavior slightly — the very first TLV with type 0 would now set prev_tlv_type = 0, and a second type 0 would still pass. A proper fix would use an Option<u64> or a bool flag. But since type 0 is rejected anyway, this is low priority.)
There was a problem hiding this comment.
Same as the other comment — type 0 is PAYER_METADATA_TYPE, unconditionally rejected by the catch-all. The prev_tlv_type \!= 0 guard only matters for the first iteration, which is the intended behavior.
| /// Compute the payer signature message per BOLT 12 signature calculation. | ||
| fn compute_payer_signature_message(note: Option<&str>, merkle_root: &sha256::Hash) -> Message { | ||
| let mut inner_hasher = sha256::Hash::engine(); | ||
| if let Some(n) = note { | ||
| inner_hasher.input(n.as_bytes()); | ||
| } | ||
| inner_hasher.input(merkle_root.as_ref()); | ||
| let inner_msg = sha256::Hash::from_engine(inner_hasher); | ||
|
|
||
| let tag_hash = sha256::Hash::hash(PAYER_SIGNATURE_TAG.as_bytes()); | ||
|
|
||
| let mut final_hasher = sha256::Hash::engine(); | ||
| final_hasher.input(tag_hash.as_ref()); | ||
| final_hasher.input(tag_hash.as_ref()); | ||
| final_hasher.input(inner_msg.as_ref()); | ||
| let final_digest = sha256::Hash::from_engine(final_hasher); | ||
|
|
||
| Message::from_digest(*final_digest.as_byte_array()) | ||
| } |
There was a problem hiding this comment.
Design concern: This payer signature construction uses a non-standard double-hash pattern — SHA256(tag || tag || SHA256(note || merkle_root)) — rather than the standard BOLT 12 pattern used elsewhere (SHA256(tag || tag || merkle_root), as in TaggedHash::from_merkle_root).
Two things to verify against the spec:
-
No length-prefix on the note: The inner hash concatenates
note_bytes || merkle_root_byteswithout a length delimiter. Since the merkle root is always 32 bytes, the boundary is deterministic — but this is unusual and should be confirmed as spec-intended. -
Nonevs empty note ambiguity: WhennoteisNone, the inner hash isSHA256(merkle_root). WhennoteisSome(""), it's alsoSHA256(merkle_root)since"".as_bytes()is empty. Both produce the same signature, meaning a verifier cannot distinguish "no note" from "empty note". If the spec distinguishes these cases, this needs a fix (e.g., including a presence flag in the hash).
There was a problem hiding this comment.
This follows the spec exactly. The payer proof spec defines payer_signature.sig as signing SHA256(note || merkle_root) using the standard BOLT 12 tagged hash format (SHA256(tag || tag || msg)). The "double hash" is just the BIP 340 tagged hash construction applied to the inner SHA256(note || merkle_root) message. This is the specified behavior, not a deviation.
| let len: BigSize = Readable::read(&mut record_cursor)?; | ||
| payer_signature = Some(Readable::read(&mut record_cursor)?); | ||
| let note_len = len.0.saturating_sub(64); |
There was a problem hiding this comment.
Potential issue with malformed proofs: len.0 is parsed from untrusted input. If len.0 is, say, 63 (less than the 64 bytes needed for the signature), Readable::read at line 575 will attempt to read 64 bytes but the cursor only has len.0 bytes of value remaining. This will fail with a decode error, which is correct.
However, saturating_sub(64) silently produces 0 here, masking the fact that the length was invalid. Consider explicitly rejecting len.0 < 64:
if len.0 < 64 {
return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue));
}This would provide a clearer error and avoid relying on the signature read to catch the malformed length.
There was a problem hiding this comment.
Not a bug — the cursor reads from record.record_bytes which is bounded by TlvStream. After reading _type and _len, the remaining cursor data is exactly len.0 bytes. If len.0 < 64, Readable::read for Signature hits EOF on the cursor and returns a decode error. The saturating_sub for note_len never executes because the sig read already failed.
| /// Compute omitted markers per BOLT 12 payer proof spec. | ||
| fn compute_omitted_markers(tlv_data: &[TlvMerkleData]) -> Vec<u64> { | ||
| let mut markers = Vec::new(); | ||
| let mut prev_included_type: Option<u64> = None; | ||
| let mut prev_marker: Option<u64> = None; | ||
|
|
||
| for data in tlv_data { | ||
| if data.tlv_type == 0 { | ||
| continue; | ||
| } | ||
|
|
||
| if !data.is_included { | ||
| let marker = if let Some(prev_type) = prev_included_type { | ||
| prev_type + 1 | ||
| } else if let Some(last_marker) = prev_marker { | ||
| last_marker + 1 | ||
| } else { | ||
| 1 | ||
| }; | ||
|
|
||
| markers.push(marker); | ||
| prev_marker = Some(marker); | ||
| prev_included_type = None; | ||
| } else { | ||
| prev_included_type = Some(data.tlv_type); | ||
| prev_marker = None; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Subtle invariant: compute_omitted_markers unconditionally skips TLV type 0 at line 390, assuming it's always the first record (payer_metadata). But the function operates on a &[TlvMerkleData] with no assertion that the first entry IS type 0. If this function were ever called with a TLV stream whose first record is NOT type 0 (e.g., a different invoice format), the implicit TLV0 assumption would produce incorrect markers.
Consider either:
- Adding
debug_assert!(tlv_data.first().map(|d| d.tlv_type) == Some(0))at the top, or - Documenting the precondition that the first TLV must be type 0
There was a problem hiding this comment.
Correct by design — per spec, type 0 (invreq_metadata) is always omitted and implicit (never included in markers). The function receives the full TlvMerkleData vec which always starts with TLV 0 from the invoice bytes. The if data.tlv_type == 0 { continue } just skips the implicit omission. If called with pre-filtered data that excludes type 0, the skip is a no-op — the algorithm still works correctly.
fixup! Add BOLT 12 payer proof implementation Guard include_type against signature-range TLV types (240-1000) Per spec, the payer proof signature (type 240), preimage (242), omitted_tlvs (244), missing_hashes (246), leaf_hashes (248), and payer_signature (250) are all handled separately by the payer proof format. Before this fix, include_type(240) would insert 240 into included_types, causing serialize_payer_proof to emit the invoice's signature record as an included invoice TLV *and* then emit it again as the payer proof's own TLV_SIGNATURE field — producing a malformed TLV stream with duplicate type 240. Fix: - include_type now rejects SIGNATURE_TYPES (240..=1000) with a new SignatureTypeNotAllowed error variant - serialize_payer_proof defensively filters SIGNATURE_TYPES from the invoice bytes iteration as a belt-and-suspenders guard Tests added: - test_include_type_rejects_signature_types: validates boundary conditions (240, 250, 1000 rejected; 239, 1001 allowed) - test_parsing_rejects_unknown_even_signature_range_types: ensures the parser rejects unknown even types in the signature range Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| let mut prev_tlv_type: u64 = 0; | ||
|
|
||
| for record in TlvStream::new(&bytes) { |
There was a problem hiding this comment.
Bug: TlvStream::new(&bytes) on untrusted input can panic. The TlvStream iterator calls .unwrap() on BigSize::read and performs unchecked slice indexing (&data[start..end] where end = offset + length from the TLV). If the input bytes are malformed (e.g., truncated BigSize, or a TLV length field claiming more bytes than available), this will panic.
All other BOLT 12 TryFrom<Vec<u8>> implementations go through ParsedMessage<T> / CursorReadable which does bounds-checked reads. This is the only parser that feeds raw untrusted bytes directly into TlvStream, creating a DoS vector via panic.
You need to either:
- Add bounds checking to
TlvStream(returnsNoneinstead of panicking on malformed input), or - Pre-validate the TLV stream before passing it to
TlvStream::new, or - Use the existing
CursorReadable/ParsedMessageinfrastructure for the initial parse pass.
| let tlv_type = record.r#type; | ||
|
|
||
| // Strict ascending order check covers both ordering and duplicates. | ||
| if tlv_type <= prev_tlv_type && prev_tlv_type != 0 { |
There was a problem hiding this comment.
Bug: This check doesn't catch duplicate type 0. prev_tlv_type starts at 0. If the first TLV has type 0, then 0 <= 0 && 0 != 0 is false, so it passes. Now prev_tlv_type = 0. If the second TLV also has type 0, the check is again 0 <= 0 && 0 != 0 → false, so a duplicate type 0 is accepted.
Fix: use an Option<u64> for prev_tlv_type initialized to None, and only skip the check on the very first record:
| if tlv_type <= prev_tlv_type && prev_tlv_type != 0 { | |
| if prev_tlv_type != 0 && tlv_type <= prev_tlv_type { |
Or alternatively, use a boolean is_first flag to skip only the initial comparison.
lightning/src/offers/payer_proof.rs
Outdated
| pub fn build_with_derived_key( | ||
| self, expanded_key: &ExpandedKey, nonce: Nonce, payment_id: PaymentId, note: Option<&str>, | ||
| ) -> Result<PayerProof, PayerProofError> { | ||
| let secp_ctx = Secp256k1::new(); |
There was a problem hiding this comment.
Nit/API: Secp256k1::new() allocates a ~1.3MB signing+verification context on every call. The adjacent derive_payer_signing_keys method (which this calls into) already accepts a secp_ctx parameter, and this is the standard pattern for public BOLT 12 APIs in this codebase. Consider accepting a &Secp256k1<secp256k1::All> parameter instead of allocating internally.
| let _type: BigSize = Readable::read(&mut record_cursor)?; | ||
| let len: BigSize = Readable::read(&mut record_cursor)?; | ||
| payer_signature = Some(Readable::read(&mut record_cursor)?); | ||
| let note_len = len.0.saturating_sub(64); |
There was a problem hiding this comment.
Minor: If len.0 < 64, the Readable::read for the 64-byte signature on line 585 will attempt to read past the available value bytes, failing with a generic DecodeError from the ? propagation. Consider adding an explicit check if len.0 < 64 { return Err(...) } before the signature read for a clearer error message and to avoid relying on the cursor's bounds for correctness.
|
|
||
| let mut inc_idx = 0; | ||
| let mut mrk_idx = 0; | ||
| let mut prev_marker: u64 = 0; | ||
| let mut node_idx: u64 = 1; | ||
|
|
||
| while inc_idx < included_records.len() || mrk_idx < omitted_markers.len() { | ||
| if mrk_idx >= omitted_markers.len() { | ||
| // No more markers, remaining positions are included | ||
| let (_, record_bytes) = included_records[inc_idx]; | ||
| let leaf_hash = tagged_hash_from_engine(leaf_tag.clone(), record_bytes); | ||
| let nonce_hash = leaf_hashes[inc_idx]; | ||
| let hash = tagged_branch_hash_from_engine(branch_tag.clone(), leaf_hash, nonce_hash); | ||
| nodes.push(TreeNode { hash: Some(hash), included: true, min_type: node_idx }); | ||
| inc_idx += 1; | ||
| } else if inc_idx >= included_records.len() { | ||
| // No more included types, remaining positions are omitted | ||
| nodes.push(TreeNode { hash: None, included: false, min_type: node_idx }); | ||
| prev_marker = omitted_markers[mrk_idx]; | ||
| mrk_idx += 1; | ||
| } else { | ||
| let marker = omitted_markers[mrk_idx]; | ||
| let (inc_type, _) = included_records[inc_idx]; | ||
|
|
||
| if marker == prev_marker + 1 { | ||
| // Continuation of current run -> omitted position | ||
| nodes.push(TreeNode { hash: None, included: false, min_type: node_idx }); | ||
| prev_marker = marker; | ||
| mrk_idx += 1; | ||
| } else { | ||
| // Jump detected -> included position comes first | ||
| let (_, record_bytes) = included_records[inc_idx]; | ||
| let leaf_hash = tagged_hash_from_engine(leaf_tag.clone(), record_bytes); | ||
| let nonce_hash = leaf_hashes[inc_idx]; | ||
| let hash = | ||
| tagged_branch_hash_from_engine(branch_tag.clone(), leaf_hash, nonce_hash); | ||
| nodes.push(TreeNode { hash: Some(hash), included: true, min_type: node_idx }); | ||
| prev_marker = inc_type; | ||
| inc_idx += 1; | ||
| } | ||
| } | ||
| node_idx += 1; | ||
| } |
There was a problem hiding this comment.
The min_type field in the reconstruction uses sequential node indices (0, 1, 2, ...) rather than actual TLV type values. Meanwhile, build_tree_with_disclosure uses actual TLV type numbers. The needs_hash and missing_with_types vectors are sorted by min_type, so the ordering must agree between producer and consumer.
This currently works because both TLV types and sequential indices are monotonically increasing, preserving relative ordering. However, this is a fragile invariant that's not documented. If either side's iteration order were to change (e.g., for experimental TLV types in a different range), the sorted orders could diverge silently, producing incorrect merkle roots that would fail signature verification with no useful diagnostic.
Consider using actual TLV types (available from included_records[inc_idx].0) instead of node_idx for min_type in the reconstruction, to make the two sides structurally consistent.
| fn compute_payer_signature_message(note: Option<&str>, merkle_root: &sha256::Hash) -> Message { | ||
| let mut inner_hasher = sha256::Hash::engine(); | ||
| if let Some(n) = note { | ||
| inner_hasher.input(n.as_bytes()); | ||
| } | ||
| inner_hasher.input(merkle_root.as_ref()); | ||
| let inner_msg = sha256::Hash::from_engine(inner_hasher); | ||
|
|
||
| let tag_hash = sha256::Hash::hash(PAYER_SIGNATURE_TAG.as_bytes()); | ||
|
|
||
| let mut final_hasher = sha256::Hash::engine(); | ||
| final_hasher.input(tag_hash.as_ref()); | ||
| final_hasher.input(tag_hash.as_ref()); | ||
| final_hasher.input(inner_msg.as_ref()); | ||
| let final_digest = sha256::Hash::from_engine(final_hasher); | ||
|
|
||
| Message::from_digest(*final_digest.as_byte_array()) |
There was a problem hiding this comment.
The payer signature message uses a non-standard double-hash construction: H(tag || tag || H(note || merkle_root)). Standard BOLT 12 signature computation (as in TaggedHash::from_merkle_root) is a single tagged hash: H(tag || tag || merkle_root).
If the spec defines the payer signature differently from the standard BOLT 12 tagged hash, this should be documented with a reference to the spec section. If the spec uses the standard construction, this needs to be fixed — the inner hash would make this incompatible with other implementations.
Also, when note is None, this computes H(merkle_root) as the inner message. When note is Some("") (empty string), it also computes H(merkle_root) since empty input contributes nothing to the hasher. These two cases are indistinguishable, meaning a verifier cannot distinguish "no note" from "empty note".
| // We should have 4 missing hashes for omitted types: | ||
| // - type 0 (single leaf) | ||
| // - types 20+30 (combined branch, min_type=20) | ||
| // - type 50 (single leaf) | ||
| // - type 60 (single leaf) | ||
| // | ||
| // The spec example only shows 3, but that appears to be incomplete | ||
| // (missing hash for type 60). Our implementation should produce 4. | ||
| assert_eq!( | ||
| disclosure.missing_hashes.len(), | ||
| 4, | ||
| "Expected 4 missing hashes for omitted types [0, 20+30, 50, 60]" | ||
| ); |
There was a problem hiding this comment.
This test explicitly disagrees with the spec ("The spec example only shows 3, but that appears to be incomplete"). If the implementation produces 4 missing hashes but the spec says 3, one of them is wrong. This needs to be resolved against the spec before merging — either the spec is right and the tree construction has a bug (perhaps types 50 and 60 should be merged into a single missing hash at a higher level), or the spec example is indeed incomplete and should be updated.
Shipping code that knowingly diverges from the spec without a clear resolution risks incompatibility with other implementations.
fixup! Add BOLT 12 payer proof implementation Fix two issues found during spec cross-check review: 1. invoice_features inclusion: check TLV presence, not parsed value Per spec, invoice_features MUST be included "if present" — meaning if the TLV exists in the invoice byte stream. The previous check compared the parsed Bolt12InvoiceFeatures against empty(), which misses the case where another implementation serializes empty features (the TLV is present but the value has no bits set). Now scans the raw invoice bytes for INVOICE_FEATURES_TYPE. 2. Secp256k1::new() → Secp256k1::signing_only() in build_with_derived_key Secp256k1::new() allocates a full signing+verification context (~1MB). Since derive_payer_signing_keys and sign_schnorr_no_aux_rand only require a Signing context, use signing_only() instead. The verification step in sign() already creates its own verification_only context. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| impl TryFrom<Vec<u8>> for PayerProof { | ||
| type Error = crate::offers::parse::Bolt12ParseError; | ||
|
|
||
| fn try_from(bytes: Vec<u8>) -> Result<Self, Self::Error> { | ||
| use crate::ln::msgs::DecodeError; | ||
| use crate::offers::parse::Bolt12ParseError; | ||
|
|
||
| let mut payer_id: Option<PublicKey> = None; | ||
| let mut payment_hash: Option<PaymentHash> = None; | ||
| let mut issuer_signing_pubkey: Option<PublicKey> = None; | ||
| let mut invoice_signature: Option<Signature> = None; | ||
| let mut preimage: Option<PaymentPreimage> = None; | ||
| let mut payer_signature: Option<Signature> = None; | ||
| let mut payer_note: Option<String> = None; | ||
|
|
||
| let mut leaf_hashes: Vec<sha256::Hash> = Vec::new(); | ||
| let mut omitted_markers: Vec<u64> = Vec::new(); | ||
| let mut missing_hashes: Vec<sha256::Hash> = Vec::new(); | ||
|
|
||
| let mut included_types: BTreeSet<u64> = BTreeSet::new(); | ||
| let mut included_records: Vec<(u64, usize, usize)> = Vec::new(); | ||
|
|
||
| let mut prev_tlv_type: u64 = 0; | ||
|
|
||
| for record in TlvStream::new(&bytes) { |
There was a problem hiding this comment.
Bug (panic on untrusted input): TlvStream::new(&bytes) is used here on untrusted payer proof bytes. However, TlvStream's iterator implementation (merkle.rs:263, 267) uses .unwrap() on BigSize reads, and performs unchecked slice indexing (merkle.rs:271) where end = offset + length can exceed the buffer size. Both of these will panic on malformed input (truncated BigSize encoding, or a length field claiming more bytes than available).
All other TlvStream callers in the codebase operate on already-validated bytes (e.g., through ParsedMessage which uses CursorReadable::read that returns errors instead of panicking). This TryFrom is the only place TlvStream is used directly on untrusted external input, creating a DoS vector.
You should either:
- Add bounds checking to
TlvStream's iterator (returnNoneon malformed data), or - Pre-validate the TLV framing before calling
TlvStream::new, or - Use a different parsing approach (like
ParsedMessage) for the initial TLV framing pass.
| pub fn include_type(mut self, tlv_type: u64) -> Result<Self, PayerProofError> { | ||
| if tlv_type == PAYER_METADATA_TYPE { | ||
| return Err(PayerProofError::InvreqMetadataNotAllowed); | ||
| } | ||
| if SIGNATURE_TYPES.contains(&tlv_type) { | ||
| return Err(PayerProofError::SignatureTypeNotAllowed); | ||
| } | ||
| self.included_types.insert(tlv_type); | ||
| Ok(self) | ||
| } |
There was a problem hiding this comment.
Bug: This guard rejects PAYER_METADATA_TYPE (0) and SIGNATURE_TYPES (240..=1000), but allows types > 1000 (experimental BOLT 12 TLV types, e.g. 1000000000+). If an invoice contains experimental TLVs and the caller includes one via include_type(1000000001), the serialized payer proof will have those records written before the payer proof TLVs (types 240-250), producing an out-of-order TLV stream — the output would go from type >1000 back down to type 240.
Consider also rejecting types >= TLV_SIGNATURE (240), since the payer proof TLVs own that range:
| pub fn include_type(mut self, tlv_type: u64) -> Result<Self, PayerProofError> { | |
| if tlv_type == PAYER_METADATA_TYPE { | |
| return Err(PayerProofError::InvreqMetadataNotAllowed); | |
| } | |
| if SIGNATURE_TYPES.contains(&tlv_type) { | |
| return Err(PayerProofError::SignatureTypeNotAllowed); | |
| } | |
| self.included_types.insert(tlv_type); | |
| Ok(self) | |
| } | |
| pub fn include_type(mut self, tlv_type: u64) -> Result<Self, PayerProofError> { | |
| if tlv_type == PAYER_METADATA_TYPE { | |
| return Err(PayerProofError::InvreqMetadataNotAllowed); | |
| } | |
| if tlv_type >= TLV_SIGNATURE { | |
| return Err(PayerProofError::SignatureTypeNotAllowed); | |
| } | |
| self.included_types.insert(tlv_type); | |
| Ok(self) | |
| } |
| // Strict ascending order check covers both ordering and duplicates. | ||
| if tlv_type <= prev_tlv_type && prev_tlv_type != 0 { | ||
| return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)); | ||
| } |
There was a problem hiding this comment.
Nit/minor: The && prev_tlv_type != 0 condition means two consecutive type-0 TLVs would bypass this ordering check (since prev_tlv_type is initialized to 0). This doesn't cause a real issue because type 0 is separately rejected by the PAYER_METADATA_TYPE check below, but the ordering guard itself has a blind spot. Consider initializing differently or using an Option<u64>:
let mut prev_tlv_type: Option<u64> = None;
// ...
if let Some(prev) = prev_tlv_type {
if tlv_type <= prev {
return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue));
}
}
prev_tlv_type = Some(tlv_type);| pub(super) fn new( | ||
| invoice: &'a Bolt12Invoice, preimage: PaymentPreimage, | ||
| ) -> Result<Self, PayerProofError> { | ||
| let computed_hash = sha256::Hash::hash(&preimage.0); | ||
| if computed_hash.as_byte_array() != &invoice.payment_hash().0 { | ||
| return Err(PayerProofError::PreimageMismatch); | ||
| } | ||
|
|
||
| let mut included_types = BTreeSet::new(); | ||
| included_types.insert(INVOICE_REQUEST_PAYER_ID_TYPE); | ||
| included_types.insert(INVOICE_PAYMENT_HASH_TYPE); | ||
| included_types.insert(INVOICE_NODE_ID_TYPE); | ||
|
|
||
| // Per spec, invoice_features MUST be included "if present" — meaning if the | ||
| // TLV exists in the invoice byte stream, regardless of whether the parsed | ||
| // value is empty. Check the raw bytes so we handle invoices from other | ||
| // implementations that may serialize empty features. | ||
| let mut invoice_bytes = Vec::new(); | ||
| invoice.write(&mut invoice_bytes).expect("Vec write should not fail"); | ||
| let has_features_tlv = | ||
| TlvStream::new(&invoice_bytes).any(|r| r.r#type == INVOICE_FEATURES_TYPE); | ||
| if has_features_tlv { | ||
| included_types.insert(INVOICE_FEATURES_TYPE); | ||
| } | ||
|
|
||
| Ok(Self { invoice, preimage, included_types }) | ||
| } |
There was a problem hiding this comment.
Nit (performance): The invoice is serialized here (line 162-163) just to check if the features TLV exists, but it's serialized again in build_unsigned() (line 245-246). Consider storing the serialized bytes in the builder to avoid the redundant serialization, or deferring the features check to build_unsigned.
|
|
||
| let is_included = included_types.contains(&record.r#type); | ||
| if is_included { | ||
| leaf_hashes.push(nonce_hash); | ||
| } |
There was a problem hiding this comment.
Naming confusion: leaf_hashes stores nonce hashes (nonce_hash), not leaf hashes. In the BOLT 12 merkle tree, the "leaf hash" is H("LnLeaf" || record_bytes) while the "nonce hash" is H(nonce_tag || type_bytes) — these are distinct values. Storing nonce hashes under the name leaf_hashes here, in the SelectiveDisclosure struct, and in the TLV encoding (type 248 = TLV_LEAF_HASHES) is confusing.
If the spec calls these "leaf hashes," a code comment clarifying the distinction would help readers.
| TLV_PAYER_SIGNATURE => { | ||
| let mut record_cursor = io::Cursor::new(record.record_bytes); | ||
| let _type: BigSize = Readable::read(&mut record_cursor)?; | ||
| let len: BigSize = Readable::read(&mut record_cursor)?; | ||
| payer_signature = Some(Readable::read(&mut record_cursor)?); | ||
| let note_len = len.0.saturating_sub(64); | ||
| if note_len > 0 { | ||
| let mut note_bytes = vec![0u8; note_len as usize]; | ||
| record_cursor | ||
| .read_exact(&mut note_bytes) | ||
| .map_err(|_| DecodeError::ShortRead)?; | ||
| payer_note = Some( | ||
| String::from_utf8(note_bytes).map_err(|_| DecodeError::InvalidValue)?, | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Potential issue: When len.0 < 64, the Readable::read for the 64-byte Signature at line 592 will read past the TLV value boundary into subsequent TLV data (since record_cursor spans the full record_bytes which includes the type and length prefix, but the value is only len.0 bytes after the prefix). The Readable read doesn't know about the TLV length boundary — it just reads 64 bytes from the cursor.
In practice, record_bytes is bounded by TlvStream to exactly type_len + length_len + value_len bytes, so reading 64 bytes of signature when value_len < 64 would either read into the type/length prefix of the next TLV (if record_bytes was somehow longer) or fail with a short read. The short read case is safe, but consider adding an explicit check: if len.0 < 64 { return Err(...) } before reading the signature to make the error handling clearer.
5 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