-
Notifications
You must be signed in to change notification settings - Fork 431
[RFC] Add BOLT 12 payer proof primitives #4297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[RFC] Add BOLT 12 payer proof primitives #4297
Conversation
|
I've assigned @valentinewallace as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4297 +/- ##
==========================================
- Coverage 86.61% 86.36% -0.26%
==========================================
Files 158 159 +1
Lines 102730 103699 +969
Branches 102730 103699 +969
==========================================
+ Hits 88984 89556 +572
- Misses 11328 11708 +380
- Partials 2418 2435 +17
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes, though I didn't dig into the code at a particularly low level.
lightning/src/offers/payer_proof.rs
Outdated
| const TLV_INVREQ_PAYER_ID: u64 = 88; | ||
| const TLV_INVOICE_PAYMENT_HASH: u64 = 168; | ||
| const TLV_INVOICE_FEATURES: u64 = 174; | ||
| const TLV_INVOICE_NODE_ID: u64 = 176; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: most of these constants can/should be reused elsewhere in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in b16dc60 - now importing and using INVOICE_REQUEST_PAYER_ID_TYPE from invoice_request.rs. Also added a TODO for the invoice TLV types (168, 174, 176) which could potentially be exported from invoice.rs.
lightning/src/offers/payer_proof.rs
Outdated
| struct PayerProofContents { | ||
| payer_id: PublicKey, | ||
| payment_hash: PaymentHash, | ||
| invoice_node_id: PublicKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we refer to this as the "issuer signing pubkey" elsewhere to avoid overloading with the "node id" (the p2p thing) cause we (at a minimum, I guess CLN does not) try hard to avoid using the node id again in bolt 12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e609ea5 - renamed invoice_node_id to issuer_signing_pubkey throughout (field, constant, accessor method, and doc comments).
| self.included_types.insert(164); | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth having a generic "select by id" thing (maybe requiring it be in the experimental range?), given we'll add support for custom TLVs eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already an include_type(tlv_type: u64) method that allows including any TLV by its type number (except invreq_metadata which is forbidden by spec). Is that what you're looking for, or did you want something more restrictive that only allows experimental types (>= 1,000,000,000)?
| } | ||
|
|
||
| /// Sign the proof with the payer's key to create a complete proof. | ||
| pub fn sign<F>(self, sign_fn: F, note: Option<&str>) -> Result<PayerProof, PayerProofError> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also have a signing method which uses the automated payer key derivation from an ExpandedKey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 7b523bf - new sign_with_derived_key(expanded_key, nonce, note) method that derives the payer signing key using derive_keys() and signs the proof. It also validates that the derived key matches the expected payer_id.
lightning/src/offers/merkle.rs
Outdated
| break; | ||
| } | ||
|
|
||
| let left_positions: Vec<_> = (0..num_leaves).step_by(step).collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, please don't collect to build a vec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9f84e19 - now using direct iterator zipping like root_hash() does:
for (left_pos, right_pos) in (0..num_leaves).step_by(step).zip((offset..num_leaves).step_by(step)) {| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct SelectiveDisclosure { | ||
| /// Nonce hashes for included TLVs (in TLV type order). | ||
| pub leaf_hashes: Vec<sha256::Hash>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, don't we need the tlv id in this? This doesn't look sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation stores nonce hashes without TLV type IDs. The types come from included_records passed separately to reconstruct_merkle_root(), and we match by position (both in ascending TLV type order).
I think what might "not look sufficient" is that SelectiveDisclosure isn't self-contained - you need the accompanying included_records to know which TLV type each hash belongs to. Looking at the struct alone, there's no way to know what each hash corresponds to.
Would you prefer making this explicit by storing (tlv_type, hash) pairs instead?
pub nonce_hashes: Vec<(u64, sha256::Hash)>, // (tlv_type, nonce_hash) pairsThis would make the struct self-describing. Let me know your thoughts.
| let mut is_included: Vec<bool> = vec![false; num_leaves]; | ||
| let mut min_types: Vec<u64> = vec![u64::MAX; num_leaves]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to avoid the unnecessary allocations here.
| let branch_tag = tagged_hash_engine(sha256::Hash::hash("LnBranch".as_bytes())); | ||
|
|
||
| let mut hashes: Vec<Option<sha256::Hash>> = vec![None; num_leaves]; | ||
| let mut is_included: Vec<bool> = vec![false; num_leaves]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, there's lot of allocations that we should be able to avoid.
| #[allow(dead_code)] | ||
| leaf_hashes: Vec<sha256::Hash>, | ||
| #[allow(dead_code)] | ||
| omitted_tlvs: Vec<u64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be revealed, only the presence of N TLV(s) between X (the included TLV below) and Y (the included TLV above) should be required, no? Its a bit of a privacy leak to reveal the set of TLVs in some cases, I can imagine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation already does this if I understand your comment correctly - the field stores privacy-preserving markers, not actual TLV type numbers. Looking at the spec example:
- TLVs: 0(omit), 10(incl), 20(omit), 30(omit), 40(incl), 50(omit), 60(omit)
- Markers stored:
[11, 12, 41, 42]
The marker algorithm (in compute_omitted_markers):
- After included TLV type X, the first omitted TLV gets marker X+1
- Consecutive omitted TLVs get marker prev_marker+1
So markers [11, 12] only reveal "2 omitted TLVs after type 10" and [41, 42] reveals "2 omitted TLVs after type 40" - it does not reveal that types 20, 30, 50, 60 were the actual omitted types.
The naming omitted_tlvs is confusing though - I can rename it to omitted_markers to make it clearer that these aren't actual TLV types. Does that address your concern, or did I misunderstand?
| invoice_created_at: Option<Duration>, | ||
| #[allow(dead_code)] | ||
| invoice_features: Option<Bolt12InvoiceFeatures>, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We presumably want a place to store custom included TLVs.
84597cc to
2324361
Compare
Implements the payer proof extension to BOLT 12 as specified in lightning/bolts#1295. This allows proving that a BOLT 12 invoice was paid by demonstrating possession of the payment preimage, a valid invoice signature, and a payer signature. Key additions: - Extend merkle.rs with selective disclosure primitives for creating and reconstructing merkle trees with partial TLV disclosure - Add payer_proof.rs with PayerProof, PayerProofBuilder, and UnsignedPayerProof types for building and verifying payer proofs - Support bech32 encoding with "lnp" prefix
2324361 to
9f84e19
Compare
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