You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Improve taproot channels support and add tests (#3136)
* 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]>
caseclassMissingNonce(overridevalchannelId:ByteVector32, fundingTxId: TxId) extendsChannelException(channelId, s"next nonce for funding tx $fundingTxId is missing")
157
-
caseclassInvalidNonce(overridevalchannelId:ByteVector32, fundingTxId: TxId) extendsChannelException(channelId, s"next nonce for funding tx $fundingTxId is not valid")
caseclassMissingCommitNonce (overridevalchannelId:ByteVector32, fundingTxId: TxId, commitmentNumber: Long) extendsChannelException(channelId, s"commit nonce for funding tx $fundingTxId and commitmentNumber=$commitmentNumber is missing")
157
+
caseclassInvalidCommitNonce (overridevalchannelId:ByteVector32, fundingTxId: TxId, commitmentNumber: Long) extendsChannelException(channelId, s"commit nonce for funding tx $fundingTxId and commitmentNumber=$commitmentNumber is not valid")
158
+
caseclassMissingFundingNonce (overridevalchannelId:ByteVector32, fundingTxId: TxId) extendsChannelException(channelId, s"funding nonce for funding tx $fundingTxId is missing")
159
+
caseclassInvalidFundingNonce (overridevalchannelId:ByteVector32, fundingTxId: TxId) extendsChannelException(channelId, s"funding nonce for funding tx $fundingTxId is not valid")
160
+
caseclassMissingClosingNonce(overridevalchannelId:ByteVector32) extendsChannelException(channelId, "closing nonce is missing")
0 commit comments