contractcourt: add mempool watcher to notify mempool events by yyforyongyu · Pull Request #7564 · lightningnetwork/lnd

yyforyongyu · 2023-04-03T13:38:29Z

This PR adds a new subsystem to allow subscription of mempool events. In specific, it allows the caller to watch the spending event of a given utxo, and returns the spending tx when found in mempool.

Fixes #4254
Depends on

TODO

Roasbeef · 2023-04-03T18:54:26Z

Nice, this'll serve to reduce the perceived swap latency of swap services like Loop, as the swap can be completed as soon as the transaction hits the mempool (no need to wait for the eventual conf, which might take some time w/ todays mempool).

chainntnfs/log.go

chainntnfs/mempool.go

chainntnfs/bitcoindnotify/bitcoind.go

chainntnfs/btcdnotify/btcd.go

chainntnfs/interface.go

contractcourt/htlc_timeout_resolver.go

Roasbeef · 2023-04-06T00:39:33Z

Changed the base branch to master.

Roasbeef · 2023-04-06T00:42:24Z

Dep btcwallet branch is merged.

Roasbeef

This is really coming along, nice work!

I think the final design thing here is that: we should still be fetching the mempool on start up so we can handle the restart case properly.

chainntnfs/mempool.go

chainntnfs/bitcoindnotify/bitcoind.go

chainntnfs/mempool.go

contractcourt/htlc_timeout_resolver.go

lnutils/sync_map.go

contractcourt/htlc_timeout_resolver.go

Crypt-iQ

Think it's very close

chainntnfs/mempool.go

Crypt-iQ · 2023-04-10T14:26:12Z

chainntnfs/mempool.go

does this need to be stored back into subscribedInputs? That pattern was used elsewhere, but doesn't seem necessary since clients is a pointer?

I suspect so, usually whenever I mess with multiple layers of maps, I always need to update the intermediate value all the wya up the parent stack.

A unit test should reveal what actual behavior here.

yeah this is just a pointer so no need to store it back. This is also why clients is also a sync map so it's safe to update it concurrently.

There's a unit test TestMempoolUnsubscribeEvent checks that the clients here are updated when checking subscribedInputs map.

Don't see this test, typo or do you mean TestMempoolUnsubscribeConfirmedSpentTx?

the test is in the following commit

Crypt-iQ · 2023-04-10T15:02:29Z

contractcourt/htlc_incoming_contest_resolver.go

Commit message is slightly inaccurate here. For LND, the outgoing broadcast delta is 0, so typically the force close is broadcasted when currentHeight == h.htlcExpiry unless there are other HTLCs, user-initiated, etc. Other implementations may broadcast with non-zero deltas though. The earliest the timeout transaction can be confirmed is at height h.htlcExpiry+1 so the conditional could instead read if uint32(currentHeight) > h.htlcExpiry to stop preimage-claiming as soon as the counterparty can time it out. I don't think it makes a big difference here though and we could potentially even bump the h.htlcExpiry+1 to something larger to account for variable fee environments

Cool this commit is now removed. It was added to trigger the race condition used in the itest testExtraMempoolPreimage. Now that test will only be added in the fee bumper PR, will also move this commit and its related discussion there.

Crypt-iQ · 2023-04-10T15:05:40Z

contractcourt/htlc_timeout_resolver.go

If there is a preimage spend in the mempool, it will cause the resolver to clean up and mark it as ResolverOutcomeClaimed. But if the timeout tx eventually confirms, the resolver will not be around to mark it as ResolverOutcomeTimeout. This might mess up somebody's accounting. I think it would be better if the resolver wasn't cleaned up until an on-chain resolution occurs. We should be fine with sending multiple resolution messages for a single CircuitKey even if the types (success vs fail) differ. This newer behavior could also use an itest. If it's not possible to make an itest that we can commit, a hacky itest would also work that tests the behavior of duplicate resolution messages sent to the switch.

IIUC, then below, even once we see the transaction in the mempool, we'll wait until things are fully confirmed before we try to clean up state. See the consumeSpendEvents method below. After we see things in the mempool we resolve then send back, but wait to exit the loop until things are confirmed on chain.

Or do you mean that the incoming link should n't also be resolved (on disk) until the outgoing one? In the scenario where both links need to go on chain.

This might mess up somebody's accounting.

Was thinking about this too, and wondering how one could find out the extra htlc "earned" from the timeout path.
And does it also mean we can rely on the bucket contractsBucketKey to tell if an htlc is settled or failed? If so we can get rid of the finalHtlcsBucket?

but wait to exit the loop until things are confirmed on chain.

