Anchor commitments 2024 #9264
Draft
Anchor commitments 2024 #9264
+1,472
−353
Conversation
This file contains 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
* add anchor ln features * peer.use_anchors is added * channel.has_anchors is added
* in order to be able to sweep to_remote in an onchain backup scenario we need to retain the private key for the payment_basepoint * to facilitate the above, we open a channel derived from a static secret (tied to the wallet seed), the static_payment_key combined with the funding pubkey (multisig_key), which we can restore from the channel closing transaction
* changes the htlc outputs' witness script to have a csv lock of 1 * send signatures for remote ctx with ANYONECANPAY|SINGLE * refactor htlc weight (useful for zero-fee-htlc)
* to_remote has now an additional csv lock of 1 * anchor outputs are added if to_local/remote outputs are present * funder balance is reduced to accomodate anchors
* sweep to_remote output, as this is now a p2wsh (previously internal wallet address) * sweep htlc outputs with new scripts
* add a method for backups to sweep to_remote * to_remote sweeping needs the payment_basepoint's private key to sign the sweep transaction * we restore the private key from our funding multisig pubkey (pubished with the closing transaction) and a static payment key secret * lower the final balance of the backup regtest, which is due to additional sweep transactions
* testing of anchor channels is controlled via TEST_ANCHOR_CHANNELS * rewrite tests in test_lnchannel.py
* tests are kept variable via TEST_ANCHOR_CHANNELS
* sets the weight of htlc transactions to zero, thereby putting a zero fee for the htlc transactions * add inputs to htlc-tx for fee bumping * switches feature flags * disable anchor test vectors, which are now partially invalid
* in test_lnutil, patch htlc weight to pass original anchor commitment test vectors * activate tests for both commitment types
naming scheme: tx(s)_our/their_ctx/htlctx_output-description function names are shortened to whether a single (tx) or several sweep transactions (txs) are generated
Due to anchor channel's sighash.SINGLE and sighash.ANYONECANPAY, several HTLC-transactions can be combined. This means we must watch for revoked outputs in the HTLC transaction not only at index 0 but at any index. Also, any input can now contain preimages which we have to extract.
Due to malleability of HTLC-transactions, we can't send presigned justice transactions for the second-stage HTLC transactions, which is why we now send first-stage justice transactions for anchor channels.
8 tasks
SomberNight
reviewed
Oct 20, 2024
| # generate justice transactions | ||
| gen_tx = lambda: tx_sweep_htlctx_output( | ||
| sweep_address=sweep_address, | ||
| output_idx=output_idx, |
There was a problem hiding this comment.
Oh wow, amazing that the linter caught this!
./electrum/lnsweep.py:244:24: B023 Function definition does not bind loop variable 'output_idx'.
output_idx=output_idx,
^
1 B023 Function definition does not bind loop variable 'output_idx'.
Comment on lines
868
to
875
| addr = None | ||
| assert self.is_static_remotekey_enabled() | ||
| our_payment_pubkey = self.config[LOCAL].payment_basepoint.pubkey | ||
| addr = make_commitment_output_to_remote_address(our_payment_pubkey) | ||
| if self.lnworker: | ||
| assert self.lnworker.wallet.is_mine(addr) | ||
| addr = make_commitment_output_to_remote_address(our_payment_pubkey, has_anchors=self.has_anchors()) | ||
| # this assert fails with anchor output channels | ||
| #if self.lnworker: | ||
| # assert self.lnworker.wallet.is_mine(addr) | ||
| return addr |
There was a problem hiding this comment.
Something weird happened in a rebase here.
The original branch looks like this: https://github.com/bitromortac/electrum/blob/5cb97c077c799aa6ebd06492645b9c4f738f85f8/electrum/lnchannel.py#L325
which looks okay.
However this new implementation is not good. There is a reason that assert is there, and it failing signals a problem.
The sweep_address() method is supposed to return a wallet address that we can send coins to as part of closing this channel. It is called from a lot of places, e.g. code that is trying to sweep HTLCs will send coins here, or even the mutual close will send our coins here. Clearly we don't want the mutual close to send coins to a CSV-locked non-wallet address :)
I've tried to clarify this method in dd140df
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 was rebased in july 2024 from #7509
I guess it needs to be rebased again :-)
Here are my personal notes regarding this rebase:
REDEEMEDstate reached too early in test_breach)unlockcommand)