RFC for PoC of bolt12 contacts formally known as a BLIP 42 #4210
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.
lightning/src/ln/channelmanager.rs
Outdated
| let builder = builder.contact_secrets(contacts.clone()); | ||
| // Create a minimal offer for BLIP-42 contact exchange (just node_id, no description/paths) | ||
| // TODO: Create a better minimal offer with a single blinded path hop for privacy, | ||
| // while keeping the size small enough to fit in the onion packet. |
There was a problem hiding this comment.
Luckily the onion packet is 64K so we're not too worried about the size. Indeed, as you note in your questions, we should probably try to figure out if the dev probably wants us to use an async offer (based on async availability/config) and use one of those if we can.
There was a problem hiding this comment.
I will add a complete answer later but my message was refering to the current CI error https://github.com/lightningdevkit/rust-lightning/actions/runs/19137908502/job/54694666815?pr=4210#step:9:6040 that probably an offer with two blinded path is too big?
node 0 ERROR [lightning::ln::outbound_payment:1486] Can't construct an onion packet without exceeding 1300-byte onion hop_data length for payment with id 0101010101010101010101010101010101010101010101010101010101010101 and hash 74b87079b704f2162403361478f1bca041556cd1103e9882b394d5e4bb379c41
node 0 TRACE [lightning::ln::channelmanager:15385] Failed paying invoice: OnionPacketSizeExceeded
node 0 TRACE [lightning::onion_message::messenger:1538] Constructing onion message when responding with Invoice Error to an onion message: InvoiceError { erroneous_field: None, message: UntrustedString("OnionPacketSizeExceeded") }
node 0 TRACE [lightning::onion_message::messenger:1575] Buffered onion message when responding with Invoice Error to an onion message
thread 'ln::offers_tests::rejects_keysend_to_non_static_invoice_path' (4319) panicked at lightning/src/ln/functional_test_utils.rs:1283:9:
assertion `left == right` failed: expected 1 monitors to be added, not 0
left: 0
right: 1
async offer (based on async availability/config)
Oh good point will look into it next!
There was a problem hiding this comment.
Yea, we're space-constrained only in the async case. Annoying.
There was a problem hiding this comment.
Ok, I followed the Phoenix offer construction that is creating an offer with a single blinded path or any, and I added an easy way to create it from the OfferFlow. I think the API (it is used inside the tests) is fairly easy now to use from the user's point of view. What do you think?
I still need to look inside the async offer, that it is next on my todo list
lightning/src/offers/contacts.rs
Outdated
| /// | ||
| /// 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<(), ()>.
lightning/src/offers/contacts.rs
Outdated
| #[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?
lightning/src/offers/contacts.rs
Outdated
| // 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
5 times, most recently
from
1acf88d to
4bbab5d
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>
vincenzopalazzo
force-pushed
the
macros/blip02-prep-v2
branch
from
4bbab5d to
a650c93
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4210 +/- ##
==========================================
- Coverage 89.38% 89.34% -0.05%
==========================================
Files 180 181 +1
Lines 139834 140079 +245
Branches 139834 140079 +245
==========================================
+ Hits 124985 125147 +162
- Misses 12262 12335 +73
- Partials 2587 2597 +10
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:
|
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
a650c93 to
40bd9e4
Compare
3 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