Improvements and bug fixes to taproot channels #814
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
It was missing in several cases, and the `OpenChannel` command allowed using other channel types.
We refactor `Commitments.kt` to better match `eclair` and clean up pattern matching (use exhaustive, future-proof pattern matching). We also remove alternative feerate sigs, which aren't necessary anymore since we can now use package relay. The ACINQ node will stop sending `update_fee` and will keep the commitment feerate at `1 sat/byte`.
Error handling was missing from a few places (incorrectly assuming that nonces were already provided and signature couldn't fail). We also use exhaustive pattern matching everywhere, and clean-up the nonces and funding tx index parameters. We also revert some formatting nits, which make the file inconsistent.
The nonce management was incomplete for mutual close RBF (see changes to `Negotiating.kt`). We must add unit tests to catch those bugs. Mutual close was also not working properly because `revoke_and_ack` nonces weren't taken into account in the `ShuttingDown` state. We also refactor the nonce fields in the various channel states, in which some nonces where unnecessary (e.g. already contained in the `shutdown` message). We make the ordering of fields consistent: `remoteNextCommitNonces` always comes right after `commitments`, since it should always be filled (for taproot channels) and is necessary to update the `commitments`.
We switch almost all existing tests to use the taproot channel type. We fix a few bugs found by the existing test suite: - preimage extraction from HTLC transactions was missing - channel_ready didn't contain local commit nonces - channel_reestablish nonces had an off-by-one in the commit index
sstone
approved these changes
Oct 6, 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 PR contains all the changes made during my review of #805: it's better reviewed commit by commit (check the commit messages for details).
I have reviewed all the code but not the tests yet, we need to spend some time on testing now but otherwise the code is looking good!
Builds on top of #805