RFC for PoC of bolt12 contacts formally known as a BLIP 42#4210
RFC for PoC of bolt12 contacts formally known as a BLIP 42#4210vincenzopalazzo wants to merge 5 commits into
Conversation
|
👋 I see @jkczyz was un-assigned. |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
@TheBlueMatt the status of this PR is kind doing everything that the specs specifieds, but I am taking some opinionate decision like on the offer to inject inside the invoice_request (that I am calling in my mental model
|
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
A handful of high-level comments. Apologies if there's something I missed cause I don't recall exactly how all the bLIP 42 stuff was supposed to work.
| /// [`Event::PaymentFailed`]). | ||
| pub retry_strategy: Retry, | ||
| /// Contact secrets to include in the invoice request for BLIP-42 contact management. | ||
| /// If provided, these secrets will be used to establish a contact relationship with the recipient. |
There was a problem hiding this comment.
This needs to be substantially more filled-out, including information about intended UX and UI integration logic, how/when to derive secrets, etc.
| /// | ||
| /// Since the second case should be very infrequent, it's more likely that the remote node | ||
| /// is malicious and we shouldn't store them in our contacts list. | ||
| pub fn verify(&self, offer: &Offer) -> bool { |
There was a problem hiding this comment.
nit: verify methods should never return a bool, they should Result<(), ()>.
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct ContactSecrets { | ||
| primary_secret: [u8; 32], | ||
| additional_remote_secrets: Vec<[u8; 32]>, |
There was a problem hiding this comment.
Should this be a single Option? I'm a bit unclear on what UX would result in there being more than one.
There was a problem hiding this comment.
We should have an additional one for when the country party will not remember us, but use somehow the same offer, so we will use this vector to store additional secrets that the other side sends to us. IIRC this should be the logic
| } | ||
| } | ||
|
|
||
| /// We derive our contact secret deterministically based on our offer and our contact's offer. |
There was a problem hiding this comment.
I'm a bit unclear on this usecase here - using our_private_key (I guess it implies the node_id?) means that it won't match what we put in offers (we should never be reusing the node_id as an offer identity, IMO), but in general we should be using a fresh identity for every issued offer, so its not clear to me how often this will work. Also, we don't currently use it, just expose it as a freestanding function. If we want to support this derivation IMO we should integrate it somehow into the API.
| /// - our contact added us without using the contact_secret we initially sent them | ||
| /// - our contact is using a different wallet from the one(s) we have already stored |
There was a problem hiding this comment.
This doesn't tell me, a wallet developer, what to do here - am I supposed to have some kind of UI that says "attribute this payment to an outbound payment" (hopefully not no one is gonna build that)? Or is there some way of automatically detecting that (maybe based on validated 353?), in which case we should automate that somehow, presumably?
| // FIXME: this can be simply a function call? | ||
| impl UnverifiedContactAddress { | ||
| /// Creates a new [`UnverifiedContactAddress`]. | ||
| pub fn new(address: ContactAddress, expected_offer_signing_key: PublicKey) -> Self { |
There was a problem hiding this comment.
There's a lot of stuff here that could really use examples discussing how to use it in an overall flow but also actual sample code. From reading this API its really not clear to me when/how I'd use this - the type doesn't exist outside of a freestanding struct.
There was a problem hiding this comment.
I think UnverifiedContactAddress is overkill now that I am rereading the code and probably I should remove it, I did this implementation when I was doing some kind of overdesign here! sorry about that!
|
🔔 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. |
vincenzopalazzo
force-pushed
the
macros/blip02-prep-v2
branch
from
be4f248 to
755fed4
Compare
vincenzopalazzo
force-pushed
the
macros/blip02-prep-v2
branch
from
8a24949 to
f562577
Compare
vincenzopalazzo
force-pushed
the
macros/blip02-prep-v2
branch
6 times, most recently
from
4bbab5d to
a650c93
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4210 +/- ##
==========================================
- Coverage 86.40% 86.38% -0.03%
==========================================
Files 158 159 +1
Lines 109293 109522 +229
Branches 109293 109522 +229
==========================================
+ Hits 94439 94607 +168
- Misses 12309 12361 +52
- Partials 2545 2554 +9
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:
|
vincenzopalazzo
force-pushed
the
macros/blip02-prep-v2
branch
from
a650c93 to
40bd9e4
Compare
Implements bLIP 42 contact secret derivation for mutual authentication in Lightning Network payments. - Add ContactSecret struct for TLV serialization with Readable/Writeable - Add ContactSecrets for managing primary and additional remote secrets - Add compute_contact_secret() for deterministic secret derivation - Support offers with issuer_signing_pubkey and blinded paths Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
…et, invreq_payer_offer Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Implements BLIP-42 contact management for the sender side: - Add contact_secret and payer_offer fields to InvoiceRequestContents - Add builder methods: contact_secrets(), payer_offer() - Add accessor methods: contact_secret(), payer_offer() - Add OptionalOfferPaymentParams fields for contact_secrects and payer_offer - Update ChannelManager::pay_for_offer to pass contact information - Add create_compact_offer_builder to OffersMessageFlow for small payer offers - Update tests to include new InvoiceRequestFields Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
vincenzopalazzo
force-pushed
the
macros/blip02-prep-v2
branch
from
40bd9e4 to
1180171
Compare
| pub fn compute_contact_secret( | ||
| our_private_key: &SecretKey, their_offer: &Offer, | ||
| ) -> Result<ContactSecrets, Bolt12SemanticError> { | ||
| let offer_node_id = if let Some(issuer) = their_offer.issuer_signing_pubkey() { | ||
| // If the offer has an issuer signing key, use it | ||
| issuer | ||
| } else { | ||
| // Otherwise, use the last node in the first blinded path (if any) | ||
| their_offer | ||
| .paths() | ||
| .iter() | ||
| .filter_map(|path| path.blinded_hops().last()) | ||
| .map(|hop| hop.blinded_node_id) | ||
| .next() | ||
| .ok_or(Bolt12SemanticError::MissingSigningPubkey)? | ||
| }; | ||
| // Compute ECDH shared secret (multiply their public key by our private key) | ||
| let scalar: Scalar = our_private_key.clone().into(); | ||
| let secp = Secp256k1::verification_only(); | ||
| let ecdh = offer_node_id.mul_tweak(&secp, &scalar).expect("Multiply"); |
There was a problem hiding this comment.
Bug: ECDH computation is not symmetric when using blinded paths
The function extracts blinded_node_id from the last hop of the other party's blinded path (line 167). However, blinded_node_id is NOT the peer's actual public key — it's actual_pubkey * blinding_factor, where the blinding factor is derived from a random session key during blinded path construction.
This means the ECDH won't produce matching results for two parties who each have blinded-path-only offers:
- Alice computes:
alice_privkey * (bob_pubkey * bob_blinding) = alice_privkey * bob_privkey * bob_blinding * G - Bob computes:
bob_privkey * (alice_pubkey * alice_blinding) = alice_privkey * bob_privkey * alice_blinding * G
These are equal only if bob_blinding == alice_blinding, which is not the case since each party uses a random session key.
The same issue exists when issuer_signing_pubkey() is present: LDK's OfferBuilder::deriving_signing_pubkey produces a derived key (from expanded_key + nonce), not the node's actual public key. So our_private_key (the "node identity" key per the docstring) won't ECDH-match with the derived signing pubkey either.
The test vectors pass because the hard-coded private keys happen to correspond to the public keys in the pre-built offer strings (i.e., they are the blinded/derived private keys, not raw node keys). But real callers passing their node identity key would get non-matching secrets.
To fix this, either:
- The function should accept the private key corresponding to the caller's offer's exposed public key (and the docstring should reflect this), with an API to extract that key, or
- The function should take both offers and compute the secret differently (e.g., using a hash of both offer bytes as input, rather than ECDH).
| }; | ||
| // Compute ECDH shared secret (multiply their public key by our private key) | ||
| let scalar: Scalar = our_private_key.clone().into(); | ||
| let secp = Secp256k1::verification_only(); |
There was a problem hiding this comment.
Performance: Secp256k1::verification_only() is expensive to construct
Secp256k1::verification_only() allocates and precomputes EC tables every time it's called. This function should accept a &Secp256k1<T> parameter instead of creating a new context per invocation, consistent with how the rest of LDK passes secp contexts around.
| let payer_offer = | ||
| payer_offer_bytes.and_then(|bytes| crate::offers::offer::Offer::try_from(bytes).ok()); |
There was a problem hiding this comment.
Silent data loss on deserialization round-trip
If payer_offer_bytes contains valid TLV data that fails Offer::try_from() parsing (e.g., due to unknown required fields, or a newer offer format), the payer_offer is silently set to None. On re-serialization, this data is lost.
This means a write -> read -> write cycle can drop the payer_offer field. If this is persisted (e.g., in PaymentContext::Bolt12Offer), stored data can silently degrade.
Consider either propagating the parse error (returning DecodeError), or storing the raw bytes alongside the parsed Offer so that re-serialization preserves unknown data.
| pub(super) fn payer_offer(&self) -> Option<crate::offers::offer::Offer> { | ||
| self.inner | ||
| .invreq_payer_offer | ||
| .as_ref() | ||
| .and_then(|bytes| crate::offers::offer::Offer::try_from(bytes.clone()).ok()) |
There was a problem hiding this comment.
Re-parses offer bytes on every call
payer_offer() calls Offer::try_from(bytes.clone()) on every invocation, allocating and parsing each time. Consider caching the parsed Offer in the struct, or lazily parsing once and storing the result.
| /// Sets the payer's offer for BLIP-42 contact management. | ||
| /// | ||
| /// This will include the serialized offer bytes in the invoice request, | ||
| /// allowing the recipient to identify which offer the payer is responding to. |
There was a problem hiding this comment.
Misleading docstring: This says "allowing the recipient to identify which offer the payer is responding to", but the payer_offer is the payer's own offer that the recipient can use to contact the payer back. It is not related to identifying which offer the payer is responding to (that's already implicit in the invoice request itself). The OptionalOfferPaymentParams::payer_offer docstring in channelmanager.rs gets this right.
Review SummaryNew Issues FoundInline comments posted:
Previously Reported Issues (Still Unaddressed)These were flagged in the prior review and remain applicable:
Resolved from Prior Review
|
Replace `Option<[u8; 32]>` with `Option<ContactSecret>` across the invoice_request internal storage, the `InvoiceRequest::contact_secret()` accessor, and the `InvoiceRequestFields` round-trip type. The wire format is unchanged (`ContactSecret` writes/reads the same 32 raw bytes via its existing `Writeable`/`Readable` impls), so this is a pure type-system refactor. Add `INVREQ_CONTACT_SECRET_TYPE` and `INVREQ_PAYER_OFFER_TYPE` `pub(super)` constants in `offers::contacts`, mirroring the pattern of `INVOICE_REQUEST_PAYER_ID_TYPE` and the other TLV-type constants in `offers/`. Replace the numeric literals in `ExperimentalInvoiceRequestTlvStream` with these constants so the BLIP 42 TLV numbers are defined in exactly one place. Also document BLIP 42's anti-redirection rule on the public `InvoiceRequestFields::contact_secret` field, since enforcement is the application's responsibility — LDK does not own a contacts store. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`InvoiceRequestFields` is round-tripped through the blinded payment path's `path_id` (via `Bolt12OfferContext`), so older LDK nodes can end up parsing a serialization produced by a newer LDK. Per BOLT 1 "odd, it's OK; even, you die", optional/forward-compatible fields must use odd TLV types so unknown-to-an-older-reader fields are skipped rather than rejected with `DecodeError::UnknownRequiredField`. The original PoC used even types 6 and 8 for `contact_secret` and `payer_offer`, which would have broken forward compat between LDK versions once shipped. Move them to odd 7 and 9. Reserve 11 and 13 for `invreq_payer_bip_353_name` and `invreq_payer_bip_353_signature` in a follow-up commit. The wire format change is safe in-flight: PR lightningdevkit#4210 has not been released, so no deployed consumer is reading types 6 or 8 today. Flagged by `ldk-claude-review-bot` on the previous push. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| /// Checks if the given secret matches either the primary secret or any additional remote secret. | ||
| pub fn matches(&self, secret: &ContactSecret) -> bool { | ||
| &self.primary_secret == secret || self.additional_remote_secrets.contains(secret) | ||
| } |
There was a problem hiding this comment.
Security (Low): Non-constant-time comparison for cryptographic secrets
ContactSecrets::matches() uses the derived PartialEq for ContactSecret, which performs a byte-by-byte comparison that short-circuits on the first mismatch. For a value used to authenticate payment senders, a constant-time comparison (e.g., using a constant-time equality function) would be more appropriate.
Practical exploitability is very low (the timing difference is nanoseconds, masked by network latency and payment processing overhead), but constant-time comparison is a best practice for any secret material used in authentication.
| /// Checks if the given secret matches either the primary secret or any additional remote secret. | |
| pub fn matches(&self, secret: &ContactSecret) -> bool { | |
| &self.primary_secret == secret || self.additional_remote_secrets.contains(secret) | |
| } | |
| /// Checks if the given secret matches either the primary secret or any additional remote secret. | |
| pub fn matches(&self, secret: &ContactSecret) -> bool { | |
| // Use constant-time comparison to prevent timing side-channels on secret material. | |
| let primary_match = bitcoin::hashes::cmp::fixed_time_eq( | |
| self.primary_secret.as_bytes(), | |
| secret.as_bytes(), | |
| ); | |
| let additional_match = self.additional_remote_secrets.iter().any(|s| { | |
| bitcoin::hashes::cmp::fixed_time_eq(s.as_bytes(), secret.as_bytes()) | |
| }); | |
| primary_match || additional_match | |
| } |
Note: check that bitcoin::hashes::cmp::fixed_time_eq or an equivalent constant-time function is available. If not, a simple constant-time comparison loop (xor all bytes, check result) would suffice.
| // Compute ECDH shared secret (multiply their public key by our private key) | ||
| let scalar: Scalar = our_private_key.clone().into(); | ||
| let secp = Secp256k1::verification_only(); | ||
| let ecdh = offer_node_id.mul_tweak(&secp, &scalar).expect("Multiply"); |
There was a problem hiding this comment.
Bug: mul_tweak is NOT the standard ECDH operation
PublicKey::mul_tweak computes point * scalar but its contract is as a "tweak" — it was designed for key derivation, not ECDH. While mathematically it performs EC point multiplication (and the result here IS the ECDH shared point), the standard way to do ECDH in secp256k1 is via secp256k1::ecdh::shared_secret_point or SharedSecret::new. Using the proper ECDH API:
- Makes the intent clearer
- Uses a purpose-built code path that's been audited for side-channel resistance
- Avoids relying on internal behavior of the tweak API
Consider:
use bitcoin::secp256k1::ecdh::SharedSecret;
let shared = SharedSecret::new(&offer_node_id, our_private_key);This also eliminates the need for the Secp256k1::verification_only() context entirely (the SharedSecret::new API doesn't require one).
| /// let params = OptionalOfferPaymentParams { | ||
| /// payer_offer: Some(payer_offer), | ||
| /// ..Default::default() | ||
| /// }; | ||
| /// ``` | ||
| /// | ||
| /// [`OffersMessageFlow::create_compact_offer_builder`]: crate::offers::flow::OffersMessageFlow::create_compact_offer_builder | ||
| pub payer_offer: Option<Offer>, |
There was a problem hiding this comment.
Design concern: No size validation on payer_offer
The Offer passed here gets serialized into both:
- The invoice request's experimental TLV (the onion message carrying the invreq)
InvoiceRequestFields→Bolt12OfferContext→ the final hop's encrypted payload in every blinded payment path of the resulting invoice
For (2), the HTLC onion packet has a ~1300-byte total payload budget across all hops. Adding 70-200+ bytes for a compact offer (plus TLV overhead) to the final hop's encrypted data reduces the available space for intermediate hops, limiting route length.
Consider adding a size check (e.g., offer.as_ref().len() <= MAX_PAYER_OFFER_SIZE) and returning an error if the offer is too large, or at least documenting the size constraint so callers know they must use a compact offer.
5 participants
With this PR I am proposing the first implementation for the BLIP 42 implementation that allow to send a "contact" with a contact secret for verification when making an invoice request during a
pay_for_offer.The current implementation is injecting the BLIP 42 information by default and there is no way to op out to this feature (and IMHO would be could to have a way to disable it).
In addition this RFC it is just a way to collect the first comments on API and design choice that I made and to collect feedback on how I manage stuff on the ldk internal stuff, probably there is some more simple way of doing the same thing.
However, this PR has already two problem:
invreq_payer_bip_353_signature