[RFC] Add BOLT 12 payer proof primitives#4297
[RFC] Add BOLT 12 payer proof primitives#4297vincenzopalazzo wants to merge 7 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 108586 +4138
Branches 104448 108586 +4138
==========================================
+ Hits 89706 93655 +3949
- Misses 12235 12272 +37
- Partials 2507 2659 +152
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
| /// 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>
| 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".
There was a problem hiding this comment.
Added spec reference comment. The construction follows the spec: payer_signature.sig signs msg = SHA256(note || merkle_root) using the standard BOLT 12 tagged hash.
| // 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.
There was a problem hiding this comment.
Known open question. The spec example omits TLV 60's hash — our 4 is correct per the tree structure. Will follow up with spec authors on BOLT PR 1295.
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>
|
|
||
| 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.
There was a problem hiding this comment.
Added a clarifying comment noting that these are nonce hashes, named leaf_hashes to match the spec's TLV type 248.
| 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.
There was a problem hiding this comment.
Partially addressed in 3378fa3 — added value_bytes to TlvRecord and read_value() method. The included_records still stores (u64, usize, usize) offsets rather than TlvRecords because TlvRecord borrows from the byte slice and can't outlive the iterator loop. Storing offsets and indexing bytes[start..end] after the loop is the borrow-checker-friendly approach.
| 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.
lightning/src/offers/merkle.rs
Outdated
| 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
}There was a problem hiding this comment.
Done in 3378fa3. Simplified to single prev_value tracking variable.
lightning/src/offers/merkle.rs
Outdated
| 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);There was a problem hiding this comment.
Done in 3378fa3. Collapsed into a single (Some(l), Some(r)) arm with conditional missing_hash push and included |= left_incl || right_incl.
| 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.
There was a problem hiding this comment.
Addressed as part of the TlvRecord changes. Serialization already uses Writeable for BigSize and Signature. Hash lists use extend_from_slice since sha256::Hash doesn't impl Writeable.
There was a problem hiding this comment.
Partially addressed in 3378fa3 — the serialization still uses manual BigSize().write() + extend_from_slice() for the payer proof TLVs since they have non-standard formats (concatenated hashes, BigSize lists). The included invoice records use TlvRecord::write() via the existing Writeable impl.
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.
There was a problem hiding this comment.
Deferred — creating a composite type requires implementing Readable/Writeable for sha256::Hash which isn't available. The current approach of iterating value_bytes with manual reads is practical for now.
There was a problem hiding this comment.
I'll defer this for a follow-up. The hash list encoding (concatenated 32-byte hashes without individual length prefixes) doesn't match standard Readable/Writeable for Vec<sha256::Hash>, so a custom wrapper would be needed. Happy to add it if you feel strongly.
| /// - 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.
There was a problem hiding this comment.
The test helper and reconstruct_merkle_root serve different purposes — one builds Vec<bool> positions, the other TreeNodes with hashes. Sharing the interleaving logic without Vec allocations in non-test code would need a callback pattern that adds more complexity than it removes.
There was a problem hiding this comment.
Looking at this more carefully, reconstruct_positions (test-only) and reconstruct_merkle_root share the interleaving logic but diverge significantly: one builds a Vec<bool>, the other builds TreeNodes with hashes. Extracting the shared part would require a callback/closure pattern that's arguably harder to follow. Since reconstruct_positions is #[cfg(test)] only, I'd lean towards keeping them separate for clarity. Happy to revisit if you disagree.
|
|
||
| // 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.
There was a problem hiding this comment.
Investigating — there's a conflict between spec text ('more than one number larger than...') and the spec example which has [41, 42] both > 40. Will follow up on BOLT PR 1295.
| 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.
There was a problem hiding this comment.
Same as above — TLV0 is always prepended in payer proof context. The function is private (pub(super)) and only called from TryFrom<Vec<u8>> which always has TLV0. Adding a doc comment noting the invariant.
| 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.
lightning/src/offers/payer_proof.rs
Outdated
| 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.
There was a problem hiding this comment.
Noted — the double serialization is functionally correct since Bolt12Invoice::write just copies self.bytes. Deferring optimization.
There was a problem hiding this comment.
Fixed in 3378fa3 — same change, serialize once in new().
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>
vincenzopalazzo
force-pushed
the
macros/proof-of-payment-bolt12-spec
branch
from
712d3fc to
64c7f9c
Compare
Address jkczyz review feedback: - Add `value_bytes` field and `read_value()` method to `TlvRecord`, eliminating the `read_tlv_value` helper. TLV parsing in TryFrom now uses `record.read_value()` for known types and `record.value_bytes` for hash lists and omitted markers, avoiding redundant type+length re-parsing. (Threads 2, 5, 19, 22, 49) - Simplify `compute_omitted_markers` from dual tracking variables (`prev_included_type` / `prev_marker`) to a single `prev_value`, since they are mutually exclusive. (Thread 20, 47) - Simplify `build_tree_with_disclosure` match: all four `(Some, Some)` arms shared the same branch hash + min_type update. Collapsed into a single arm with conditional missing_hash push. (Thread 21, 48) - Use `PrintableString` for `payer_note()` accessor return type, matching the pattern used by other BOLT 12 types. (Thread 25, 52) - Change `validate_omitted_markers_for_parsing` to return `DecodeError` directly instead of `PayerProofError`, since the only caller maps to `Bolt12ParseError::Decode` anyway. Removed unused error variants `InvalidData`, `MissingRequiredField`, `OmittedMarkersContainIncluded`, `TooManyTrailingOmittedMarkers`. (Thread 18, 45) - Avoid double serialization of invoice bytes: serialize once in `PayerProofBuilder::new()` and store in the builder, reusing in `build_unsigned()`. (Thread 16, 31, 42, 58) - Remove unused `crate::io::Read` import. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| // MUST NOT contain signature TLV types | ||
| if SIGNATURE_TYPES.contains(&marker) { | ||
| return Err(crate::ln::msgs::DecodeError::InvalidValue); |
There was a problem hiding this comment.
Nit (performance): The parser allocates two Secp256k1::verification_only() contexts (~0.6 MB each): one inside merkle::verify_signature (line 706) and another here. Consider passing a single context to both verification calls — e.g., by inlining the verify_signature logic or accepting a context parameter:
let secp_ctx = Secp256k1::verification_only();
// Verify invoice signature
let tagged_hash = TaggedHash::from_merkle_root(SIGNATURE_TAG, merkle_root);
let issuer_xonly: XOnlyPublicKey = issuer_signing_pubkey.into();
secp_ctx.verify_schnorr(&invoice_signature, tagged_hash.as_digest(), &issuer_xonly)
.map_err(|_| Bolt12ParseError::Decode(DecodeError::InvalidValue))?;
// Verify payer signature
let message = UnsignedPayerProof::compute_payer_signature_message(...);
secp_ctx.verify_schnorr(&payer_signature, &message, &payer_id.into())
.map_err(|_| Bolt12ParseError::Decode(DecodeError::InvalidValue))?;This halves the heap allocation for signature verification during parsing.
| 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; |
There was a problem hiding this comment.
Bug: Empty bytes input passes validate_tlv_framing (the while loop body is never entered), then TlvStream::new(&bytes) produces an empty iterator. The for record in TlvStream::new(&bytes) loop doesn't execute, so all required fields (payer_id, payment_hash, etc.) remain None. The subsequent .ok_or(...) checks correctly return errors.
However, this is a minor robustness concern — consider adding an explicit early return for empty input before the TlvStream iteration to provide a clearer error:
if bytes.is_empty() {
return Err(Bolt12ParseError::Decode(DecodeError::InvalidValue));
}This avoids running the rest of the parser and producing potentially confusing error messages (e.g., MissingPayerSigningPubkey instead of a more descriptive "empty proof").
| hash: Some(data.per_tlv_hash), | ||
| included: data.is_included, | ||
| min_type: data.tlv_type, | ||
| }) |
There was a problem hiding this comment.
Potential overflow: 2 << level will overflow usize when level >= usize::BITS - 1 (63 on 64-bit, 31 on 32-bit). While this can't happen in practice (would require ~2^31 TLV records on 32-bit, impossible to allocate), the same pattern appears in four places:
build_tree_with_disclosure(here)reconstruct_merkle_rootfirst pass (line 587)reconstruct_merkle_rootsecond pass (line 634)root_hash(existing code, line 179)
Consider using 1usize.checked_shl(level + 1) and breaking on None, or simply adding if level >= usize::BITS as u32 - 1 { break; } as a safety guard. This would make the code robust against pathological inputs regardless of platform.
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