[RFC] Add BOLT 12 payer proof primitives#4297
[RFC] Add BOLT 12 payer proof primitives#4297vincenzopalazzo wants to merge 6 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.88% 86.24% +0.36%
==========================================
Files 159 161 +2
Lines 104448 108548 +4100
Branches 104448 108548 +4100
==========================================
+ Hits 89706 93622 +3916
- Misses 12235 12256 +21
- Partials 2507 2670 +163
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
| .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.
lightning/src/offers/payer_proof.rs
Outdated
| 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.
lightning/src/offers/payer_proof.rs
Outdated
| 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.
lightning/src/offers/payer_proof.rs
Outdated
| 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.
lightning/src/offers/payer_proof.rs
Outdated
| 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.
| /// a run, and the first marker after an included type X must be X + 1 | ||
| fn validate_omitted_markers_for_parsing( | ||
| omitted_markers: &[u64], included_types: &BTreeSet<u64>, | ||
| ) -> Result<(), PayerProofError> { |
There was a problem hiding this comment.
Why do we need to use PayerProofError if the only caller replaces it with Bolt12ParseError::Decode?
| included_records.push(( | ||
| tlv_type, | ||
| record.end - record.record_bytes.len(), | ||
| record.end, | ||
| )); |
There was a problem hiding this comment.
It would be cleaner if included_records stored TlvRecords instead and have reconstruct_merkle_root take them, too.
| match tlv_type { | ||
| INVOICE_REQUEST_PAYER_ID_TYPE => { | ||
| 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_id = Some(Readable::read(&mut record_cursor)?); | ||
| 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.
Could we add as a method to TlvRecord? We've already read the bytes, so re-reading the bytes is redundant. We can always store a value_bytes field in TlvRecord to avoid re-parsing.
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
With inspiration from my last round of comments, I asked Claude if there are any more opportunities to simplify the code.
| 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; |
There was a problem hiding this comment.
Straight from Claude:
prev_included_type and prev_marker are mutually exclusive. The marker is always prev_value + 1 where prev_value tracks the last included type or last marker. The whole function reduces to:
fn compute_omitted_markers(tlv_data: &[TlvMerkleData]) -> Vec<u64> {
let mut markers = Vec::new();
let mut prev_value: u64 = 0;
for data in tlv_data {
if data.tlv_type == 0 { continue; }
if !data.is_included {
let marker = prev_value + 1;
markers.push(marker);
prev_value = marker;
} else {
prev_value = data.tlv_type;
}
}
markers
}| match (left_hash, right_hash, left_incl, right_incl) { | ||
| (Some(l), Some(r), true, false) => { | ||
| missing_with_types.push((right_min_type, r)); | ||
| nodes[left_pos].hash = | ||
| Some(tagged_branch_hash_from_engine(branch_tag.clone(), l, r)); | ||
| nodes[left_pos].included = true; | ||
| nodes[left_pos].min_type = | ||
| core::cmp::min(nodes[left_pos].min_type, right_min_type); | ||
| }, | ||
| (Some(l), Some(r), false, true) => { | ||
| missing_with_types.push((nodes[left_pos].min_type, l)); | ||
| let left_min = nodes[left_pos].min_type; | ||
| nodes[left_pos].hash = | ||
| Some(tagged_branch_hash_from_engine(branch_tag.clone(), l, r)); | ||
| nodes[left_pos].included = true; | ||
| nodes[left_pos].min_type = core::cmp::min(left_min, right_min_type); | ||
| }, | ||
| (Some(l), Some(r), true, true) => { | ||
| nodes[left_pos].hash = | ||
| Some(tagged_branch_hash_from_engine(branch_tag.clone(), l, r)); | ||
| nodes[left_pos].included = true; | ||
| nodes[left_pos].min_type = | ||
| core::cmp::min(nodes[left_pos].min_type, right_min_type); | ||
| }, | ||
| (Some(l), Some(r), false, false) => { | ||
| nodes[left_pos].hash = | ||
| Some(tagged_branch_hash_from_engine(branch_tag.clone(), l, r)); | ||
| nodes[left_pos].min_type = | ||
| core::cmp::min(nodes[left_pos].min_type, right_min_type); | ||
| }, | ||
| (Some(_), None, _, _) => {}, | ||
| _ => unreachable!("Invalid state in merkle tree construction"), | ||
| } |
There was a problem hiding this comment.
Also from Claude:
All four (Some(l), Some(r), ...) arms compute the branch hash and update min_type. The only differences: whether to push to missing_with_types and whether to set included = true. Could be restructured as:
let combined = tagged_branch_hash_from_engine(branch_tag.clone(), l, r);
if left_incl != right_incl {
let (missing_type, missing_hash) = if right_incl {
(nodes[left_pos].min_type, l)
} else {
(right_min_type, r)
};
missing_with_types.push((missing_type, missing_hash));
nodes[left_pos].included = true;
} else {
nodes[left_pos].included |= left_incl;
}
nodes[left_pos].hash = Some(combined);
nodes[left_pos].min_type = core::cmp::min(nodes[left_pos].min_type,
right_min_type);
lightning/src/offers/payer_proof.rs
Outdated
| TLV_PREIMAGE => { | ||
| 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)?; | ||
| let mut preimage_bytes = [0u8; 32]; | ||
| record_cursor | ||
| .read_exact(&mut preimage_bytes) | ||
| .map_err(|_| DecodeError::ShortRead)?; | ||
| preimage = Some(PaymentPreimage(preimage_bytes)); | ||
| }, | ||
| TLV_OMITTED_TLVS => { | ||
| 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)?; | ||
| let end_pos = record_cursor.position() + len.0; | ||
| while record_cursor.position() < end_pos { | ||
| let marker: BigSize = Readable::read(&mut record_cursor)?; | ||
| omitted_markers.push(marker.0); | ||
| } | ||
| }, | ||
| TLV_MISSING_HASHES => { | ||
| 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)?; | ||
| if len.0 % 32 != 0 { | ||
| return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)); | ||
| } | ||
| let num_hashes = len.0 / 32; | ||
| for _ in 0..num_hashes { | ||
| let mut hash_bytes = [0u8; 32]; | ||
| record_cursor | ||
| .read_exact(&mut hash_bytes) | ||
| .map_err(|_| DecodeError::ShortRead)?; | ||
| missing_hashes.push(sha256::Hash::from_byte_array(hash_bytes)); | ||
| } | ||
| }, | ||
| TLV_LEAF_HASHES => { | ||
| 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)?; | ||
| if len.0 % 32 != 0 { | ||
| return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue)); | ||
| } | ||
| let num_hashes = len.0 / 32; | ||
| for _ in 0..num_hashes { | ||
| let mut hash_bytes = [0u8; 32]; | ||
| record_cursor | ||
| .read_exact(&mut hash_bytes) | ||
| .map_err(|_| DecodeError::ShortRead)?; | ||
| leaf_hashes.push(sha256::Hash::from_byte_array(hash_bytes)); | ||
| } | ||
| }, | ||
| 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); |
There was a problem hiding this comment.
I'd prefer if we could implement Readable where possible. Here's what Claude told me:
PaymentPreimage already implements Readable (reads 32 bytes and wraps). So TLV_PREIMAGE could just be:
TLV_PREIMAGE => {
preimage = Some(record.read_value()?);
},Same pattern as TLV_SIGNATURE. The length validation is implicit — Readable for PaymentPreimage reads exactly 32 bytes and fails if the cursor is short.
For the hash lists, there's no Readable for sha256::Hash, but there could be — it's the same pattern as Sha256dHash, PaymentPreimage, PaymentHash,
and Hmac: read 32 bytes, wrap. If one were added, then TLV_MISSING_HASHES and TLV_LEAF_HASHES could use a cursor over value_bytes:
let mut cursor = io::Cursor::new(record.value_bytes);
while (cursor.position() as usize) < record.value_bytes.len() {
missing_hashes.push(Readable::read(&mut cursor)?);
}No manual chunks_exact / copy_from_slice / from_byte_array, and the len % 32 check becomes implicit (a short final read would error). That also deduplicates the two arms — they'd be identical except for the target vec.
For TLV_PAYER_SIGNATURE, read_value doesn't work directly because the value is a signature followed by an optional note — it's a composite encoding. But Readable::read from a cursor on value_bytes already handles the signature part, which is what's being done.
So the concrete opportunities:
TLV_PREIMAGE: replace withrecord.read_value()?(one line, likeTLV_SIGNATURE)TLV_MISSING_HASHES/TLV_LEAF_HASHES: addReadableforsha256::Hashinser.rs, then both arms become a cursor loop — or a shared helper
| BigSize(TLV_PREIMAGE).write(&mut bytes).expect("Vec write should not fail"); | ||
| BigSize(32).write(&mut bytes).expect("Vec write should not fail"); | ||
| bytes.extend_from_slice(&self.preimage.0); | ||
|
|
||
| if !self.disclosure.omitted_markers.is_empty() { | ||
| let omitted_len: u64 = self | ||
| .disclosure | ||
| .omitted_markers | ||
| .iter() | ||
| .map(|m| BigSize(*m).serialized_length() as u64) | ||
| .sum(); | ||
| BigSize(TLV_OMITTED_TLVS).write(&mut bytes).expect("Vec write should not fail"); | ||
| BigSize(omitted_len).write(&mut bytes).expect("Vec write should not fail"); | ||
| for marker in &self.disclosure.omitted_markers { | ||
| BigSize(*marker).write(&mut bytes).expect("Vec write should not fail"); | ||
| } | ||
| } | ||
|
|
||
| if !self.disclosure.missing_hashes.is_empty() { | ||
| let len = self.disclosure.missing_hashes.len() * 32; | ||
| BigSize(TLV_MISSING_HASHES).write(&mut bytes).expect("Vec write should not fail"); | ||
| BigSize(len as u64).write(&mut bytes).expect("Vec write should not fail"); | ||
| for hash in &self.disclosure.missing_hashes { | ||
| bytes.extend_from_slice(hash.as_ref()); | ||
| } | ||
| } | ||
|
|
||
| if !self.disclosure.leaf_hashes.is_empty() { | ||
| let len = self.disclosure.leaf_hashes.len() * 32; | ||
| BigSize(TLV_LEAF_HASHES).write(&mut bytes).expect("Vec write should not fail"); | ||
| BigSize(len as u64).write(&mut bytes).expect("Vec write should not fail"); | ||
| for hash in &self.disclosure.leaf_hashes { | ||
| bytes.extend_from_slice(hash.as_ref()); | ||
| } | ||
| } | ||
|
|
||
| let note_bytes = note.map(|n| n.as_bytes()).unwrap_or(&[]); | ||
| let payer_sig_len = 64 + note_bytes.len(); | ||
| BigSize(TLV_PAYER_SIGNATURE).write(&mut bytes).expect("Vec write should not fail"); | ||
| BigSize(payer_sig_len as u64).write(&mut bytes).expect("Vec write should not fail"); | ||
| payer_signature.write(&mut bytes).expect("Vec write should not fail"); | ||
| bytes.extend_from_slice(note_bytes); |
There was a problem hiding this comment.
Similarly here with Writeable.
lightning/src/offers/payer_proof.rs
Outdated
| payer_signature = Some(Readable::read(&mut record_cursor)?); | ||
| let note_len = len.0.saturating_sub(64); |
There was a problem hiding this comment.
Might make sense to make a composite type for this.
| } | ||
|
|
||
| /// The payer's note, if any. | ||
| pub fn payer_note(&self) -> Option<&str> { |
| /// - marker=42, prev=41: 42 == 42, continuation → omitted, prev=42 | ||
| /// Result: [O, I, O, O, I, O, O] | ||
| #[cfg(test)] | ||
| fn reconstruct_positions(included_types: &[u64], omitted_markers: &[u64]) -> Vec<bool> { |
There was a problem hiding this comment.
Seems like this duplicates much of reconstruct_merkle_root. What be great if we can re-use that somehow without introducing new Vec allocations in non-test code.
Fix five parsing safety issues found during review: 1. TlvStream panic on malformed input (CRITICAL): TlvStream::new() assumes well-formed input and panics on malformed BigSize or out-of-bounds lengths. Added validate_tlv_framing() pre-check before TlvStream::new() in TryFrom<Vec<u8>>, mirroring the validation ParsedMessage/CursorReadable provides for other BOLT 12 types. 2. include_type() allowed types > 1000: SIGNATURE_TYPES is 240..=1000, so experimental types > 1000 slipped through. Changed guard to reject all types >= 240 (TLV_SIGNATURE) since the entire range is reserved for signature/payer-proof TLVs. 3. Duplicate type-0 not detected: prev_tlv_type was initialized to 0 with a `!= 0` guard, so `if tlv_type <= prev_tlv_type && prev_tlv_type != 0` passed for two consecutive type-0 records. Changed to Option<u64>. 4. note_len truncation on 32-bit: `note_len as usize` truncates on 32-bit platforms. Changed to usize::try_from() with proper error. 5. Missing len < 64 check for payer_signature: saturating_sub(64) silently produced 0 for short values, masking malformed input. Added explicit check returning InvalidValue. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| // Count markers larger than largest included | ||
| if marker > max_included { | ||
| trailing_count += 1; | ||
| } | ||
|
|
||
| prev = marker; | ||
| } | ||
|
|
||
| // MUST NOT contain more than one number larger than largest included | ||
| if trailing_count > 1 { | ||
| return Err(PayerProofError::TooManyTrailingOmittedMarkers); | ||
| } |
There was a problem hiding this comment.
Bug: Producer-consumer mismatch on trailing_count. The compute_omitted_markers function (merkle.rs:384) produces one marker per omitted TLV after the last included type, with no limit. But this validation rejects proofs with trailing_count > 1.
This means build_unsigned can produce proofs that TryFrom<Vec<u8>> rejects. Concrete scenario:
If a BOLT 12 invoice has unknown/future invoice TLV types above 176 (the highest required included type INVOICE_NODE_ID_TYPE), those types are present in bytes_without_sig (since SIGNATURE_TYPES only filters 240..=1000) and will be omitted by default. Each generates a trailing marker > 176.
Example: Invoice has TLV types [0, 88, 168, 176, 178, 180] (types 178/180 are unknown future invoice types in the valid 160..240 range). Required included types: {88, 168, 176}, so max_included = 176. Markers for omitted types 178 and 180 would be [177, 178] — both > 176, so trailing_count = 2 → rejected.
This also applies to invoices with experimental invoice types (3_000_000_000+), which are not filtered by SIGNATURE_TYPES.
The trailing_count check and compute_omitted_markers need to agree — either the producer should cap trailing markers (merging multiple trailing omitted TLVs into a single marker), or the consumer should accept multiple trailing markers.
| fn build_unsigned(self) -> Result<UnsignedPayerProof, PayerProofError> { | ||
| let mut invoice_bytes = Vec::new(); | ||
| self.invoice.write(&mut invoice_bytes).expect("Vec write should not fail"); | ||
| let mut bytes_without_sig = Vec::with_capacity(invoice_bytes.len()); | ||
| for r in TlvStream::new(&invoice_bytes).filter(|r| !SIGNATURE_TYPES.contains(&r.r#type)) { | ||
| bytes_without_sig.extend_from_slice(r.record_bytes); | ||
| } | ||
|
|
||
| let disclosure = | ||
| merkle::compute_selective_disclosure(&bytes_without_sig, &self.included_types)?; |
There was a problem hiding this comment.
Nit: bytes_without_sig includes ALL non-signature TLVs from the invoice, including experimental invoice types (3_000_000_000+) and any unknown types in ranges 1001-999_999_999. These get processed by compute_selective_disclosure and will be omitted by default (since they're not in included_types). This interacts with the trailing_count > 1 validation bug noted on validate_omitted_markers_for_parsing — invoices with experimental types will produce unparseable proofs.
Consider documenting that the TLV stream fed to compute_selective_disclosure should match the set of TLVs that contribute to the invoice's merkle root, and ensuring the producer and consumer agree on the handling of trailing omitted types.
|
|
||
| // Build TreeNode vec directly by interleaving included/omitted positions, | ||
| // eliminating the intermediate Vec<bool> from reconstruct_positions_from_records. | ||
| let num_nodes = 1 + included_records.len() + omitted_markers.len(); | ||
| let mut nodes: Vec<TreeNode> = Vec::with_capacity(num_nodes); | ||
|
|
||
| // TLV0 is always omitted |
There was a problem hiding this comment.
The implicit TLV0 node prepended here assumes the original TLV stream starts with type 0 (payer_metadata). This is true for BOLT 12 invoices, but the function's API doesn't enforce or document this invariant.
If compute_selective_disclosure is called on a TLV stream that doesn't contain type 0 (as in the unit test test_selective_disclosure_computation with types [1, 2]), the producer creates a 2-node tree but this reconstruction creates a 3-node tree (with the extra TLV0 node), causing InsufficientMissingHashes because the needs_hash count won't match missing_hashes.
Consider either:
- Adding a
debug_assert!thatomitted_markersandincluded_recordsare consistent with the TLV0 assumption, or - Documenting on both
compute_selective_disclosureandreconstruct_merkle_rootthat the TLV stream MUST start with type 0.
| for inc_type in inc_iter.by_ref() { | ||
| if inc_type + 1 == marker { | ||
| found = true; | ||
| break; | ||
| } | ||
| if inc_type >= marker { | ||
| return Err(PayerProofError::InvalidData("omitted_markers not minimized")); | ||
| } | ||
| } | ||
| if !found { | ||
| return Err(PayerProofError::InvalidData("omitted_markers not minimized")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential issue: inc_type + 1 on line 781 can overflow if inc_type == u64::MAX. While include_type() rejects types >= 240, the parser's catch-all _ arm accepts any type < 240 as included — but an adversary who crafts a TLV with type 239 would have inc_type + 1 = 240, which is fine. However, this function is also called with included_types populated from parsed data. On 32-bit platforms or with unusual inputs, inc_type could theoretically be very large. Using inc_type.checked_add(1) would be defensive.
Similarly, expected_next = marker + 1 on line 794 would overflow if marker == u64::MAX. A subsequent marker != expected_next comparison against expected_next = 0 would always succeed (since markers must be > 0), causing the code to search for a matching included type rather than treating it as a continuation. While the ascending order check prevents marker == u64::MAX from being followed by another marker, the overflow is still unsound.
| 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.
The invoice is serialized here to check for the features TLV, and then serialized again in build_unsigned() (line 246-247). Since Bolt12Invoice::write just copies self.bytes, the result is deterministic and the double serialization is functionally correct — but it's an unnecessary allocation. Consider storing the serialized bytes in the builder or deferring the features check.
More importantly: this check uses TlvStream::new(&invoice_bytes) which iterates ALL TLVs. But INVOICE_FEATURES_TYPE (174) is in the invoice range — the any() call will short-circuit after finding it, but it still scans through all preceding TLVs (offer, invreq, etc.). A TlvStream::new(&invoice_bytes).range(INVOICE_TYPES) would be more targeted.
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