Multi rfq receive (`AddInvoice` multiple hop hints) #1457

GeorgeTsagk · 2025-03-27T13:16:43Z

Description

This PR introduces the ability for tapd nodes to create invoices which involve multiple peer quotes. When the peerPubKey is left unspecified in the AddInvoice RPC we no longer return an error, but instead acquire quotes with all peers that have a valid asset channel with us.

Within this PR we also extract some rfq/liquidity related functions to the rfq package to keep rpcserver.go more clean.

Closes #1359

Based on #1423 (using that as base branch for now to avoid bloated diff)

tapchannel/aux_traffic_shaper.go

taprpc/tapchannelrpc/tapchannel.proto

rpcserver.go

rfq/manager.go

rpcserver.go

Roasbeef · 2025-03-31T23:32:42Z

rpcserver.go

+	// invoice amount. Since peers have varying prices for the assets, we
+	// pick the most expensive rate in order to allow for any combination of
+	// MPP shards through our set of chosen peers.
+	expensiveRate := acquiredQuotes[0].rate


Why should we pick the most expensive quote? This is effectively the same as giving the sender an option for the most expensive route in terms of fees.

From the invoice creator's PoV we only really see assets, but at the end of the day we have to direct the sender into sending some msat value.

If we pick any other than the most expensive quote (of the "rfq handpicks set") we may run into the case where the sender believes they pushed the full amt to satisfy the invoice, but because some shards went through the "expensive" link, the receiver actually sees less on their end, so they never settle the invoice and we end up in a stalemate.

Roasbeef · 2025-03-31T23:34:37Z

rpcserver.go


 	// Convert the asset amount into a fixed-point.
 	assetAmount := rfqmath.NewBigIntFixedPoint(req.AssetAmount, 0)

 	// Calculate the invoice amount in msat.
-	valMsat := rfqmath.UnitsToMilliSatoshi(assetAmount, *askAssetRate)
+	valMsat := rfqmath.UnitsToMilliSatoshi(assetAmount, *expensiveRate)


Ah ok, I think this is making a bit more sense now: ultimately we need to put some value into the invoice. I think we'll want to do another level of filtering here to ensure that we don't force the user to overpay. Or will the acceptance deviation check handle that?

Yeah since the RFQ quotes have been processed by our RFQ manager, we know that they respect the user configured tolerance.

We never do violate the configured tolerance, but we do lean towards higher values here. Which translates into more expensive invoices that the sender has to pay.

rpcserver.go

coveralls · 2025-04-25T16:11:07Z

Pull Request Test Coverage Report for Build 15121932291

Details

41 of 304 (13.49%) changed or added relevant lines in 4 files are covered.
16 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.2%) to 36.969%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapcfg/server.go	0	1	0.0%
rfq/manager.go	38	153	24.84%
rpcserver.go	0	147	0.0%

Files with Coverage Reduction	New Missed Lines	%
asset/group_key.go	2	57.89%
rfq/manager.go	4	16.06%
rpcserver.go	4	0.0%
tapgarden/caretaker.go	6	68.68%

Totals
Change from base Build 15055985108:	0.2%
Covered Lines:	26668
Relevant Lines:	72136

💛 - Coveralls

Roasbeef

Changes are looking really good!

Only blocking factor now IMO is the addition of unit test coverage for the new helper functions that were moved from the rpcserver to the rfq package (which are also expanded in this PR).

rfq/manager.go

rpcserver.go

Roasbeef · 2025-04-29T00:24:11Z

rpcserver.go

@@ -7785,6 +7790,7 @@ func (r *rpcServer) AddInvoice(ctx context.Context,
 		return nil, fmt.Errorf("invoice request must be specified")
 	}
 	iReq := req.InvoiceRequest
+	existingQuotes := iReq.RouteHints != nil


Hmm, perhaps we should actually make validation stricter here and disallow adding extra route hints? Otherwise a user can add hop hints that affect our reachability, or encodes the wrong details, etc.

I considered this to be the case where the user probably knows what they're doing. This carve-out was made for cases where quotes are negotiated externally, and the user provides them manually via the route hints.

IIUC what you're saying is that the user could actually get confused with the field and provide normal route hints for btc channels? In that case they're simply bypassing all asset related fields, as the invoice amt and route hints will persist as is, making this effectively a btc invoice.

If we want to block the above case, which doesn't sound unreasonable, I'll export/reuse the IsAssetInvoice helper we added to the AuxInvoiceManager a while ago. If the route hints don't correspond to valid & accepted quotes, we'll return an error on the RPC.

