Skip to content

Conversation

@t-bast
Copy link
Member

@t-bast t-bast commented Jun 3, 2025

When a channel was force-closed, we previously stored partially created closing transactions for each output of the commitment transaction. In some cases those transactions didn't include any on-chain fee (HTLC txs
with 0-fee anchors) and weren't signed. The fee is set in the publisher actors which then sign those transactions.

This was an issue because we used those partial transactions as if they were the final transaction that would eventually confirm: this made it look like RBF wasn't an option to callers, and was thus easy to misuse.

We now change our data model to only store the outputs of the commit tx that we may spend, without any actual spending transaction. We only store spending transactions once they are confirmed, at which point they can safely be used as closing transactions by callers such as our balance checks.

This lets us get rid of some codec migration code related to closing transactions, and is more future-proof because we now don't need to encode closing transactions and can thus change their format easily. This also reduces the size of our channel state during force-close.

When we restart, we re-compute pending closing transactions, which means we potentially RBF transactions claiming our main output, which is a good thing. In the longer term, once we stop supporting non-anchor commitment formats, we should probably re-work the publisher actors to always automatically RBF those transactions.

⚠️ There is one unwanted noticeable change for recovery close: we never stored the remote per-commitment point, which means we aren't able to RBF our main transaction if necessary. Is this something we'd like to support?

❗ Reviewers should start reviewing the changes made to ChannelData.scala: that's the main change and goal of this PR. Then another important change is in Channel.scala when restoring a channel (reconnection) where we re-compute 2nd-stage and 3rd-stage transactions. The rest of the PR is just fixing existing code to work with the new format.

@t-bast t-bast force-pushed the refactor-commit-published branch from caf8877 to d8da778 Compare June 3, 2025 09:51
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 92.13974% with 18 lines in your changes missing coverage. Please review.

Project coverage is 86.06%. Comparing base (f14b92d) to head (b48ace3).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
...wire/internal/channel/version2/ChannelTypes2.scala 65.38% 9 Missing ⚠️
...n/scala/fr/acinq/eclair/balance/CheckBalance.scala 55.55% 4 Missing ⚠️
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 92.68% 3 Missing ⚠️
...ire/internal/channel/version3/ChannelCodecs3.scala 60.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3097      +/-   ##
==========================================
+ Coverage   85.80%   86.06%   +0.26%     
==========================================
  Files         236      238       +2     
  Lines       21299    21548     +249     
  Branches      859      849      -10     
==========================================
+ Hits        18275    18546     +271     
+ Misses       3024     3002      -22     
Files with missing lines Coverage Δ
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 92.16% <100.00%> (+0.02%) ⬆️
...r/acinq/eclair/blockchain/fee/OnChainFeeConf.scala 100.00% <100.00%> (+4.34%) ⬆️
...in/scala/fr/acinq/eclair/channel/ChannelData.scala 100.00% <100.00%> (ø)
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 92.74% <100.00%> (+1.58%) ⬆️
...la/fr/acinq/eclair/channel/fsm/ErrorHandlers.scala 85.00% <100.00%> (+4.64%) ⬆️
...la/fr/acinq/eclair/transactions/Transactions.scala 95.99% <100.00%> (-0.10%) ⬇️
...wire/internal/channel/version0/ChannelTypes0.scala 97.89% <100.00%> (-2.11%) ⬇️
...ire/internal/channel/version2/ChannelCodecs2.scala 97.58% <100.00%> (+0.75%) ⬆️
...wire/internal/channel/version3/ChannelTypes3.scala 100.00% <ø> (+18.86%) ⬆️
...ire/internal/channel/version4/ChannelCodecs4.scala 98.71% <100.00%> (+0.04%) ⬆️
... and 4 more

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

When a channel was force-closed, we previously stored partially created
closing transactions for each output of the commitment transaction. In
some cases those transactions didn't include any on-chain fee (HTLC txs
with 0-fee anchors) and weren't signed. The fee is set in the publisher
actors which then sign those transactions.

This was an issue because we used those partial transactions as if they
were the final transaction that would eventually confirm: this made it
look like RBF wasn't an option to callers, and was thus easy to misuse.

We now change our data model to only store the outputs of the commit tx
that we may spend, without any actual spending transaction. We only
store spending transactions once they are confirmed, at which point they
can safely be used as closing transactions by callers such as our
balance checks.

This lets us get rid of some codec migration code related to closing
transactions, and is more future-proof because we now don't need to
encode closing transactions and can thus change their format easily.
This also reduces the size of our channel state during force-close.
@t-bast t-bast force-pushed the refactor-commit-published branch from d8da778 to 64b81e1 Compare June 3, 2025 16:46
@t-bast t-bast marked this pull request as ready for review June 3, 2025 16:46
@t-bast t-bast requested a review from sstone June 3, 2025 16:46
t-bast added 4 commits June 6, 2025 11:09
We add a `max-closing-feerate` config parameter to cap the feerate used
for closing transactions which aren't at risk of being double-spent.
Node operators should generally use a low value here, and update it when
they need to reclaim liquidity if needed.
Instead of introducing a `DirectedHtlcId`, we split incoming and
outgoing HTLCs into separate maps, which is more natural.
@t-bast t-bast requested review from pm47 and sstone June 12, 2025 08:37
@t-bast t-bast merged commit 52b7652 into master Jun 13, 2025
1 check passed
@t-bast t-bast deleted the refactor-commit-published branch June 13, 2025 06:41
t-bast added a commit that referenced this pull request Aug 19, 2025
We introduced a mechanism to limit the feerate used for non-urgent
force-close transactions in #3097. We now provide a way to update
this feerate on a per-channel basis, without restarting the eclair
node, by using the `forceclose` API.

Fixes #3108.
t-bast added a commit that referenced this pull request Aug 19, 2025
We introduced a mechanism to limit the feerate used for non-urgent
force-close transactions in #3097. We now provide a way to update
this feerate on a per-channel basis, without restarting the eclair
node, by using the `forceclose` API.

Fixes #3108.
t-bast added a commit that referenced this pull request Sep 5, 2025
We introduced a mechanism to limit the feerate used for non-urgent
force-close transactions in #3097. We now provide a way to update
this feerate on a per-channel basis, without restarting the eclair
node, by using the `forceclose` API.

Fixes #3108.
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.

5 participants