Offers: send invoice request #81

orbitalturtle · 2023-11-17T21:00:04Z

This PR adds the step of the offers flow where we send the invoice request to the offer creator.

This relies on the addition of this offers RPC call to our ldk-sample fork, if anyone would like to take a look: lndk-org/ldk-sample#6

I left a couple of high level questions inline as well.

For our integration tests to work properly we need to use a custom version of lnd's RPC that allows signing the tagged hash (BIP 340) of a message. This was introduced in LND at commit dacb86f.

orbitalturtle · 2023-11-17T21:26:43Z

src/lndk_offers.rs

+        .peer_connected(
+            &intermediate_nodes[0],
+            &Init {
+                features: init_features,


Two high level questions here.

One: So it seems that send_onion_message requires that we supply a full path to the offer creator, including a path of intermediate nodes to the blinded path as well as the blinded path. Here we're connecting directly to the introduction node so we can just send the onion message directly. But idk, maybe if the introduction node isn't accepting direct peer connects (?) or something, we could find a route to the introduction node?

Two: the peer we're connecting to here is the introduction node provided in the offer, so we can assume it provides onion messaging support correct? Or should we double check that it advertises support for good measure?

But idk, maybe if the introduction node isn't accepting direct peer connects (?) or something, we could find a route to the introduction node?

Right now everyone is accepting direct connects (afaik), but we definitely should do our own pathfinding to the introduction node to improve sender privacy. We can leave this for a followup to keep it simple for now. I think ldk may also fill out some of this functionality, so we should get an idea of what they're going to do / what we need to do ourselves.

Or should we double check that it advertises support for good measure?

The only case where the introduction node doesn't support onion messaging + route blinding is if the recipient created an invalid path. Makes sense to me to have a validation step where we sanity check this, because there's no point proceeding if they couldn't make a good path for us.

Cool, leaving that pathfinding step as a follow up sounds good - I'll open an issue as a reminder.

Makes sense to me to have a validation step where we sanity check this, because there's no point proceeding if they couldn't make a good path for us.

Good point, added a check for that.

Ok so I made some changes. It might seem a little weird that we use another onion messenger instance here... it's kind of unnecessary, but one key thing it provides is turning our InvoiceRequest into a proper onion message that we can send. It looks like LDK will export the create_onion_message fn in the next release so we can use it directly... So when that's out, we could get rid of the onion messenger instance here and just directly use lnd's SendCustomMessage call instead, but using the onion messenger here is the only workaround I could think of for now

orbitalturtle · 2023-11-20T00:26:42Z

Oi, now that I'm working on waiting for the invoice to be returned... I think I coded things a bit backwards here. Feel free to hold off on review while I rethink and rework this a bit. :)

carlaKC

Started a review and forgot to submit - will wait on changes that work with offer request otherwise!

Makefile

src/lib.rs

carlaKC · 2023-11-17T21:33:31Z

src/lndk_offers.rs

+        .peer_connected(
+            &intermediate_nodes[0],
+            &Init {
+                features: init_features,


But idk, maybe if the introduction node isn't accepting direct peer connects (?) or something, we could find a route to the introduction node?

Right now everyone is accepting direct connects (afaik), but we definitely should do our own pathfinding to the introduction node to improve sender privacy. We can leave this for a followup to keep it simple for now. I think ldk may also fill out some of this functionality, so we should get an idea of what they're going to do / what we need to do ourselves.

Or should we double check that it advertises support for good measure?

The only case where the introduction node doesn't support onion messaging + route blinding is if the recipient created an invalid path. Makes sense to me to have a validation step where we sanity check this, because there's no point proceeding if they couldn't make a good path for us.

For the integration tests to work we need to add the walletrpc subservice to the lnd make process so we can use the DeriveKey RPC call.

carlaKC

Looking good!

Major comments:

Provide a way for end user to specify amount for invoice request
Check whether we're connected to introduction node, connect if not

src/tests/test_utils.rs

carlaKC · 2023-12-11T14:39:23Z

src/lndk_offers.rs

+    }
+
+    let invoice_request =
+        create_invoice_request(client.clone(), offer, vec![], Network::Regtest, 20000)


Amount is hardcoded in here?

Shouldn't we (in create_invoice_request):

Allow the caller to specify Some(u64) to set an amount to pay

If the offer has an amount, ensure that Some(user amount) is > offer amount

If the offer does not have an amount (or is zero amt), ensure that we have Some(user amount)

carlaKC · 2023-12-11T14:44:58Z

src/lndk_offers.rs

+    onion_messenger
+        .peer_connected(


Here we're notifying the onion_messenger that we've connected to the introduction node but I don't see where we actually tell LND to make the P2P connection?

Ideally, we don't have to duplicate this logic here - we tell LND to connect to the peer and then wait for the onion_messenger's existing logic to process that connection?

That is going to introduce some annoying wait/backoff loop, but I think that's okay if we're going to introduce #82 (we can delete the connection logic once we're properly pathfinding).

Sounds good, so pretty much, connect to the introduction node, and have a consume_messenger_events loop running here that will be just be looking for the PeerConnected event and respond as needed?

have a consume_messenger_events loop running here

Not sure I understand why we'd have a second loop running here? Shouldn't we have the onion messenger running already if we're paying an offer to handling all of the sending/receiving of messages?

The onion_messenger instance we're using here in this cli/offers code is different from the one running when lndk is started up. So I don't think it'll receive those peer connect notifications? So another consume_messenger_events is what I thought you meant as far as not duplicating logic.

However, I think the easiest thing would be to just, connect to the peer here as well as give the onion messenger this information. I don't think some duplicated code here would be too terrible, especially if we're going to get rid of it later anyway.

And on that note, not sure if you saw my comment from above. We might not need the onion messenger instance here at all in the next release of LDK, IIUC #81 (comment)

The onion_messenger instance we're using here in this cli/offers code is different from the one running when lndk is started up.

Right, of course.

On a higher level, I think it would make sense to me that we pull out all of our onion messaging logic into one struct (ie, run_onion_messenger becomes its run function) like you've started to do with SendOnion and then surface send_invoice_request so that we don't have to duplicate some of the logic we already have (send_onion / next_onion_message_for_peer / relay_outgoing_msg_event). I hope that makes some sense, I'll take a proper look at the code and see if it works out the way I think it does.

Opened this up to add some context: #85

carlaKC · 2023-12-11T14:46:48Z

tests/integration_tests.rs

+
+    // Here we'll produce a little network path:
+    //
+    // ldk1 <-> ldk2 <-> lnd


Would be good to have a test where LND isn't connected to the introduction point already.

tests: update lnd submodule to tagged hash change

3984f7e

For our integration tests to work properly we need to use a custom version of lnd's RPC that allows signing the tagged hash (BIP 340) of a message. This was introduced in LND at commit dacb86f.

orbitalturtle commented Nov 17, 2023

View reviewed changes

orbitalturtle requested a review from carlaKC November 17, 2023 21:26

carlaKC reviewed Nov 20, 2023

View reviewed changes

orbitalturtle force-pushed the send-invoice-request branch from ef8fbe3 to d2e4f9a Compare November 22, 2023 04:06

orbitalturtle added 6 commits November 21, 2023 22:15

tests: add walletrpc subserver to lnd Makefile/README

f490dee

For the integration tests to work we need to add the walletrpc subservice to the lnd make process so we can use the DeriveKey RPC call.

Pin rust-lightning to a custom version

40c6331

Ignore log init error

ec247fd

Add tokio clock default to satisfy clippy

0bac19c

tests: specify log level in ldk nodes

2c6efcc

tests: move and export onion_message fn for itests

4a79f4c

orbitalturtle force-pushed the send-invoice-request branch from d2e4f9a to dbefae9 Compare November 22, 2023 04:24

orbitalturtle added 3 commits November 22, 2023 00:16

Export onion messaging tools for offers and itests

484f462

offers: send invoice request

b656715

README: Fix bakemacaroon typo

7bd2a28

orbitalturtle force-pushed the send-invoice-request branch from dbefae9 to 7bd2a28 Compare November 22, 2023 06:23

orbitalturtle requested a review from carlaKC November 22, 2023 06:23

orbitalturtle mentioned this pull request Dec 8, 2023

offers: wait for offers invoice #84

Closed

carlaKC reviewed Dec 11, 2023

View reviewed changes

carlaKC mentioned this pull request Dec 20, 2023

Potential Offer Handling #85

Closed

orbitalturtle closed this Jan 2, 2024

orbitalturtle mentioned this pull request Jan 2, 2024

Offers: send invoice request #87

Merged

orbitalturtle deleted the send-invoice-request branch May 30, 2024 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offers: send invoice request #81

Offers: send invoice request #81

orbitalturtle commented Nov 17, 2023

orbitalturtle Nov 17, 2023

carlaKC Nov 17, 2023

orbitalturtle Nov 22, 2023

orbitalturtle commented Nov 20, 2023

carlaKC left a comment

carlaKC Nov 17, 2023

carlaKC left a comment

carlaKC Dec 11, 2023

carlaKC Dec 11, 2023

orbitalturtle Dec 13, 2023 •

edited

Loading

carlaKC Dec 13, 2023 •

edited

Loading

orbitalturtle Dec 13, 2023 •

edited

Loading

carlaKC Dec 20, 2023

carlaKC Dec 20, 2023

carlaKC Dec 11, 2023

Offers: send invoice request #81

Offers: send invoice request #81

Conversation

orbitalturtle commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbitalturtle commented Nov 20, 2023

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbitalturtle Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

carlaKC Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

orbitalturtle Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orbitalturtle Dec 13, 2023 •

edited

Loading

carlaKC Dec 13, 2023 •

edited

Loading

orbitalturtle Dec 13, 2023 •

edited

Loading