rpcserver.go

Roasbeef · 2025-04-29T00:31:29Z

rpcserver.go

+		acquiredQuotes = acquiredQuotes[:maxRfqHopHints]
+	}
+
+	// TODO(george): We want to cancel back quotes that didn't make it into


Let's spin this into an issue post merge so it doesn't get lost.

Reminder here.

Roasbeef · 2025-04-29T00:35:04Z

rpcserver.go

+			rate:  rate,
+			// Since the channels are sorted, we now the value with
+			// the greatest remote balance is at index 0.
+			channel: channels[0],


Something to keep in mind here is that when receiving shards our peer does non-strict forwarding on the LND level. The channel we pick here does not bind the actual link that will be used for the HTLC.

With the recent set of liquidity fixes, IIUC we're much stricter here now (being too lax was itself a bug).

Looks like actually just the channel ID and pubkey are explicitly needed, and we fetch the policy using one of lnd's RPC calls.

lightninglabs-deploy · 2025-05-06T01:05:29Z

@ffranr: review reminder
@GeorgeTsagk, remember to re-request review from reviewers when ready

Roasbeef

LGTM 🦮

Roasbeef · 2025-05-13T23:50:29Z

rpcserver.go

+		acquiredQuotes = acquiredQuotes[:maxRfqHopHints]
+	}
+
+	// TODO(george): We want to cancel back quotes that didn't make it into


Reminder here.

Roasbeef · 2025-05-13T23:51:45Z

There's an itest failure. Not sure if it's a flake though, can't view the logs from the UI as they're truncated.

ffranr

I've added naming suggestions.

rfq/manager.go

ffranr · 2025-05-16T13:51:36Z

rfq/manager_test.go

+	proof3 = proof.Proof{
+		Asset: asset3,
+	}
+	testGroupKey = pubKeyFromUint64(2121)


Perhaps we should just set this to a random group key? I think we have functions for generating random group keys. And then we don't need pubKeyFromUint64. Although, I do like pubKeyFromUint64, random key is better practice for more coverage IMO.

can't really find the random generator you're referring to

Generally we do lean towards having simple "mock" values for most of our fields in unit tests, where everything is deterministic. Could consider a prop test but not sure if this code is worth that coverage?

Instead of using the wallet anchor channel lister we now use the lndclient one, as it offers a few more arguments that we want to use in the rfq manager. This is done as a preparatory step as we're going to push more responsibilities around channel picking to the rfq package.

We now move some of the rfq channel helpers that previously lived in rpcserver to the rfq package. We also use the new channel lister we introduced previously.

GeorgeTsagk · 2025-05-19T10:28:08Z

Since this is a backwards-compatible change, I thought it might be worth noting that current LiT tests fail simply because of a returned error change
https://github.com/lightninglabs/taproot-assets/actions/runs/15110194077/job/42467761965?pr=1457#step:10:11376

We now extend our rfq channel related helpers in order to return a map of results instead of singular results. Since multi-rfq requires handling quotes with multiple peers, we don't care about handpicking a single best result but instead return all valid candidates.

Since we introduced the RfqChannel helper to the rfq package we take this opportunity to simplify the codebase of the addScidAlias helper.

As another preparatory step before introducing multiple quotes to the invoices, we add a simple parser that converts an rfq quote along with some other crucial information to a valid bolt11 hop hint.

We extract away another piece of logic that will be re-used in the new reformed AddInvoice endpoint. Since we will be querying multiple peers for quotes, we need to abstract this away into a function.

We now change the quote related flow of AddInvoice to instead query all of our peers for valid quotes. We keep the best subset of accepted quotes up to the max allowed number of hop hints and place it into the bolt11 invoice.

We rename some of the existing structures or helper methods to be shorter and more directly indicate their purpose.

levmi assigned GeorgeTsagk Mar 27, 2025

levmi added this to Taproot-Assets Project Board Mar 27, 2025

github-project-automation bot moved this to 🆕 New in Taproot-Assets Project Board Mar 27, 2025

levmi moved this from 🆕 New to 🏗 In progress in Taproot-Assets Project Board Mar 27, 2025

GeorgeTsagk force-pushed the taprpc-groupkey-support branch from 99e85a7 to e2e8e47 Compare March 31, 2025 19:19

Roasbeef requested changes Mar 31, 2025

View reviewed changes

