Skip to content

Make ClaimHtlcTx replaceable#2102

Merged
t-bast merged 2 commits intomasterfrom
replaceable-claim-htlc-txs
Jan 3, 2022
Merged

Make ClaimHtlcTx replaceable#2102
t-bast merged 2 commits intomasterfrom
replaceable-claim-htlc-txs

Conversation

@t-bast
Copy link
Member

@t-bast t-bast commented Dec 14, 2021

We add claim-htlc transactions to the set of transactions that we will RBF if they don't confirm quickly.

This is important because the feerate is evaluated when we see the remote commitment in the mempool, so it may be outdated by the time we can actually publish the claim-htlc transaction (either because of the absolute timelock or because the remote commitment took a long time to confirm).

It's very simple to set the fees for these transactions: since we can unilaterally sign, we simply decrease the output amount and re-sign. Note that we do it for both standard commitments and anchor output commitments. For standard commitments, these transactions are marked as non-RBFable, so RBF attempts will simply fail if the previous transaction is in the mempool. But it's still worth attempting it, as it may have been dropped from the mempool in high-fee environments.

The ReplaceableTxPublisher deserves some refactoring: integrating these various types of transactions shows where the model works and where it doesn't. But I've got a few other changes I'd like to make which impact the architecture, so I'd like to keep it like this for now and refactor after a few more PRs.

@t-bast t-bast force-pushed the replaceable-claim-htlc-txs branch from a153d80 to a002cce Compare December 14, 2021 08:55
@t-bast t-bast marked this pull request as ready for review December 14, 2021 10:52
@t-bast t-bast requested a review from pm47 December 14, 2021 13:38
@t-bast t-bast force-pushed the replaceable-claim-htlc-txs branch from a002cce to 7ad4420 Compare December 15, 2021 08:18
Add the replaceable trait to ClaimHtlcTx and ensure the channel sends the
corresponding command to the TxPublisher.
These transactions are much simpler than HTLC transactions: we don't add
new inputs, we just decrease the output amount and we can unilaterally sign
the updated transaction.
@t-bast t-bast force-pushed the replaceable-claim-htlc-txs branch from 7ad4420 to 3d0761b Compare December 16, 2021 09:25
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how easy this change is! Shows that our model is good.

The ReplaceableTxPublisher deserves some refactoring

A few minor remarks:

@t-bast
Copy link
Member Author

t-bast commented Jan 3, 2022

How about renaming RawTxPublisher to FinalTxPublisher?

Good idea, it's a better distinction with ReplaceableTxPublisher.
Even though I have in the back of my mind ideas for a future where such "final" txs can be CPFP-ed through their change output (e.g. for funding transactions), but whether it's Raw or Final doesn't matter for that.

I'll do this in a follow-up PR.

Wonder if this precheck should be done when starting the RawTxPublisher actor, which would then stop right away

We can't do that for the ReplaceableTxPublisher, because the decision is based on the map kept by the TxPublisher.
It's true that we could do it for the RawTxPublisher, but I don't like creating this asymmetry, I think it's better to do it at the same place for both types of publishers (so it has to be in the TxPublisher).

This log should make it clear that the existing tx has a higher feerate and that's why we don't replace it

In the next PR (#2113, not yet completely ready for review) I change this to use a deadline instead of an explicit feerate, so I'll fix it there.

@t-bast t-bast merged commit 2827be8 into master Jan 3, 2022
@t-bast t-bast deleted the replaceable-claim-htlc-txs branch January 3, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants