[RFC] Add BOLT 12 payer proof primitives#4297
[RFC] Add BOLT 12 payer proof primitives#4297vincenzopalazzo wants to merge 3 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4297 +/- ##
==========================================
+ Coverage 86.20% 87.20% +1.00%
==========================================
Files 160 164 +4
Lines 107545 110168 +2623
Branches 107545 110168 +2623
==========================================
+ Hits 92707 96070 +3363
+ Misses 12214 11569 -645
+ Partials 2624 2529 -95
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
There was a problem hiding this comment.
A few notes, though I didn't dig into the code at a particularly low level.
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
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. |
fb8c68c to
9ad5c35
Compare
| 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.
88ffb7c to
84d2374
Compare
|
Quick update after the latest push. I cherry-picked the follow-up fixes that came out of the Codex/spec review. What changed:
I also updated the external test vectors here: At this point, the real review bugs found around the producer/parser mismatch are fixed on the branch. The one discussion I still consider open is the The other remaining review threads look like follow-up material to me (TLV0 invariant docs, parser shape, secp context reuse), not missing correctness fixes. |
Move the invoice/refund payer key derivation logic into reusable helpers so payer proofs can derive the same signing keys without duplicating the metadata and signer flow.
c96cef5 to
d5baee8
Compare
lightning/src/offers/payer_proof.rs
Outdated
| 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.
Why can't we implement Writable for sha256::Hash?
| } else { | ||
| let marker = prev_value + 1; | ||
| markers.push(marker); | ||
| prev_value = marker; | ||
| } |
There was a problem hiding this comment.
Bug (producer-consumer mismatch): compute_omitted_markers generates markers sequentially via prev_value + 1 with no awareness of the SIGNATURE_TYPES (240..=1000) hole. Both validate_omitted_markers (line 662) and validate_omitted_markers_for_parsing (line 833) reject markers that fall in this range.
This means if an invoice has enough omitted TLVs after the last included type that sequential markers reach 240, the producer generates an invalid proof that the consumer rejects. Concretely:
- Required included types max out at 176 (
INVOICE_NODE_ID_TYPE) - Omitted invoice types in 178..238 contribute ~31 positions, generating markers 177..207
- Each additional omitted experimental type (>1000) adds one more marker: 208, 209, ..., 239, 240 (rejected!)
- Only 33 experimental invoice types beyond the 178..238 range triggers this
While unlikely today, this is a correctness gap: the producer and consumer disagree on how markers interact with the reserved range. The fix depends on how the spec handles it — either the producer needs to skip markers in SIGNATURE_TYPES (jumping from 239 to 1001), or the consumer needs to allow them. Both sides need to agree.
Consider adding a debug_assert that no generated markers fall in SIGNATURE_TYPES to catch this during testing, and a runtime check or skip logic once the spec clarifies the behavior.
lightning/src/offers/payer_proof.rs
Outdated
| /// `TlvStream::new()` assumes well-formed input and panics on malformed BigSize | ||
| /// values or out-of-bounds lengths. This function validates the framing first, | ||
| /// returning an error instead of panicking on untrusted input. | ||
| fn validate_tlv_framing(bytes: &[u8]) -> Result<(), crate::ln::msgs::DecodeError> { |
There was a problem hiding this comment.
Could we define this on TlvStream even if only an is_valid static method? Maybe as a follow-up we can build it into TlvStream or have some wrapper for performing validation.
There was a problem hiding this comment.
Make sense to me, I will not resolve this review also if I moved the is_valid to remember to open a new tracking issue for this
jkczyz
left a comment
There was a problem hiding this comment.
Made some of the more involved requests in https://github.com/jkczyz/rust-lightning/tree/macros/proof-of-payment-bolt12-spec. Didn't want to push anything directly to your branch in case you were still making changes. But feel free to grab those commits. They are referenced in my comments.
lightning/src/offers/payer_proof.rs
Outdated
| 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.
Let's take jkczyz@34aaab3 so we can reuse as much code as possible.
Add the payer proof types, selective disclosure merkle support, parsing, and tests for constructing and validating BOLT 12 payer proofs from invoices.
6739c39 to
b52a5f3
Compare
Encapsulate invoice, preimage, and nonce in PaidBolt12Invoice and surface it in PaymentSent. Rework builder to return UnsignedPayerProof with SignFn/sign_message integration, use encode_tlv_stream! for serialization, move helpers to DisclosedFields methods, and address naming conventions and TLV validation feedback. Co-Authored-By: Jeffrey Czyz <jkczyz@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b52a5f3 to
c836395
Compare
|
Thanks @jkczyz I should have addressed everything that you asked me! |
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