Improve taproot channels support and add tests #3136
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We provide both funding keys to the nonce generation algorithm, which better matches the interface provided by libsecp256k1. We also provide the `funding_txid` to the signing nonce generation to improve the extra randomness used.
This isn't necessary since we now use `next_commitment_number` to indicate whether we want a retransmission of `commit_sig` or not on reconnection.
When extracting the preimage from potential HTLC-success transactions, we remove the unnecessary check on the signature size, which could be gamed by our peer if they sign with a different sighash than none.
`CommitTx` and `ClosingTx` never have extra utxos, we can thus define a dedicated function overload instead of explicitly providing `Map.empty` in many places.
The previous implementation didn't handle the dual-funding RBF case. We add support for this case and clean-up the local nonces TLVs created during `channel_reestablish`.
We add documentation to taproot TLVs and clean-up the codec files. We clean-up the constructors for various lightning messages and better type channel spending signatures (see `commit_sig` for example). We remove the redundant `NextLocalNonce` TLV from `revoke_and_ack`. We also rename closing nonces to be more clear, as local/remote only is confusing. We add codecs tests, as the PR didn't contain any encoding test for the new TLVs. Writing those tests allowed us to discover a bug in the encoding of `txId`s, which must be encoded in reverse endianness.
We were missing our local nonce when retransmitting `channel_ready` on reconnection for taproot channels.
We refactor the taproot closing helpers and fix a bug where the `closer` TLV was filled instead of using the `closee` TLV (when signing remote transactions in `signSimpleClosingTx`). The diff with the base branch may look complex, but it is actually to make the diff with `master` much smaller and easier to review, by grouping logic that applies to both taproot and non-taproot cases. We also move the `shutdown` creation helpers to a separate file, like what is done for the `channel_ready` creation helper.
The `tweaked` suffix doesn't seem to be useful, we're already stating that this is specific to Phoenix.
We rename the exceptions on remote nonces to more clearly apply to the commit tx, funding tx or closing tx. We add more fields to help debug. We remove the unnecessary log line on setting the remote commit nonces (the messages are logged and contain the nonces TLV, which is enough for debugging). We clear nonces on disconnection, to avoid polluting a fresh connection with invalid data from the previous connection. We fix a few `channel_reestablish` bugs, where the *next* commit nonce was used instead of the *current* commit nonce when retransmitting our `commit_sig`.
The previous code was calling `handleLocalError` in `spendLocalCurrent` which created an infinite loop, since `handleLocalError` was itself calling `spendLocalCurrent` to publish the commit tx. This was clearly untested, otherwise this issue would have been noticed earlier. More tests will be added in future commits to address this issue. We can actually greatly simplify error handling during force-close by making `fullySignedCommitTx` ignore errors, which is safe since we only use this *after* receiving the remote `commit_sig`, which we validate (we force-close with the previous commitment if the remote nonce or partial sig is invalid). We improve pattern matching to fully match on the commitment format, which is important to ensure that when adding v3 taproot channels we automatically get a compiler error to give us the opportunity to set nonces correctly, otherwise we will silently miss some code paths. We also fix the `Shutdown` state, where the remote commit nonces were not provided to the `sendCommit` method, which means we couldn't settle HTLCs for taproot channels after exchanging `shutdown`. This needs unit tests as well, which will be added in future commits.
We're tweaking the official taproot channel type for Phoenix by using HTLC transactions that have the same feerate as the commit tx. This will only be support for Phoenix users, so we rename those cases to explicitly mention that this is for Phoenix. We also more explicitly only support upgrades of the commitment format to this specific commitment format during a splice: this isn't a BOLT mechanism so we can simply ignore malicious nodes who try to update to something different. We can change this in the future when this becomes a standard mechanism.
We refactor the `InteractiveTxBuilder` to: - leverage existing signing helpers (from `Transactions.SpliceTx`) - revert unnecessary changes (`receiveInput()`) - add more documentation around `tx_complete` nonce management - add early nonce validation (in `validateTx`) - clean-up error handling
The existing code duplicated the interactive-tx test suite for taproot. This has many drawbacks: - it runs a lot of tests twice for exactly the same codepaths - it spins up another `bitcoind` instance, which is expensive - it doesn't test the taproot-related stuff in enough depth - it doesn't test taproot with `eclair-signer = true` We improve this by reverting these test changes and instead: - changing some of the tests to use taproot - duplicating a couple of tests that have different behavior in taproot and non-taproot cases - adding a test for the commitment upgrade during a splice
The base branch simply duplicated some test suites for taproot without any changes: this gave a false sense of safety, as it actually simply ran twice a lot of tests that exercised the same code paths that didn't have anything related to taproot. It also made the test suite slower, since the duplicated tests were some of our slowest tests. We revert this test suite duplication and instead add tests specific to taproot channels. This showed that there were a lot of bugs in subtle edge cases that weren't discovered previously: - simple close RBF didn't work at all (nonce management was missing) - shutting down a taproot channels that had pending HTLCs didn't work at all either (nonce management wasn't handled) - reconnecting a channel during mutual close lead to a force-close - reconnection with partially signed interactive-tx session wasn't properly checking the next commit nonce for the partially signed tx We also correctly test all scenarios of revoked commitments during and after a commitment upgrade to taproot. Thankfully, this scenario didn't require any code changes, since a revoked commitment that doesn't spend the latest funding transaction must be kept in either our `active` or `inactive` commitments list, which means we have access to the format and parameters of this specific commitment and don't need to brute-force various commitment formats.
sstone
reviewed
Aug 12, 2025
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenSingleFunded.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenSingleFunded.scala
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/Channel.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala
Outdated
Show resolved
Hide resolved
…ns (#3138) If we have not received their funding locked, we may have active commitments that our peer has already pruned and will not send a nonce for, and which we need to ignore when checking nonces sent in their `channel_reestablish` message.
We iterate on the refactoring from #3138 to further simplify the channel reestablish handler: we more explicitly return a single optional message from helper functions when possible, and move comments back to the handler instead of the helper functions.
sstone
approved these changes
Aug 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a PR that targets #3103.
I know that this PR contains a lot of commits, but that's on purpose: I spent a lot of time ensuring most of the commits are well-scoped, trivial to review, have a good commit message explaining what is done and the motivation for the change. Please review commit-by-commit and read the commit messages.
By spending such a large amount of time on the taproot changes and improving the tests, I've found a lot of subtle bugs: overall, the "vanilla" scenario works fine with just #3103, but many many edge cases or scenarios that slightly differ from the ideal scenario wasn't working...more details about those bugs can be found in each commit message.
My goal is that we go through the following steps:
CommitParamsto individual commitments #3118 inmaster(it would be useful to have a review from @pm47)masteras well: since there is no way to activate the feature yet (unless we modify the code), this should be safe to have onmaster, and will make it much easier to iterate on small fixes/improvements until we have cross-compatibility withlnd(it would be a big waste of time to have to handle rebasing with changes onmaster)I think we should be able to get those done quite soon, which will be a big step forward! After that, I see two important follow-ups that must be done before officially shipping the feature:
prevtxfield ininteractive-txwhen splicing taproot channels, or when there is a taproot input (such as the swap-in-potentiam input for Phoenix): this will finally get rid of the issue where utxos coming from large transactions (such as coinjoins) cannot be used