github-project-automation bot moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Mar 31, 2025

GeorgeTsagk force-pushed the taprpc-groupkey-support branch from e2e8e47 to e9e1de8 Compare April 4, 2025 13:13

Base automatically changed from taprpc-groupkey-support to main April 4, 2025 14:52

GeorgeTsagk force-pushed the multi-rfq-receive branch from c5d02b1 to 204964f Compare April 25, 2025 15:50

GeorgeTsagk mentioned this pull request Apr 25, 2025

Multi rfq receive itest lightninglabs/lightning-terminal#1050

Merged

GeorgeTsagk requested review from ffranr and Roasbeef April 25, 2025 15:58

GeorgeTsagk force-pushed the multi-rfq-receive branch from 204964f to 2a288fb Compare April 25, 2025 17:17

Roasbeef reviewed Apr 29, 2025

View reviewed changes

GeorgeTsagk force-pushed the multi-rfq-receive branch from 2a288fb to 77dd73f Compare May 9, 2025 15:06

GeorgeTsagk requested a review from Roasbeef May 10, 2025 15:14

Roasbeef approved these changes May 13, 2025

View reviewed changes

GeorgeTsagk force-pushed the multi-rfq-receive branch 3 times, most recently from 48f9b0d to 58f862e Compare May 15, 2025 15:40

ffranr reviewed May 16, 2025

View reviewed changes

GeorgeTsagk added 2 commits May 19, 2025 11:56

multi: move rfq channel helpers to rfq manager

f441b9e

We now move some of the rfq channel helpers that previously lived in rpcserver to the rfq package. We also use the new channel lister we introduced previously.

GeorgeTsagk force-pushed the multi-rfq-receive branch from 58f862e to 33b78d9 Compare May 19, 2025 10:07

GeorgeTsagk requested a review from ffranr May 19, 2025 10:42

GeorgeTsagk added 9 commits May 19, 2025 21:03

rfq: add ComputeChannelAssetBalance tests

306e297

rfq: addScidAlias uses RfqChannel helper

90cd457

Since we introduced the RfqChannel helper to the rfq package we take this opportunity to simplify the codebase of the addScidAlias helper.

rfq: add RfqToHopHint helper in manager

8d8cc02

As another preparatory step before introducing multiple quotes to the invoices, we add a simple parser that converts an rfq quote along with some other crucial information to a valid bolt11 hop hint.

tapchannel: export IsAssetInvoice helper

0b1c1e5

rpcserver: add acquireBuyOrder helper

b7ed69a

We extract away another piece of logic that will be re-used in the new reformed AddInvoice endpoint. Since we will be querying multiple peers for quotes, we need to abstract this away into a function.

rpcserver: AddInvoice supports multi-rfq

945fdd3

We now change the quote related flow of AddInvoice to instead query all of our peers for valid quotes. We keep the best subset of accepted quotes up to the max allowed number of hop hints and place it into the bolt11 invoice.

rfq+rpcserver: rename helper methods/structs for readability

42c07a5

We rename some of the existing structures or helper methods to be shorter and more directly indicate their purpose.

taprpc: update AddInvoice documentation

8abf803

GeorgeTsagk force-pushed the multi-rfq-receive branch from 33b78d9 to 8abf803 Compare May 19, 2025 19:52

ffranr approved these changes May 20, 2025

View reviewed changes

ffranr added this pull request to the merge queue May 20, 2025

Merged via the queue into main with commit f6baa2c May 20, 2025
17 of 18 checks passed

github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board May 20, 2025

This was referenced May 27, 2025

[feature]: AddInvoice: allow specifying an rfq_id #1442

Open

[bug]: AddInvoiceResponse only returns a single accepted_buy_quote #1565

Open

[feature]: AddInvoiceResponse: also show rejected buy quotes #1569

Open

Multi rfq receive (AddInvoice multiple hop hints) #1457

Multi rfq receive (AddInvoice multiple hop hints) #1457

Conversation

GeorgeTsagk commented Mar 27, 2025

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coveralls commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15121932291

Details

💛 - Coveralls

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented May 6, 2025

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented May 13, 2025

Uh oh!

ffranr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeorgeTsagk commented May 19, 2025

Uh oh!

Uh oh!

Uh oh!

Multi rfq receive (`AddInvoice` multiple hop hints) #1457

Multi rfq receive (`AddInvoice` multiple hop hints) #1457

coveralls commented Apr 25, 2025 •

edited

Loading