The loop will continue, but the calling function waitForMempoolOrBlockSpend will exit immediately. If the timeout actually hits the chain, then handleCommitSpend won't be called. This would leave the output from the 2nd-level-timeout unclaimed and also give inaccurate accounting. The user would be made whole since they're claiming on the incoming link with the preimage, but this case would leave the value of the outgoing HTLC on the table.

Keeping this behavior might be acceptable since there are no funds at risk and it avoids introducing potential complexity to the switch's resolution message logic.

And does it also mean we can rely on the bucket contractsBucketKey to tell if an htlc is settled or failed?

I don't think so since when a contract (which includes success/timeout claims) is resolved, it is deleted from contractsBucketKey.

itest/lnd_multi-hop_test.go

chainntnfs/mempool.go

Roasbeef · 2023-04-11T00:19:56Z

chainntnfs/mempool.go

I suspect so, usually whenever I mess with multiple layers of maps, I always need to update the intermediate value all the wya up the parent stack.

A unit test should reveal what actual behavior here.

chainntnfs/bitcoindnotify/bitcoind.go

chainntnfs/interface.go

Roasbeef · 2023-04-11T00:28:14Z

contractcourt/htlc_timeout_resolver.go

IIUC, then below, even once we see the transaction in the mempool, we'll wait until things are fully confirmed before we try to clean up state. See the consumeSpendEvents method below. After we see things in the mempool we resolve then send back, but wait to exit the loop until things are confirmed on chain.

Or do you mean that the incoming link should n't also be resolved (on disk) until the outgoing one? In the scenario where both links need to go on chain.

itest/lnd_multi-hop_test.go

Roasbeef · 2023-04-13T23:54:58Z

itest/lnd_multi-hop_test.go

Alternatively, could to ht.Skipf if we detect it's a neutrino backend at the top.

agree should be skipped

I don't think we have test coverage for this resolver for neutrino tho

chainntnfs/mempool.go

Crypt-iQ · 2023-04-14T17:15:46Z

itest/lnd_multi-hop_test.go

agree should be skipped

Crypt-iQ

LGTM 🚜 after btcwallet dep updated, also a linter error?

This commit adds a mempool notifier which notifies the subscriber the spending event found in the mempool for a given input.

This commit changes the `subscribedInputs` to store a map of subscribers so multiple subscribers are allowed to receive events from the same outpoint.

This commit adds the method `UnsubscribeEvent` to cancel a single subscription.

This commit adds the mempool watcher to bitcoind notifier to allow the notifier managing the starting and stopping of the watcher.

This commit adds the mempool watcher to btcd notifier to allow the notifier managing the starting and stopping of the watcher.

This commit adds the interface `MempoolWatcher` and uses it in the chain registry.

Also fixes the docs and rename `isSuccessSpend` to `isPreimageSpend`.

This commit extends the current htlc timeout resolver to also watch for preimage spend in mempool for a full node backend. If mempool enabled, the resolver will watch the spend of the htlc output in mempool and blocks **concurrently**, as if they are independent. Ideally, a transaction will first appear in mempool then in a block. However, there's no guarantee it will appear in **our** mempool since there's no global mempool, thus we need to watch the spend in two places in case it doesn't go through our mempool. The current design favors the spend event found in blocks, that is, when the tx is confirmed, we'd abort the monitoring and conitnue since the outpoint cannot be double spent and re-appear in mempool again. This is not true in the rare case of reorg, and we will handle reorg seperately.

This commit removes the subscribed inputs from mempool notifier when the relevant transaction is confirmed.

This commit adds more debug logs for witness beacon and channel arbitrator.

Roasbeef

LGTM 🦄

yyforyongyu added channel closing Related to the closing of channels cooperatively and uncooperatively HTLC mempool labels Apr 3, 2023

yyforyongyu added this to the v0.16.1 milestone Apr 3, 2023

yyforyongyu self-assigned this Apr 3, 2023

yyforyongyu force-pushed the mempool-watch branch 4 times, most recently from 94ac9ab to f0d19e7 Compare April 3, 2023 15:38

Roasbeef requested changes Apr 3, 2023

View reviewed changes

yyforyongyu force-pushed the mempool-watch branch 3 times, most recently from 19381ba to 71d8d23 Compare April 5, 2023 21:24

Roasbeef changed the base branch from 0-16-1-staging to master April 6, 2023 00:39

Roasbeef requested changes Apr 6, 2023

View reviewed changes

Crypt-iQ reviewed Apr 6, 2023

View reviewed changes

