channels: check RFQ quote compatibility with channel at pathfind time #1583
Conversation
guggero
added
payment-channel
RFQ
Pull Request Test Coverage Report for Build 15448702081Details
💛 - Coveralls |
Adds a test case to validate the fix in lightninglabs/taproot-assets#1583, by adding a test that: - Creates two asset channels between Alice and Bob - Creates a BTC channel between Bob and Charlie - The two asset channels each have a different asset in them - The balance of the pences channel is decreased (lower bandwidth) - An RFQ payment is attempted, with pences as the payment asset
|
I've verified this by adding a commit to lightninglabs/lightning-terminal#1082, which currently makes the tests fail. |
tapchannelmsg/wire_msgs_test.go
Outdated
| // ExtractHexDump extracts the hex bytes from a hex dump string, which is | ||
| // typically formatted with an offset, hex bytes, and ASCII representation. | ||
| func ExtractHexDump(input string) ([]byte, error) { |
There was a problem hiding this comment.
hex bytes?
// ExtractHexDump parses a hex dump string (typically formatted with offsets,
// hex-encoded byte values, and ASCII representation) and returns the decoded bytes.
There was a problem hiding this comment.
Also, how were these .hexdump files generated? Maybe we should write that down somewhere. A README.md in testdata?
There was a problem hiding this comment.
I mentioned in the commit message that these are helpers to decode hex dumps from a trace log of lnd/litd. But I guess it's useful to have that in the Godoc comment too.
tapchannelmsg/records.go
Outdated
| case specifier.HasGroupPubKey(): | ||
| targetGroupKey := specifier.UnwrapGroupKeyToPtr() | ||
| if targetGroupKey == nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
if the specifier has a group pub key but then we end up with targetGroupKey == nil, that's an error. I don't think it means that the channel is incompatible with the specifier necessarily because asset ID might still match.
maybe use specifier.UnwrapGroupKeyOrErr here and return an error?
There was a problem hiding this comment.
We check with specifier.HasGroupPubKey() above, so the nil case really should never happen. It's just defensive. I don't think it makes sense to return an error just for that.
To debug issues with channels, it's super helpful if we can decode the long hexdumps of the funding, commitment and HTLC blobs. This commit adds simple unit tests that decode those blobs and outputs them as JSON.
tapchannelmsg/records.go
Outdated
| // If the specifier has a group key, then we must have a group | ||
| // key set on the OpenChannel. | ||
| return lfn.MapOptionZ( | ||
| o.GroupKey.ValOpt(), |
There was a problem hiding this comment.
I'm not sure this is correct, if the specifier has a group key, we could still use the channel even if it was funded with a single asset ID, if that asset ID is part of the group
There was a problem hiding this comment.
But the specifier here is what was used by the user to create the quote. And if the quote was created for a group key, then the channel needs to have a group defined as well.
If the user creates a quote for a single asset ID in a grouped channel that only has that asset ID, then the specifier here will have the asset ID.
There was a problem hiding this comment.
Hmm, but I guess just re-using the AssetMatchesSpecifier function does make sense... Will try it.
During pathfinding, when an HTLC doesn't have an asset ID or group key encoded, we can only find out if a channel is compatible after looking at the specifier of the quote. We add that to make sure pathfinding doesn't give false positives for channels that then can't be used because they're not compatible.
|
|
||
| // One of the asset IDs in the channel does not match the quote, | ||
| // we don't want to route this HTLC over this channel. | ||
| if !match { |
Adds a test case to validate the fix in lightninglabs/taproot-assets#1583, by adding a test that: - Creates two asset channels between Alice and Bob - Creates a BTC channel between Bob and Charlie - The two asset channels each have a different asset in them - The balance of the pences channel is decreased (lower bandwidth) - An RFQ payment is attempted, with pences as the payment asset
Adds a test case to validate the fix in lightninglabs/taproot-assets#1583, by adding a test that: - Creates two asset channels between Alice and Bob - Creates a BTC channel between Bob and Charlie - The two asset channels each have a different asset in them - The balance of the pences channel is decreased (lower bandwidth) - An RFQ payment is attempted, with pences as the payment asset
This fixes an edge case in the pathfinding logic of
lnd(reported on Slack):asset2, both channels report bandwidth as shown above, because we didn't have the check in this PR.lndskipped the second channel with the log message2025-06-03 12:23:32.296 [DBG] CRTR: Skipped edge 4561176653273366528: not max bandwidth, bandwidth=41166477370 mSAT, maxBandwidth=53340397469 mSATThis PR breaks this cycle by not reporting any bandwidth for the first channel, since from the quote we can figure out the channel isn't actually compatible.