Track HTLCs in rfq policies #1186

GeorgeTsagk · 2024-11-11T13:44:02Z

Description

This PR adds a simple in-memory tracking of accepted htlcs in the RFQ policy level. HTLCs that passed the policy check are now tracked by each policy, possibly affecting future policy check outcomes (depends on individual policy's implementation)

coveralls · 2024-11-11T13:53:12Z

Pull Request Test Coverage Report for Build 12074140759

Details

0 of 134 (0.0%) changed or added relevant lines in 5 files are covered.
19 unchanged lines in 5 files lost coverage.
Overall coverage decreased (-0.09%) to 40.796%

Changes Missing Coverage	Changed/Added Lines	%
rfq/manager.go	1	0.0%
tapcfg/server.go	1	0.0%
chain_bridge.go	4	0.0%
rpcserver.go	16	0.0%
rfq/order.go	112	0.0%

Files with Coverage Reduction	New Missed Lines	%
tappsbt/create.go	2	53.22%
tapchannel/aux_leaf_signer.go	3	43.43%
universe/interface.go	4	51.95%
tapgarden/caretaker.go	4	68.87%
tapdb/multiverse.go	6	68.21%

Totals
Change from base Build 12057455142:	-0.09%
Covered Lines:	25787
Relevant Lines:	63210

💛 - Coveralls

guggero

As mentioned offline, we also need to track and take into account canceled HTLCs from previous attempts.
These can be subscribed to with https://lightning.engineering/api-docs/api/lnd/router/subscribe-htlc-events.

guggero · 2024-11-11T14:26:28Z

rfq/order.go

+// TrackAcceptedHtlc accounts for the newly accepted htlc. This may affect the
+// acceptance of future htlcs.
+func (c *AssetSalePolicy) TrackAcceptedHtlc(htlc lndclient.InterceptedHtlc) {
+	c.CurrentAmountMsat += htlc.AmountOutMsat


Not sure if these need to be atomic values? Since multiple goroutines could manipulate them concurrently.

rfq/order.go

ffranr

I'm up-to-date with this PR. I agree with Oli's comments.

GeorgeTsagk · 2024-11-18T18:10:06Z

@ffranr @guggero
I updated the PR to now include an htlc event subscription.
We now maintain a map of htlc circuit keys to rfq policy
When an htlc forward is failed we retrieve the htlc circuit key, fetch the corresponding policy and have it to stop tracking the htlc.

GeorgeTsagk · 2024-11-18T18:11:32Z

Will also see how to add some coverage here

ffranr · 2024-11-19T19:38:07Z

Will also see how to add some coverage here

@GeorgeTsagk should i review this before you add coverage or wait for that extra changes?

GeorgeTsagk · 2024-11-22T12:22:05Z

Will also see how to add some coverage here

@GeorgeTsagk should i review this before you add coverage or wait for that extra changes?

You can review
Coverage will soon be added in the LitD itests

tapchannel/aux_leaf_signer.go

tapchannel/aux_leaf_signer_test.go

rfq/order.go

GeorgeTsagk · 2024-11-28T18:44:08Z

Rebased on #1224 to include manual rfq scid functionality in SendPayment

guggero

Looks pretty good. Left a couple of comments to simplify the code and make it more concurrency safe.

guggero · 2024-11-29T15:39:05Z

chain_bridge.go

@@ -376,9 +377,17 @@ func (l *LndRouterClient) DeleteLocalAlias(ctx context.Context, alias,
 	return l.lnd.Router.XDeleteLocalChanAlias(ctx, alias, baseScid)
 }

+func (l *LndRouterClient) SubscribeHtlcEvents(


nit: requires Godoc comment (for the same method in the HtlcSubscriber interface as well).

guggero · 2024-11-29T15:43:49Z

rfq/order.go

 	// AskAssetRate is the quote's asking asset unit to BTC conversion rate.
 	AskAssetRate rfqmath.BigIntFixedPoint

+	// htlcToAmt maps the unique htlc identifiers to the effective amount
+	// that they carry.
+	htlcToAmt lnutils.SyncMap[string, lnwire.MilliSatoshi]


So both the htlcToAmt and CurrentAmountMsat need to be in sync. If we use an atomic value for one and a sync map for the other, then it's still possible for a read from another goroutine to get a different value while we're updating things (e.g. the map was updated but the total not yet).

So I think because we have two separate values, we need a sync.RWMutex instead. But then we can use a normal map here at least.

guggero · 2024-11-29T15:44:06Z

rfq/order.go

@@ -152,7 +168,8 @@ func (c *AssetSalePolicy) CheckHtlcCompliance(
 		maxAssetAmount, c.AskAssetRate,
 	)

-	if htlc.AmountOutMsat > policyMaxOutMsat {
+	if (c.CurrentAmountMsat + htlc.AmountOutMsat) > policyMaxOutMsat {
+		// if htlc.AmountOutMsat > policyMaxOutMsat {


nit: commented out code.

guggero · 2024-11-29T15:46:52Z

rfq/order.go

+
+	c.CurrentAmountMsat += htlc.AmountOutMsat
+
+	htlcIDStr := htlcIdentifierStr(


I think we can probably simplify the interface of this TrackAcceptedHtlc a bit by just passing in circuitKey and amount. Then it's IMO a bit more apparent what's happening (looks more like a map add and delete) and the policy doesn't need to know how the key is calculated.

guggero · 2024-11-29T15:49:17Z

rfq/order.go

+func (c *AssetSalePolicy) TrackAcceptedHtlc(htlc lndclient.InterceptedHtlc) {
+	c.CurrentAmountMsat += htlc.AmountOutMsat
+
+	htlcIDStr := htlcIdentifierStr(


We can use structs as map keys directly. And as long as there are no pointer values in a struct (meaning the struct can be directly compared with == without the pointers leading to side effects), things work out as expected. So we can just use the circuit key directly as the map key, no need to convert to string.

guggero · 2024-11-29T15:50:15Z

rfq/order.go

+		return
+	}
+
+	c.CurrentAmountMsat -= amt


Yeah, we definitely need a mutex here. Otherwise another goroutine might read this value while we're waiting on the LoadAndDelete above to complete.

guggero · 2024-11-29T15:51:21Z

rfq/order.go

@@ -95,9 +103,17 @@ type AssetSalePolicy struct {
 	// the policy.
 	MaxOutboundAssetAmount uint64

+	// CurrentAssetAmountMsat is the total amount that is held currently in
+	// accepted htlcs.


nit: use the same capitalization for HTLC everywhere? So s/htlcs/HTLCs here and throughout the file.

guggero · 2024-11-29T15:54:24Z

rfq/order.go

+	// the policy that applies to it. We need this map because for failed
+	// HTLCs we don't have the RFQ data available, so we need to cache this
+	// info.
+	htlcToPolicy lnutils.SyncMap[string, Policy]


Same here, can use the circuit key as the map key directly. Here a sync map is enough and we don't need a mutex, as it's just a single map that needs to be synced.

guggero · 2024-11-29T15:57:23Z

rfq/order.go

+			case linkFail != nil:
+				// Fetch the policy that is related to this
+				// htlc.
+				policy, found :=


nit: format as:

policy, found := h.htlcToPolicy.LoadAndDelete( htlcIDStr, )

instead.

GeorgeTsagk requested review from guggero and ffranr November 11, 2024 13:44

GeorgeTsagk self-assigned this Nov 11, 2024

guggero requested changes Nov 11, 2024

View reviewed changes

dstadulis added the rfq label Nov 11, 2024

dstadulis added this to the v0.5 (v0.4.2 rename) milestone Nov 11, 2024

GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from 781102e to e559106 Compare November 13, 2024 15:32

ffranr reviewed Nov 18, 2024

View reviewed changes

GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from e559106 to e5edcfd Compare November 18, 2024 18:08

GeorgeTsagk requested review from guggero and ffranr November 18, 2024 18:11

GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from e5edcfd to 1fd418e Compare November 22, 2024 17:42

GeorgeTsagk mentioned this pull request Nov 22, 2024

itest: Extend custom channel liquidity itest for RFQ HTLC tracking lightninglabs/lightning-terminal#906

Open

ffranr reviewed Nov 27, 2024

View reviewed changes

GeorgeTsagk force-pushed the rfq-track-accepted-amount branch 2 times, most recently from af862cd to 0dd58d6 Compare November 28, 2024 09:46

GeorgeTsagk added 4 commits November 28, 2024 17:04

taprpc: add scid parameter to tchrpc.SendPayment

4eb7e88

rpcserver: skip rfq negotiation on manual SendPayment rfq

f2557d3

multi: add SubscribeHtlcEvents call

8acf339

rfq: policies track accepted htlcs

5c62e34

GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from 0dd58d6 to 5c62e34 Compare November 28, 2024 18:43

GeorgeTsagk requested a review from ffranr November 28, 2024 18:51

guggero reviewed Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track HTLCs in rfq policies #1186

Track HTLCs in rfq policies #1186

GeorgeTsagk commented Nov 11, 2024

coveralls commented Nov 11, 2024 •

edited

Loading

guggero left a comment

guggero Nov 11, 2024

ffranr left a comment

GeorgeTsagk commented Nov 18, 2024

GeorgeTsagk commented Nov 18, 2024

ffranr commented Nov 19, 2024

GeorgeTsagk commented Nov 22, 2024

GeorgeTsagk commented Nov 28, 2024

guggero left a comment

guggero Nov 29, 2024

guggero Nov 29, 2024

guggero Nov 29, 2024

guggero Nov 29, 2024

guggero Nov 29, 2024

guggero Nov 29, 2024

guggero Nov 29, 2024

guggero Nov 29, 2024

guggero Nov 29, 2024


		c.CurrentAmountMsat += htlc.AmountOutMsat

		htlcIDStr := htlcIdentifierStr(

Track HTLCs in rfq policies #1186

Are you sure you want to change the base?

Track HTLCs in rfq policies #1186

Conversation

GeorgeTsagk commented Nov 11, 2024

Description

coveralls commented Nov 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12074140759

Details

💛 - Coveralls

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffranr left a comment

Choose a reason for hiding this comment

GeorgeTsagk commented Nov 18, 2024

GeorgeTsagk commented Nov 18, 2024

ffranr commented Nov 19, 2024

GeorgeTsagk commented Nov 22, 2024

GeorgeTsagk commented Nov 28, 2024

guggero left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 11, 2024 •

edited

Loading