-
Notifications
You must be signed in to change notification settings - Fork 132
Add Multi-RFQ Send #1613
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?
Add Multi-RFQ Send #1613
Conversation
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.
Looks pretty good! Have a couple of questions and suggestions, nothing major though.
rfqmsg/records.go
Outdated
if err != nil { | ||
return err | ||
} | ||
list := make([]ID, num) |
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 allocate memory from a number we received over the wire. Need to check and error out the length before to make sure we limit the number of bytes we allocate.
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.
Good point, so far the total length of available RFQ IDs is open ended.
We should introduce a static limit (which would also limit the max number of quotes we acquire) and enforce it everywhere.
@@ -456,16 +457,43 @@ func (s *AuxTrafficShaper) ProduceHtlcExtraData(totalAmount lnwire.MilliSatoshi, | |||
return totalAmount, htlcCustomRecords, nil | |||
} | |||
|
|||
if htlc.RfqID.ValOpt().IsNone() { | |||
return 0, nil, fmt.Errorf("no RFQ ID present in HTLC blob") | |||
if htlc.AvailableRfqIDs.IsNone() { |
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 re backward compatibility. And also just to make sure we don't miss any edge case. After all, we explicitly set the singular htlc.RfqID
in this function, so are we a 100% sure there is no code path where either the ProduceHtlcExtraData
or PaymentBandwidth
would be called with htlc.RfqID
set?
IMO we should check both and only error out if none of them (or both) are set at the same time, with a comment that we'd only really expect the list to be set in the future.
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.
For the PaymentBandwidth
and ProduceHtlcExtraData
the htlc.RfqID
is replaced by htlc.AvailableRfqIDs
The only use of htlc.RfqID
is when ProduceHtlcExtraData
locks into a specific RFQ, which will be used to produce the actual HTLC that will be sent out to our peer
From that point forward everything is the same (we only use the htlc.RfqID
to route HTLCs / accept HTLCs to invoices)
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 re using an existing quote in the SendPayment
RPC... Don't we have that covered by an itest?
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.
Yeap, we have it covered, and the itest was failing! so +1 for our coverage, but I realize I never noticed the itest failing
So the reason it failed is because we were setting the existing RPC user provided RFQ as the single RfqID, instead it had to change to instead set it as the only available RFQ ID in the new field
2bb2369
to
df8ce52
Compare
Pull Request Test Coverage Report for Build 16071985844Details
💛 - Coveralls |
Please update taproot-assets/taprpc/tapchannelrpc/tapchannel.proto Lines 125 to 130 in df8ce52
group_key in addition to asset_id in there.
|
What is this expected to do with an invoice with no amount specified? |
taproot-assets/taprpc/tapchannelrpc/tapchannel.proto Lines 139 to 144 in df8ce52
but with multi-rfq send, I would expect the ability to provide an array. Not sure that you want to fix that in this PR, but maybe it should be a separate issue? I did not make this comment for multi-rfq receive because of #1442 . |
df8ce52
to
aa55b42
Compare
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.
Did another pass, getting very close here too.
"outgoing channel") | ||
// If the HTLC doesn't have any available RFQ IDs, it's incomplete, and | ||
// we cannot determine the bandwidth. | ||
if htlc.AvailableRfqIDs.IsNone() { |
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.
So you're 100% certain for all the code paths that lead into paymentBandwidth
there isn't a single one where potentially we only have the old RFQ ID set? What about this for example:
Line 7674 in 2e7ae0b
nil, fn.Some(quote.ID), fn.None[[]rfqmsg.ID](), |
default: | ||
return 0, fmt.Errorf("no accepted quote found for RFQ ID "+ | ||
"%x (SCID %d)", rfqID[:], rfqID.Scid()) | ||
if bandwidth > 0 { |
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 think "Given we establish 1 quote per peer, only one of the RFQ IDs in the array should produce some bandwidth." would probably be a good comment to add above this statement.
@@ -456,16 +457,43 @@ func (s *AuxTrafficShaper) ProduceHtlcExtraData(totalAmount lnwire.MilliSatoshi, | |||
return totalAmount, htlcCustomRecords, nil | |||
} | |||
|
|||
if htlc.RfqID.ValOpt().IsNone() { | |||
return 0, nil, fmt.Errorf("no RFQ ID present in HTLC blob") | |||
if htlc.AvailableRfqIDs.IsNone() { |
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 re using an existing quote in the SendPayment
RPC... Don't we have that covered by an itest?
aa55b42
to
019b54a
Compare
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 comments around robustness within the context of multi peer RFQ queries.
rpcserver.go
Outdated
// For each peer that satisfies our query above, we'll try and | ||
// establish an RFQ quote for the asset specifier and max amount | ||
// of this payment. | ||
for peer := range chanMap { | ||
quote, err := r.acquireSellOrder( | ||
ctx, &rpcSpecifier, paymentMaxAmt, expiry, | ||
&peer, | ||
) |
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 think the code would be better if this for-loop happened in a separate function.
I think acquireSellOrder
is blocking, am I right? If so then the first peer could block until expiry
(which i think is the invoice expiry) and then we don't have a chance to query any other peer.
We might need to call r.acquireSellOrder
for each peer synchronously here. And the quote response timeout should be much less than the invoice expiry.
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.
Changed the quote acquisition to be async, also used a shorter timeout for the RFQ negotiation
Getting a vague error with
|
The RPC method now uses all of the introduced features. Instead of acquiring just one quote we now extract that logic into a helper and call it once for each valid peer. We then encode the array of available RFQ IDs into the first hop records and hand it over to LND.
019b54a
to
d90193a
Compare
"outgoing channel") | ||
// If the HTLC doesn't have any available RFQ IDs, it's incomplete, and | ||
// we cannot determine the bandwidth. | ||
if htlc.AvailableRfqIDs.IsNone() { |
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 think we should add a commit at the end of this PR that removes the single RFQ ID from the rfqmsg.NewHtlc()
function. That way we're sure we don't accidentally have this problem in other places.
default: | ||
return 0, fmt.Errorf("no accepted quote found for RFQ ID "+ | ||
"%x (SCID %d)", rfqID[:], rfqID.Scid()) | ||
if bandwidth > 0 { |
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.
Comment still missing.
@@ -217,3 +217,7 @@ replace github.com/golang-migrate/migrate/v4 => github.com/lightninglabs/migrate | |||
|
|||
// Note this is a temproary replace and will be removed when taprpc is tagged. | |||
replace github.com/lightninglabs/taproot-assets/taprpc => ./taprpc | |||
|
|||
replace github.com/lightningnetwork/lnd => github.com/GeorgeTsagk/lnd v0.0.0-20250702100924-4ad84627e108 |
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.
Can bump this to v0.19.2-beta.rc1
(which should not need the sqldb
rewrite anymore).
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.
Can bump this to
v0.19.2-beta.rc1
(which should not need thesqldb
rewrite anymore).
now it's v0.19.2-beta
!
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.
seems like really this should remove the replace and update
Line 33 in d90193a
github.com/lightningnetwork/lnd v0.19.1-beta.rc1.0.20250623232057-b48e2763a798 |
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.
Can bump this to
v0.19.2-beta.rc1
(which should not need thesqldb
rewrite anymore).now it's
v0.19.2-beta
!
actually lnd v0.19.2-beta
is not compatible with this PR because it does not use github.com/lightningnetwork/lightning-onion v1.2.1-0.20240815225420-8b40adf04ab9
@@ -328,6 +340,10 @@ func (s *AuxTrafficShaper) paymentBandwidth(htlc *rfqmsg.Htlc, | |||
return 0, err | |||
} | |||
|
|||
// We know that we establish 1 quote per peer in the scope of |
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.
Ah, here it is. Please move this to the previous commit.
|
||
// multiRfqNegotiationTimeout is the timeout within which we expect our | ||
// peer and our node to reach an agreement over a quote. This timeout | ||
// is a bit shorter |
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: incomplete comment.
// We make sure to always write the result to a channel before | ||
// returning. This way the collector can expect a certain number | ||
// of items via the channels. | ||
go func(peer route.Vertex) { |
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 think we should use errgroup.Group
instead, would make the error cases a bit more explicit.
r.RejectedQuote.ErrorMessage) | ||
// Send out the information about the acquired quotes on the | ||
// stream. | ||
for _, quote := range acquiredQuotes { |
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 could also be extracted into a function, would make it a bit more explicit that this only informs the user and that call to rpcutils.UnmarshalRfqFixedPoin()
is only for the log statement.
I seem to get this issue if the sender has multiple taproot asset channels. If the sender is a node with only a single taproot asset channel, it succeeds. |
I was having a problem where I could not make a direct asset payment from alice to bob over an asset channel to an asset invoice. I was getting
then I would also get this went on for a while, but now I cannot reproduce anymore, the direct asset payment now succeeds. Just want to make a comment here in case someone else sees this behavior, or as an initial data point if I get the problem to come back somehow. |
FYI: lightningnetwork/lnd#10056 is limiting my ability to do much complicated testing with this PR. |
OK, problem came back again. This time I was on another node and used
to avoid #1613 (comment) , so it seems the problem is on the receiver's side and not sender's side. On the receiver's side:
On the sender's side
and also for reference
|
As implied in #1613 (comment) , using |
#1565 (comment) might explain why |
Before we merge this PR, I think you all should look at #1565 (comment) and make sure that this is the way we want to do it so that we know that the fix for #1565 for |
From conversations offline, it sounds like we probably do want to change how this works and add a new field which is an array and eventually deprecate the old
|
@ffranr: review reminder |
@@ -217,3 +217,7 @@ replace github.com/golang-migrate/migrate/v4 => github.com/lightninglabs/migrate | |||
|
|||
// Note this is a temproary replace and will be removed when taprpc is tagged. |
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.
is this still needed?
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.
like can we change
Line 32 in d90193a
github.com/lightninglabs/taproot-assets/taprpc v1.0.8 |
v1.0.9
?
|
||
replace github.com/lightningnetwork/lnd => github.com/GeorgeTsagk/lnd v0.0.0-20250702100924-4ad84627e108 | ||
|
||
replace github.com/lightningnetwork/lnd/sqldb => github.com/GeorgeTsagk/lnd/sqldb v0.0.0-20250702100924-4ad84627e108 |
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 with this one
Line 135 in d90193a
github.com/lightningnetwork/lnd/sqldb v1.0.10 // indirect |
@@ -128,7 +128,7 @@ require ( | |||
github.com/lightninglabs/lightning-node-connect/gbn v1.0.1 // indirect | |||
github.com/lightninglabs/lightning-node-connect/mailbox v1.0.1 // indirect | |||
github.com/lightninglabs/neutrino v0.16.1 // indirect | |||
github.com/lightningnetwork/lightning-onion v1.2.1-0.20240712235311-98bd56499dfb // indirect | |||
github.com/lightningnetwork/lightning-onion v1.2.1-0.20240815225420-8b40adf04ab9 // indirect |
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.
What is the purpose of this change? If I rebase this PR onto main and keep this line, I get
# github.com/lightningnetwork/lnd/routing/blindedpath
/home/zzz/go/pkg/mod/github.com/lightningnetwork/[email protected]/routing/blindedpath/blinded_path.go:291:21: blindedPath.BlindedHops undefined (type *sphinx.BlindedPathInfo has no field or method BlindedHops)
/home/zzz/go/pkg/mod/github.com/lightningnetwork/[email protected]/routing/blindedpath/blinded_path.go:300:14: blindedPath.BlindedHops undefined (type *sphinx.BlindedPathInfo has no field or method BlindedHops)
/home/zzz/go/pkg/mod/github.com/lightningnetwork/[email protected]/routing/blindedpath/blinded_path.go:301:15: blindedPath.IntroductionPoint undefined (type *sphinx.BlindedPathInfo has no field or method IntroductionPoint)
/home/zzz/go/pkg/mod/github.com/lightningnetwork/[email protected]/routing/blindedpath/blinded_path.go:311:44: blindedPath.BlindingPoint undefined (type *sphinx.BlindedPathInfo has no field or method BlindingPoint)
/home/zzz/go/pkg/mod/github.com/lightningnetwork/[email protected]/routing/blindedpath/blinded_path.go:312:44: blindedPath.BlindedHops undefined (type *sphinx.BlindedPathInfo has no field or method BlindedHops)
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.
it seems tapd main branch is on lnd v0.19.2-beta
, but lnd v0.19.2-beta
does not yet use github.com/lightningnetwork/lightning-onion v1.2.1-0.20240815225420-8b40adf04ab9
, so that is the cause of the error messages. it seems lightningnetwork/lnd#9980 was cherry picked onto lnd v0.19.2-beta
off of master and that is why I got confused.
Description
Allows payments to use multiple quotes across multiple peers in order to pay an invoice. Previously we had to define a specific peer to negotiate a quote with, with this PR this is no longer required (but still supported) as Tapd will automatically scan our peers and establish quotes with all valid ones for the asset/amount of this payment.
The signature of
ProduceHtlcExtraData
had to be changed, as it's not possible to distinguish which of the quotes in therfqmsg.Htlc
should be used. We now provide the pubkey of the peer this HTLC is being sent to, in order to help Tapd extract the corresponding quote and calculate the correct amount of asset units.Closes #1358
Depends on: lightningnetwork/lnd#9980