Verify nonces on reconnection after we've pruned funding transactions #3138
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
…sactions 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 neeed to ignore when checking nonces sent in their `channel_reestablish` message.
No functional changes, but it was getting chunky and is now split into smaller helpers called by the main handler.
t-bast
approved these changes
Aug 18, 2025
t-bast
added a commit
that referenced
this pull request
Aug 18, 2025
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.
t-bast
added a commit
that referenced
this pull request
Aug 18, 2025
* Add more parameters to nonce generation 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. * Remove `ignoreRetransmittedCommitSig` 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. * Fix preimage extraction from remote HTLC-success txs 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. * Define signature functions without `extraUtxos` `CommitTx` and `ClosingTx` never have extra utxos, we can thus define a dedicated function overload instead of explicitly providing `Map.empty` in many places. * Refactor `channel_reestablish` local nonces creation 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`. * Clean-up nonces and partial sigs TLVs and codecs 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. * Fix `channel_ready` retransmission We were missing our local nonce when retransmitting `channel_ready` on reconnection for taproot channels. * Refactor closing helpers 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. * nit: rename Phoenix simple taproot feature The `tweaked` suffix doesn't seem to be useful, we're already stating that this is specific to Phoenix. * More detailed remote nonce exceptions 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`. * Simplify error handling 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. * Rename phoenix taproot commitment format and channel type 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. * Refactor and simplify interactive-tx for taproot 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 * Add interactive-tx taproot tests 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 * Add missing taproot channel tests 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. * Only generate local nonce if remote nonce is provided * Verify nonces on reconnection *after* we've pruned funding transactions (#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. * More refactoring of `channel_reestablish` 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. --------- Co-authored-by: Fabrice Drouin <[email protected]>
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.
When nodes get disconnected before they've exchanged
splice_locked, one of them may still count the old commitment as active and expect a nonce for it, while the other has already prune it.=> we use the pruned commitments (which will become the current commitment once we're done processing the incoming
channel_reestablishmessage) to verify incoming nonces against.The second commit is a refactoring of the
channel_reestablishhandler which had become fairly chunky, without functional changes.Third commit logs whether we accept/reject upgrades to taproot during splices.