Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anchor commitments, new payment basepoint #7509

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

bitromortac
Copy link
Contributor

@bitromortac bitromortac commented Sep 23, 2021

This PR implements a newer commitment transaction type [1]. Anchor commitments are commitment transactions that attach a dusty output per realized to_local/to_remote output, in order for the commitment transaction to be CPFP-bumpable in a force close scenario. Also HTLC transactions are affected, which are fee-amendable allowing for more inputs via the anyonecanpay sighash. The original proposal of anchor commitments [1] was not safe enough [2] which is why option_anchors_zero_fee_htlc_tx was devised [3]. Our goal is to go directly to this version, to not having to deal with legacy developments. The PR passes unit and regtest tests and is ready for initial review or feedback (see Todos).

Key changes:

  • The Channel object now carries a has_anchors property which is used to decide on scripts and locktimes, as well as sweeping behavoir.
  • Adapt commitment transactions for anchors (anchors, to_remote script 1-CSV, fee estimation, output values) (03-transactions)
  • Adapt HTLC transactions (nsequence numbers, 1 CSV-lock, sighash.ANYONECANPAY+sighash.SINGLE) (03-transactions)
  • New anchor channels have a payment_basepoint derived from the funding pubkey and a static secret to be sweepable in the event of a force close due to (onchain) backups

Testing:

  • Reproduce signatures from RFC test vectors for commitment and HTLC transactions (requires different sighash flags) (03-transactions)
  • Manually tested against lnd v0.11 (last active version of original anchors) - open/close/force close/payment
  • New integration test framework to run Electrum peer components inside a single interpreter (better debugging and easier introspection), with additional detailed channel usage scenarios. Tests are not running in sequence right now and are still subject to changes. Some tests are redundant and could be removed.

Other:

  • The watchtower is not updated to sweep to_remote as this is not time critical
  • Backups don't need an upgrade as we can see at sweep time, whether we deal with a ctx that has anchors

Todo:

  • Crosscheck RFC
  • Implement zero fee HTLCs and do fee management at sweep time
  • Reserve some UTXOs for fee bumping
  • Compatibility testing with zero-fee-htlc (once zero-fee is implemented)
  • Wallet upgrading mechanics, compatibilty wallets with old/new channels
  • Keep tests for both commitment versions (static_remotekey/anchors)?
  • Make commitment type configurable?
  • Determine fee for HTLC-transaction bumping
  • Sweep the anchor outputs (05-onchain) (omitted for this PR, because we still rely on the update_fee message)
  • Refactor to have explicit commitment and channel types [4] (another PR)

[1] Anchors PR: lightning/bolts#688
[2] https://lists.linuxfoundation.org/pipermail/lightning-dev/2020-September/002796.html
[3] Zero-fee HLTCs: lightning/bolts#824
[4] Channel types: lightning/bolts#880

Copy link
Member

@SomberNight SomberNight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So many changes :P but looks good overall.
See comments inline.

electrum/lnchannel.py Outdated Show resolved Hide resolved
@@ -502,6 +502,10 @@ def is_static_remotekey(self):
def is_upfront_shutdown_script(self):
return self.features.supports(LnFeatures.OPTION_UPFRONT_SHUTDOWN_SCRIPT_OPT)

