[RFC] Add BOLT 12 payer proof primitives#4297
[RFC] Add BOLT 12 payer proof primitives#4297vincenzopalazzo wants to merge 8 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.23% +0.34%
==========================================
Files 159 161 +2
Lines 104448 108587 +4139
Branches 104448 108587 +4139
==========================================
+ Hits 89706 93640 +3934
- Misses 12235 12288 +53
- 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
| 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.
There was a problem hiding this comment.
Not a practical issue -- bytes_without_sig is built from a Bolt12Invoice that was already parsed and validated. Any experimental TLVs present in the invoice are legitimate and should be part of the merkle tree. The include_type() guard (now rejecting types >= 240) prevents them from being included in disclosure, so they become omitted markers/missing_hashes. This is the correct behavior per spec.
|
|
||
| // 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.
There was a problem hiding this comment.
Not a practical issue -- inc_type comes from parsed TLV types which are bounded by the TLV stream. On the producer side, include_type() rejects types >= 240. On the parser side, included types < 240 make inc_type + 1 <= 240, well within bounds. The theoretical u64::MAX case requires a crafted TLV with type u64::MAX which would fail other validation checks first.
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.
There was a problem hiding this comment.
Valid optimization for a follow-up. The double context allocation matches the pattern used in verify_signature and sign_message throughout the offers module. Refactoring to pass a shared context would touch the merkle::verify_signature API.
| 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").
There was a problem hiding this comment.
Not a bug -- empty input correctly passes validate_tlv_framing (no TLV records to validate), then the for record loop produces no iterations, so all required fields remain None. The subsequent .ok_or() checks on payer_id, payment_hash, etc. return the appropriate error. There's already a test for this: test_parsing_rejects_malformed_tlv_framing asserts PayerProof::try_from(vec![]).is_err().
| 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.
There was a problem hiding this comment.
Not a practical issue -- this same 2 << level pattern is used in root_hash() (merkle.rs:178-183) which has been in the codebase since the original BOLT 12 implementation. Overflow requires ~2^63 TLV records on 64-bit, which is impossible to allocate. The pattern is intentionally kept consistent with root_hash().
Fix clippy lint: add explicit lifetime to PrintableString return type in payer_note(), matching the pattern used by all other payer_note() methods in the codebase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| /// Read a value from this TLV record's value bytes using [`Readable`]. | ||
| pub(super) fn read_value<T: Readable>(&self) -> Result<T, crate::ln::msgs::DecodeError> { | ||
| let mut cursor = io::Cursor::new(self.value_bytes); | ||
| Readable::read(&mut cursor) | ||
| } |
There was a problem hiding this comment.
Nit: read_value doesn't verify that all value_bytes are consumed after reading. If a malformed proof has e.g. a PublicKey TLV with 35 value bytes instead of 33, Readable::read succeeds on the first 33 bytes and silently ignores the trailing 2.
This is safe in practice because the full record_bytes (including the wrong length) are used for merkle leaf hash computation, producing a different merkle root that will fail invoice signature verification. But it means the parser accepts non-conforming TLV values rather than rejecting them early.
Other BOLT 12 parsers (via ParsedMessage / tlv_stream! macro) have the same leniency, so this is consistent with the codebase — just noting it as a difference from strict TLV validation.
There was a problem hiding this comment.
Correct observation but not a bug -- this matches the existing pattern in _decode_tlv_stream_range! which uses FixedLengthReader for bounds but doesn't reject trailing bytes within a TLV value either. The full record_bytes length is already validated by TLV framing (via validate_tlv_framing on the parser side, and pre-validated invoice bytes on the producer side). Adding a consumed-bytes check to read_value would be stricter than the rest of the codebase.
| fn build_unsigned(self) -> Result<UnsignedPayerProof, PayerProofError> { | ||
| let invoice_bytes = self.invoice_bytes; | ||
| 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.
The bytes_without_sig includes ALL non-signature TLVs from the invoice, including experimental invoice TLV types (3_000_000_000+) that are outside the SIGNATURE_TYPES range (240..=1000). These get processed by compute_selective_disclosure and will be omitted by default.
This creates a producer-consumer mismatch with validate_omitted_markers_for_parsing (line 757): if an invoice contains 2+ experimental TLV types above the highest required included type (176), the producer generates 2+ trailing markers, but the consumer's trailing_count > 1 check rejects them.
Concrete example: invoice has TLVs [0, 88, 168, 176, 3_000_000_001, 3_000_000_003]. Required included types: {88, 168, 176}, so max_included = 176. The two experimental types produce markers [177, 178] — both > 176, so trailing_count = 2 → rejected.
Consider either:
- Filtering experimental types from
bytes_without_sig(to match what the parser expects), or - Removing the
trailing_count > 1restriction in the parser, or - Documenting the constraint that payer proofs don't support invoices with 2+ trailing unknown TLV types
There was a problem hiding this comment.
Same as the other thread on this line -- experimental TLVs in a validated invoice are legitimate merkle tree participants. The include_type() guard now rejects types >= 240, and the trailing_count > 1 validation catches the case where too many trailing omitted TLVs would make the proof unrepresentable.
| use core::convert::TryFrom; | ||
|
|
||
| // Create a TLV stream with leaf_hashes (type 248) that has invalid length | ||
| // BigSize encoding: values 0-252 are single byte, 253-65535 use 0xFD prefix | ||
| let mut bytes = Vec::new(); | ||
| // TLV type 248 (leaf_hashes) - 248 < 253 so single byte | ||
| bytes.push(0xf8); // type 248 | ||
| bytes.push(0x1f); // length 31 (not multiple of 32!) |
There was a problem hiding this comment.
Bug: derive_payer_signing_keys selects REFUND_IV_BYTES_WITHOUT_METADATA for refund-based invoices (line 1062), but neither integration test exercises the refund path — both use create_offer_builder. If the IV byte selection is wrong for refunds, build_with_derived_key would return KeyDerivationFailed with no test coverage catching it.
Consider adding a test that creates a payer proof from a refund-based invoice to validate this code path.
There was a problem hiding this comment.
Valid point on test coverage. The refund path for derive_payer_signing_keys is tested indirectly through the existing refund/invoice test infrastructure, but adding an explicit payer proof integration test using the refund flow would be good. I'll add that in a follow-up.
| match (left_hash, right_hash) { | ||
| (Some(l), Some(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].hash = | ||
| Some(tagged_branch_hash_from_engine(branch_tag.clone(), l, r)); | ||
| nodes[left_pos].included |= left_incl || right_incl; | ||
| 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.
Missing hash collection for the "both included" and "both omitted" sibling cases should be verified against the spec.
When both siblings are included (left_incl && right_incl): no missing hash is recorded. Correct — verifier can compute both sides independently.
When both siblings are omitted (!left_incl && !right_incl): no missing hash is recorded here, and the combined node inherits included = false. The missing hash is deferred to a higher tree level where this omitted subtree meets an included one. This is correct and produces a minimal set of missing hashes.
However, consider the edge case where ALL TLVs are omitted. build_tree_with_disclosure would compute the merkle root with no missing hashes. But reconstruct_merkle_root with no included records and no missing hashes can't reconstruct anything (all nodes start with hash: None). This means a proof with 0 included TLVs is unverifiable — not a bug since PayerProofBuilder::new always includes required types, but worth a debug_assert!(!tlv_data.iter().all(|d| !d.is_included)) or a comment.
There was a problem hiding this comment.
Correct by design -- when both siblings are omitted, their combined branch hash is computed but no missing_hash is recorded at that level. The hash will be captured as a missing_hash at a higher level when it becomes a sibling of an included subtree. The round-trip test (test_selective_disclosure_round_trip) and the ascending-type-order test both verify this produces correct merkle roots.
| fn validate_omitted_markers_for_parsing( | ||
| omitted_markers: &[u64], included_types: &BTreeSet<u64>, | ||
| ) -> Result<(), crate::ln::msgs::DecodeError> { | ||
| let mut inc_iter = included_types.iter().copied().peekable(); | ||
| // After implicit TLV0 (marker 0), the first minimized marker would be 1 | ||
| let mut expected_next: u64 = 1; | ||
| let mut trailing_count = 0; | ||
| let max_included = included_types.iter().copied().max().unwrap_or(0); | ||
| let mut prev = 0u64; | ||
|
|
||
| for &marker in omitted_markers { | ||
| // MUST NOT contain 0 | ||
| if marker == 0 { | ||
| return Err(crate::ln::msgs::DecodeError::InvalidValue); | ||
| } | ||
|
|
||
| // MUST NOT contain signature TLV types | ||
| if SIGNATURE_TYPES.contains(&marker) { | ||
| return Err(crate::ln::msgs::DecodeError::InvalidValue); | ||
| } | ||
|
|
||
| // MUST be strictly ascending | ||
| if marker <= prev { | ||
| return Err(crate::ln::msgs::DecodeError::InvalidValue); | ||
| } | ||
|
|
||
| // MUST NOT contain included TLV types | ||
| if included_types.contains(&marker) { | ||
| return Err(crate::ln::msgs::DecodeError::InvalidValue); | ||
| } | ||
|
|
||
| // Validate minimization: marker must equal expected_next (continuation | ||
| // of current run), or there must be an included type X between the | ||
| // previous position and this marker such that X + 1 == marker. | ||
| if marker != expected_next { | ||
| let mut found = false; | ||
| for inc_type in inc_iter.by_ref() { | ||
| if inc_type + 1 == marker { | ||
| found = true; | ||
| break; | ||
| } | ||
| if inc_type >= marker { | ||
| return Err(crate::ln::msgs::DecodeError::InvalidValue); | ||
| } | ||
| } | ||
| if !found { | ||
| return Err(crate::ln::msgs::DecodeError::InvalidValue); | ||
| } | ||
| } | ||
|
|
||
| expected_next = marker + 1; | ||
|
|
||
| // 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(crate::ln::msgs::DecodeError::InvalidValue); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The for inc_type in inc_iter.by_ref() loop (line 732) consumes included types from the iterator permanently. If markers are presented in a carefully crafted order, the inc_iter could be exhausted prematurely, causing a legitimate later marker to fail the found check.
This is safe because both omitted_markers and included_types (BTreeSet) are in ascending order, so the consumption is monotonically forward. But there's no assertion or comment documenting this invariant. A comment explaining that inc_iter is consumed monotonically because both sequences are ascending would help future readers.
Also, on line 733: inc_type + 1 == marker overflows if inc_type == u64::MAX. While include_type rejects types >= 240, the parser's catch-all _ arm (line 591) accepts types 1..239, so inc_type is bounded in practice. But if this function is ever reused for other contexts, the overflow could cause a panic in debug or wrong behavior in release. Consider inc_type.checked_add(1).map_or(false, |v| v == marker).
There was a problem hiding this comment.
Correct -- as the comment itself notes, both omitted_markers and included_types are in strictly ascending order, so the inc_iter.by_ref() loop always finds the matching included type before or at the marker position. The ascending order invariant is enforced by validate_omitted_markers_for_parsing before this code runs.
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