Replaceable txs fee bumping #2113

t-bast · 2021-12-21T13:36:32Z

Alright, a word of advice before we start: this PR is not for the faint of heart.
If you're thinking "I have 30 minutes to spare, let's give this a quick review", you're going to be disappointed.
This is the kind of PR that will keep you awake at night, wondering why you even got into bitcoin in the first place (maybe it wasn't that bad doing boring API plumbing work in a SaaS company after all!).

But if you're one of the brave few who like a challenge, clear up your agenda for the next few days, grab a lot of coffee, get into your cozy don't-disturb-me-until-I'm-done clothes, and read on. 😎 ⚡

In this PR, we introduce a deadline to replaceable transactions, and do the best we can to get these transactions confirmed before we reach that deadline.

For htlc transactions, that deadline is the htlc expiry:

if you have the preimage, you must absolutely get your transaction confirmed before your peer can use the timeout path, otherwise you may lose funds
if you're using the timeout path, you must get your transaction confirmed as soon as possible, to get your money back even if your peer reveals the preimage late

For commitment transactions, there are two cases:

there are no htlc outputs you can claim: in that case there's no reason to rush, no funds are at risk, you can wait for a few blocks to see your commit tx confirmed
there are htlc outputs you can claim: in that case the commit tx must absolutely be confirmed before the closest htlc expiry, to ensure that you can then claim those htlcs

You may be a bit lost in all the actors in the publish package, so here is a rough explanation of why the existing layers are important. We need to evaluate the feerate based on the deadline and the current block height only once we're ready to broadcast the transaction: this means it has to be inside the ReplaceableTxPublisher actor, once all the timelocks and preconditions have been fulfilled. But then why do we also need a retry mechanism in the TxPublisher actor? There are a few reasons for that. The first reason is to handle broadcast failures: if our transaction is evicted from the mempool (e.g. replaced by an external transaction), this bubbles up to the TxPublisher, who will then choose whether we should retry or not (depending on whether the external conflicting transaction gets confirmed or not). The second reason is to handle node restarts: we then lose the intermediate publishing state, but we may have put some transactions in the mempool. The TxPublisher will ensure we try different feerates as we get closer to the deadline, and eventually replace the mempool transaction if its feerate was too low. A third reason is that deadlines can change: we may initially have no htlcs to claim from a commit tx, but then we later receive a preimage, and realize that we now need to make the commit tx confirm more quickly than we thought. This request will go through the TxPublisher (which is the entry point for channel requests), which can create a new ReplaceableTxPublisher to replace the pending one.

Each commit is self-contained a has a detailed commit message. I recommend reviewing commit-by-commit to see each step taken towards the end goal. However, after that it may make sense to review everything at once, to zoom out and ensure that this doesn't have any fundamental design flaw.

I thought about splitting this PR into multiple PRs, but the issue is that using only parts of it is less safe than the current master branch: you really need to have the fee-bumping mechanism (that comes in the last commit) to make what the previous commits do safe. Since we know of a few node operators running master on mainnet, I think it's better to merge everything at once.

The things we do in the ReplaceableTxFunder actor to reach the right feerate are very ugly, but bitcoind doesn't give us the tools yet to make it better (because we have inputs that are external to the bitcoind wallet). Please help me lobby for bitcoin/bitcoin#23201 (and a follow-up PR to add the same arguments to the bumpfee RPCs) to allow us to remove those horrible hacks!

codecov-commenter · 2021-12-21T13:50:48Z

Codecov Report

Merging #2113 (25fa5e6) into master (27579a5) will decrease coverage by 0.16%.
The diff coverage is 84.27%.

❗ Current head 25fa5e6 differs from pull request most recent head 6d1b34e. Consider uploading reports for the commit 6d1b34e to get more accurate results

@@            Coverage Diff             @@
##           master    #2113      +/-   ##
==========================================
- Coverage   87.62%   87.46%   -0.17%     
==========================================
  Files         166      169       +3     
  Lines       12886    13139     +253     
  Branches      543      559      +16     
==========================================
+ Hits        11292    11492     +200     
- Misses       1594     1647      +53

Impacted Files	Coverage Δ
.../fr/acinq/eclair/blockchain/fee/FeeEstimator.scala	`100.00% <ø> (ø)`
...in/scala/fr/acinq/eclair/channel/ChannelData.scala	`100.00% <ø> (ø)`
...n/scala/fr/acinq/eclair/json/JsonSerializers.scala	`87.87% <ø> (ø)`
...e/src/main/scala/fr/acinq/eclair/BlockHeight.scala	`60.00% <60.00%> (ø)`
...ir/channel/publish/ReplaceableTxPrePublisher.scala	`66.66% <66.66%> (ø)`
...cinq/eclair/channel/publish/FinalTxPublisher.scala	`84.21% <75.00%> (+1.71%)`	⬆️
...clair/channel/publish/ReplaceableTxPublisher.scala	`81.57% <81.31%> (-3.38%)`	⬇️
...q/eclair/channel/publish/ReplaceableTxFunder.scala	`86.91% <86.91%> (ø)`
...cinq/eclair/channel/publish/MempoolTxMonitor.scala	`88.88% <88.88%> (+6.61%)`	⬆️
...la/fr/acinq/eclair/transactions/Transactions.scala	`96.80% <93.75%> (+0.25%)`	⬆️
... and 35 more

When a channel is force-closed, we tell the `TxPublisher` the block before which the transaction should be confirmed. Then it will be up to the `TxPublisher` to set the feerate appropriately and fee-bump whenever it becomes necessary.

We previously evaluated the feerate when we issued the command to publish a transaction. However, the transaction may actually be published many blocks later (especially for HTLC-timeout transactions, for which we may issue commands long before the timeout has been reached). We change the command to include the deadline, and evaluate the feerate to meet that deadline when we're ready to broadcast a valid transaction.

We should retry replaceable transactions whenever we find a conflict in the mempool: as we get closer to the deadline, we will try higher fees. This ensures we either succeed in replacing the mempool transaction, or it gets confirmed before the deadline, which is good as well. In particular, this handles the case where we restart our node. The mempool conflict can be a transaction that we submitted ourselves, but we've lost the context in the restart. We want to keep increasing the fees of that transaction as we get closer to the deadline, so we need this retry mechanism.

This commit contains very minimal logic changes: it mostly moves code from the ReplaceableTxPublisher to two new actors (the first one to check preconditions, the second one to encapsulate the logic for funding and signing replaceable transactions). This makes it easier to understand the state machine involved in each of these steps, and will make it easier to introduce fee-bumping by reusing these actors with minimal changes.

Once we've published a first version of a replaceable transaction, at every new block we evaluate whether we should RBF it. When we choose to RBF, we try to reuse the previous wallet inputs and simply lower the change amount. When that's not possible, we ask bitcoind to add more wallet inputs.

t-bast · 2022-01-10T13:39:55Z

I have made additional E2E tests on regtest, with pending htlc-success and htlc-timeout. I have run a modified version of eclair where I increased the feerate every minute (using the CurrentFeerates event and a fake CurrentBlockCount event, without creating new blocks so that transactions stay in the mempool).

The transactions are correctly regularly fee-bumped, the state machine seems to correctly handle both cases:

the previous tx is replaced by the fee-bumped one
the fee-bumped one fails to replace the previous tx (fee increment too low)

It is working ok, nothing strange is happening. When we run out of utxos, funding simply fails and sends event to the node operator (at that point eclair cannot do anything until the bitcoind wallet receives new utxos to use for fee-bumping).

pm47 · 2022-01-10T13:57:05Z

pm47

Review for 3b1537e.

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala

* Add a BlockHeight type * Rename deadline to confirmation target * Add comments on HtlcTx type inconsistency

pm47

Review for 9bfdd90.

eclair-core/src/main/scala/fr/acinq/eclair/BlockHeight.scala

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala

* Remove toString on BlockHeight * Add confirmBefore to ReplaceableTransactionWithInputInfo * Add new codecs to add confirmation target to txs

eclair-core/src/main/scala/fr/acinq/eclair/wire/internal/channel/version0/ChannelCodecs0.scala

eclair-core/src/main/scala/fr/acinq/eclair/transactions/Transactions.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala

pm47

Review for cc65aee, not much to say.

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisher.scala

pm47

First pass on d1209ad.

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisher.scala

This avoids passing around fields that are unchanged and repeating a long list of arguments in each behavior.

* Protect against herd effect on new blocks * MempoolTxMonitor broadcasts tx status at every block

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisher.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/MempoolTxMonitor.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisher.scala

We only need a Stop message at the `FinalTxPublisher` and `ReplaceableTxPublisher` level, their child actors will be automatically stopped whenever these parent actors stop themselves.

eclair-core/src/test/resources/logback-test.xml

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala

Instead of letting the TxPublisher create a new, competing replaceable attempt, we can simply tell the existing one to update its confirmation target.

eclair-core/src/test/scala/fr/acinq/eclair/TestConstants.scala

eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPrePublisher.scala

eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxFunder.scala

eclair-core/src/test/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisherSpec.scala

pm47

🎉

Phew, that was something... but the end result is worth the effort!

Props for having gone the extra mile to facilitate the work of the reviewer. 👍

t-bast · 2022-01-18T17:49:26Z

pm47 · 2022-01-18T17:54:58Z

Me after a week-long deep dive

This feature is now ready to be widely deployed. The transaction format has been tested for a long time (between Phoenix iOS and our node) and automatic fee-bumping has been implemented in #2113.

t-bast added the Optech Make Me Famous! label Dec 23, 2021

t-bast force-pushed the tx-publisher-deadline branch from d24446e to 795daf7 Compare December 23, 2021 16:59

t-bast mentioned this pull request Jan 3, 2022

Make ClaimHtlcTx replaceable #2102

Merged

t-bast force-pushed the tx-publisher-deadline branch 3 times, most recently from be83b8c to af721ba Compare January 6, 2022 12:49

t-bast added 4 commits January 7, 2022 17:18

Add deadline information to tx publication

3b1537e

When a channel is force-closed, we tell the `TxPublisher` the block before which the transaction should be confirmed. Then it will be up to the `TxPublisher` to set the feerate appropriately and fee-bump whenever it becomes necessary.

t-bast force-pushed the tx-publisher-deadline branch from af721ba to 27d989a Compare January 7, 2022 16:18

t-bast marked this pull request as ready for review January 7, 2022 16:48

t-bast requested a review from pm47 January 7, 2022 16:48

t-bast force-pushed the tx-publisher-deadline branch from 27d989a to d1209ad Compare January 10, 2022 10:17

pm47 reviewed Jan 11, 2022

View reviewed changes

Fix review comments for 3b1537e

b066753

* Add a BlockHeight type * Rename deadline to confirmation target * Add comments on HtlcTx type inconsistency

pm47 reviewed Jan 11, 2022

View reviewed changes

Fix review comments for 9bfdd90

3cd7b83

* Remove toString on BlockHeight * Add confirmBefore to ReplaceableTransactionWithInputInfo * Add new codecs to add confirmation target to txs

pm47 reviewed Jan 12, 2022

View reviewed changes

t-bast added 2 commits January 13, 2022 09:32

fixup! Fix review comments for 9bfdd90

29b5039

fixup! fixup! Fix review comments for 9bfdd90

0a554f7

pm47 reviewed Jan 13, 2022

View reviewed changes

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/ReplaceableTxPublisher.scala Outdated Show resolved Hide resolved

This was referenced Jan 13, 2022

Use the BlockHeight type wherever it makes sense #2129

Merged

Clarify tx publisher context and logging #2131

Merged

pm47 reviewed Jan 13, 2022

View reviewed changes

Add more fields to actors private class

fa3e17d

This avoids passing around fields that are unchanged and repeating a long list of arguments in each behavior.

t-bast added 2 commits January 14, 2022 13:05

fixup! Add more fields to actors private class

441a8ee

Fix first pass review comments for d1209ad

8d13b91

* Protect against herd effect on new blocks * MempoolTxMonitor broadcasts tx status at every block

pm47 reviewed Jan 14, 2022

View reviewed changes

t-bast added 3 commits January 17, 2022 09:42

Remove explicit stopping of leaf actors

25fa5e6

We only need a Stop message at the `FinalTxPublisher` and `ReplaceableTxPublisher` level, their child actors will be automatically stopped whenever these parent actors stop themselves.

Remove intermediate CheckFee message

6d1b34e

Harmonize sendResult

315b0a6

pm47 reviewed Jan 17, 2022

View reviewed changes

eclair-core/src/test/resources/logback-test.xml Show resolved Hide resolved

eclair-core/src/main/scala/fr/acinq/eclair/channel/publish/TxPublisher.scala Show resolved Hide resolved

t-bast added 3 commits January 17, 2022 17:57

ReplaceableTxPublisher update confirmation target

4e635de

Instead of letting the TxPublisher create a new, competing replaceable attempt, we can simply tell the existing one to update its confirmation target.

Clarify comment about tx fee

0bfeb53

fixup! ReplaceableTxPublisher update confirmation target

63b7cde

pm47 reviewed Jan 18, 2022

View reviewed changes

t-bast added 4 commits January 18, 2022 13:59

Create case class to hold publish attempts

7e863ba

Handle funding tx not found

e389af6

Merge branch 'master' into tx-publisher-deadline

7e59cf6

Test the PublishAttempts case class

d76ae31

pm47 reviewed Jan 18, 2022

View reviewed changes

t-bast added 2 commits January 18, 2022 18:19

Improve tests

67e0d96

fixup! Improve tests

8b54837

pm47 approved these changes Jan 18, 2022

View reviewed changes

t-bast merged commit 40f7ff4 into master Jan 18, 2022

t-bast deleted the tx-publisher-deadline branch January 18, 2022 17:50

t-bast mentioned this pull request Jan 19, 2022

Activate anchor outputs #2134

Merged

This was referenced Jan 20, 2022

Add random speed up to replaceable tx publishing #2137

Closed

More aggressive confirmation target when low on utxos #2141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaceable txs fee bumping #2113

Replaceable txs fee bumping #2113

t-bast commented Dec 21, 2021 •

edited

Loading

codecov-commenter commented Dec 21, 2021 •

edited

Loading

t-bast commented Jan 10, 2022

pm47 commented Jan 10, 2022

pm47 left a comment

pm47 left a comment

pm47 left a comment

pm47 left a comment

pm47 left a comment

t-bast commented Jan 18, 2022

pm47 commented Jan 18, 2022

Replaceable txs fee bumping #2113

Replaceable txs fee bumping #2113

Conversation

t-bast commented Dec 21, 2021 • edited Loading

codecov-commenter commented Dec 21, 2021 • edited Loading

Codecov Report

t-bast commented Jan 10, 2022

pm47 commented Jan 10, 2022

pm47 left a comment

Choose a reason for hiding this comment

pm47 left a comment

Choose a reason for hiding this comment

pm47 left a comment

Choose a reason for hiding this comment

pm47 left a comment

Choose a reason for hiding this comment

pm47 left a comment

Choose a reason for hiding this comment

t-bast commented Jan 18, 2022

pm47 commented Jan 18, 2022

t-bast commented Dec 21, 2021 •

edited

Loading

codecov-commenter commented Dec 21, 2021 •

edited

Loading