def use_anchors(self) -> bool:
return self.features.supports(LnFeatures.OPTION_ANCHOR_OUTPUTS_OPT) \
and self.lnworker.features.supports(LnFeatures.OPTION_ANCHOR_OUTPUTS_OPT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it not enough to check self.features; why check self.lnworker.features?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, here I wanted to ask how we want to transition to anchors? The second condition was also a reminder for myself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well there are three ~reasonable options I can see:

  1. we require anchors_zero_fee for newly opened channels
  2. we open anchors_zero_fee channels whenever possible, but if the remote does not support it, we can just option static_remotekey channels
  3. we disable anchors_zero_fee by default, gate it with a config key; and change to one of the options above in a later release

I think the decision should depend on a number of things, such as

  • how many/which nodes on the network will already support anchors_zero_fee,
  • how confident we are that our code behaves correctly,
  • how the UX of reserving UTXOs in the wallet looks like,
  • whether we can batch HTLC txs (not really for fee-minimisation but rather re number of UTXOs we need to reserve)

So atm I am not sure.

electrum/tests/test_lnutil.py Outdated Show resolved Hide resolved
electrum/tests/test_lnutil.py Outdated Show resolved Hide resolved
electrum/lnutil.py Show resolved Hide resolved
electrum/lnsweep.py Outdated Show resolved Hide resolved
electrum/lnsweep.py Outdated Show resolved Hide resolved
electrum/lnutil.py Outdated Show resolved Hide resolved
electrum/lnsweep.py Outdated Show resolved Hide resolved
@bitromortac
Copy link
Contributor Author

I submitted changes via fixup commits, will squash in the end, or should I already?

@SomberNight
Copy link
Member

SomberNight commented Sep 28, 2021

I submitted changes via fixup commits, will squash in the end, or should I already?

Feel free to do whatever is convenient.
One remark: if you rebase on master for any reason, it's better to do that in a separate force-push of its own just for the rebase (instead of combining with other changes that would require a force-push). That way, it's easier to see changes in the github UI.

electrum/lnsweep.py Outdated Show resolved Hide resolved
@bitromortac bitromortac force-pushed the 2109-anchor-commitments-backup branch 2 times, most recently from 03b789b to ee2a5d7 Compare October 4, 2021 10:38
@bitromortac
Copy link
Contributor Author

I submitted changes via fixup commits, will squash in the end, or should I already?

Feel free to do whatever is convenient. One remark: if you rebase on master for any reason, it's better to do that in a separate force-push of its own just for the rebase (instead of combining with other changes that would require a force-push). That way, it's easier to see changes in the github UI.

I squashed the fixups and rebased on master separately.

There's a new commit, which switches to the anchors-zero-htlc-fee type. I have tested this together with the latest LND master. The only issue for now is how to deal with password protected wallets, where we are not able to fee-bump HTLCs automatically (or bump commitment transactions once we phase out update_fee).

@bitromortac
Copy link
Contributor Author

As discussed, for now we only enable anchor channels via a config setting (not exposed in the GUI), storing the wallet password in lnworker for dynamic signing. A wallet can only be loaded with anchor channels enabled, if it is not a watch-only wallet. Also, the anchors config option cannot be disabled, if a wallet already has anchor channels.

This way we can test more in production and we can explore further how we want to handle UTXO reservation.

@bitromortac bitromortac force-pushed the 2109-anchor-commitments-backup branch 2 times, most recently from 4ddbc84 to e70a918 Compare October 15, 2021 10:28
@bitromortac
Copy link
Contributor Author

bitromortac commented Oct 15, 2021

I reorganized the commits a bit and think this is ready for a second pass. Tests for anchor outputs are disabled right now (can be controlled via TEST_ANCHOR_CHANNELS), but they should all pass.

I think we could postpone the reservation of UTXOs for now, as we keep anchors disabled.

Edit: somehow the CI regtest hangs, but passes locally

electrum/gui/qt/installwizard.py Outdated Show resolved Hide resolved
electrum/lnworker.py Outdated Show resolved Hide resolved
electrum/tests/test_lnchannel.py Outdated Show resolved Hide resolved
@@ -2,6 +2,7 @@
export HOME=~
set -eu

TEST_ANCHOR_CHANNELS=False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for the unit tests: I think we should test by default for both possibilities.
One approach: some refactoring where most code of each test is inside a function that is parameterised with anchors_enabled, and the functions are called twice (one for each setting).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I'm setting an environment variable in subprocess.Popen. Not sure if updating of the environment dict is the best approach.

electrum/tests/integration.py Outdated Show resolved Hide resolved
electrum/lnsweep.py Outdated Show resolved Hide resolved
@bitromortac
Copy link
Contributor Author

After the recent update all tests passed, but I had to run the watchtower test twice (needs further investigation). I'm still working on the HTLC issue we discussed.

* 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
* anchor channels can be activated via `enable_anchor_channels` config
  option
* for anchor-enabled wallets, we store the wallet password, because UTXOs
need to be added to commitment or HTLC transactions dynamically
* testing of anchor channels is controlled via TEST_ANCHOR_CHANNELS
* rewrite tests in test_lnchannel.py
* tests are kept variable via TEST_ANCHOR_CHANNELS
* add funds to bob to be able to bump htlc transaction
* 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

* functinon names are shortened to whether a single (tx) or several sweep
  transactions (txs) are generated
* functions are ordered by their purpose (our/their ctx related)
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.
@bitromortac
Copy link
Contributor Author

After the recent update all tests passed, but I had to run the watchtower test twice (needs further investigation). I'm still working on the HTLC issue we discussed.

I tried to add all the edge cases concerning HTLC transaction batching. Do we need an explicit test for this? I also added a test for the watchtower in case of anchor outputs with an HTLC in the ctx (see integration.TestLightningABC.test_watchtower_htlc)

@ecdsa
Copy link
Member

ecdsa commented Mar 9, 2022

Note: I'm going to rebase this in a new branch. Since it is not possible to change the source branch of a PR, I guess this means that the conversation will have to be continued in a new PR

@ecdsa
Copy link
Member

ecdsa commented Sep 26, 2022

new branch, rebased on current master: https://github.com/spesmilo/electrum/commits/anchor_commitments_2022

changes:

  • the wallet password is not stored anymore. Instead, we will send funds to a new sequence of addresses, derived from ln_xprv.
  • the commit that was both renaming and reordering functions in lnsweep has been split into two commits, so that changes can actually be reviewed. (I checked that the reordering commit does only that)

@ecdsa
Copy link
Member

ecdsa commented Jun 9, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement LN anchor outputs
3 participants