yyforyongyu force-pushed the mempool-watch branch 2 times, most recently from 538b817 to bcf4473 Compare April 7, 2023 11:16

yyforyongyu marked this pull request as ready for review April 7, 2023 11:16

yyforyongyu changed the title ~~WIP: add mempool watcher to notify mempool events~~ contractcourt: add mempool watcher to notify mempool events Apr 7, 2023

yyforyongyu requested review from Crypt-iQ and Roasbeef April 7, 2023 11:23

yyforyongyu force-pushed the mempool-watch branch from bcf4473 to dc43f80 Compare April 7, 2023 12:30

Crypt-iQ reviewed Apr 10, 2023

View reviewed changes

Roasbeef reviewed Apr 11, 2023

View reviewed changes

yyforyongyu force-pushed the mempool-watch branch 2 times, most recently from a61124f to 2c33000 Compare April 11, 2023 07:43

Roasbeef reviewed Apr 13, 2023

View reviewed changes

Crypt-iQ reviewed Apr 14, 2023

View reviewed changes

yyforyongyu force-pushed the mempool-watch branch from 1271a6a to eb870f0 Compare April 17, 2023 10:11

Crypt-iQ approved these changes Apr 17, 2023

View reviewed changes

yyforyongyu force-pushed the mempool-watch branch from eb870f0 to d9a1fd7 Compare April 18, 2023 11:35

yyforyongyu added 15 commits April 19, 2023 01:18

chainntnfs: introduce mempool notifier

81ee3b9

This commit adds a mempool notifier which notifies the subscriber the spending event found in the mempool for a given input.

chainntnfs: allow multiple subscriber to the same input

5743dd9

This commit changes the `subscribedInputs` to store a map of subscribers so multiple subscribers are allowed to receive events from the same outpoint.

chainntnfs: add method UnsubscribeEvent to cancel a subscription

3b77e9c

This commit adds the method `UnsubscribeEvent` to cancel a single subscription.

chainntnfs: patch unit tests for mempool notifier

8b42151

chainntnfs+chainreg: add mempool watcher to BitcoindNotifier

454e76c

This commit adds the mempool watcher to bitcoind notifier to allow the notifier managing the starting and stopping of the watcher.

chainntnfs+chainreg: add mempool watcher to BtcdNotifier

ad5e798

This commit adds the mempool watcher to btcd notifier to allow the notifier managing the starting and stopping of the watcher.

chainntnfs+chainreg: add interface MempoolWatcher

e81d62f

This commit adds the interface `MempoolWatcher` and uses it in the chain registry.

contractcourt: refactor spendHtlcOutput for clarity

7ef8072

Also fixes the docs and rename `isSuccessSpend` to `isPreimageSpend`.

chainntnfs: remove subscriptions when the relevant tx is confirmed

e9c10bf

This commit removes the subscribed inputs from mempool notifier when the relevant transaction is confirmed.

lnd+contractcourt: add more debug logs

dccf9c7

This commit adds more debug logs for witness beacon and channel arbitrator.

lntest: add more methods to assert mempool state

774db0d

itest+lntest: add itest for mempool preimage watch

e715b8e

docs: update release notes for mempool watch

f72d6df

mod: update btcwallet version

d5db195

yyforyongyu force-pushed the mempool-watch branch from d9a1fd7 to 437a329 Compare April 18, 2023 17:18

chainreg: update bitcoind polling config

66d1392

yyforyongyu force-pushed the mempool-watch branch from 437a329 to 66d1392 Compare April 18, 2023 17:24

Roasbeef approved these changes Apr 18, 2023

View reviewed changes

Roasbeef merged commit ec3cb69 into lightningnetwork:master Apr 18, 2023

This was referenced Apr 18, 2023

contractcourt: create new mempool witness script template notification system #4254

Closed

release: create v0.16.1-beta.rc1 branch #7611

Merged

yyforyongyu deleted the mempool-watch branch April 19, 2023 06:04

yyforyongyu mentioned this pull request Apr 19, 2023

Start watching mempool in htlc outgoing contest resolver #7615

Open

yyforyongyu mentioned this pull request Mar 26, 2024

[3/5]sweep: make pending inputs stateful #8423

Merged

Conversation

yyforyongyu commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Roasbeef commented Apr 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Roasbeef commented Apr 6, 2023

Uh oh!

Roasbeef commented Apr 6, 2023

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Crypt-iQ left a comment

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!

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!

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!

Crypt-iQ Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yyforyongyu commented Apr 3, 2023 •

edited

Loading

Crypt-iQ Apr 11, 2023 •

edited

Loading

yyforyongyu Apr 17, 2023 •

edited

Loading