electrum icon indicating copy to clipboard operation
electrum copied to clipboard

Anchor commitments, new payment basepoint

Open bitromortac opened this issue 4 years ago • 12 comments

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:

  • [x] Crosscheck RFC
  • [x] Implement zero fee HTLCs and do fee management at sweep time
  • [ ] Reserve some UTXOs for fee bumping
  • [x] Compatibility testing with zero-fee-htlc (once zero-fee is implemented)
  • [x] Wallet upgrading mechanics, compatibilty wallets with old/new channels
  • [x] Keep tests for both commitment versions (static_remotekey/anchors)?
  • [x] Make commitment type configurable?
  • [x] 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: https://github.com/lightningnetwork/lightning-rfc/pull/688 [2] https://lists.linuxfoundation.org/pipermail/lightning-dev/2020-September/002796.html [3] Zero-fee HLTCs: https://github.com/lightningnetwork/lightning-rfc/pull/824 [4] Channel types: https://github.com/lightningnetwork/lightning-rfc/pull/880

bitromortac avatar Sep 23 '21 08:09 bitromortac

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

bitromortac avatar Sep 27 '21 12:09 bitromortac

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.

SomberNight avatar Sep 28 '21 15:09 SomberNight

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 avatar Oct 04 '21 10:10 bitromortac

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 avatar Oct 12 '21 13:10 bitromortac

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

bitromortac avatar Oct 15 '21 10:10 bitromortac

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.

bitromortac avatar Nov 10 '21 12:11 bitromortac

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)

bitromortac avatar Nov 18 '21 13:11 bitromortac

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 avatar Mar 09 '22 17:03 ecdsa

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 avatar Sep 26 '22 07:09 ecdsa

new rebase: https://github.com/spesmilo/electrum/commits/anchor_commitments_2024

ecdsa avatar Jun 09 '24 09:06 ecdsa

@ecdsa Could you make a separate PR for your rebase? I cannot usefully add comments to it without that. (note to self: lnchannel.sweep_address looks broken in the rebase)

SomberNight avatar Oct 20 '24 10:10 SomberNight

opened PR #9264

ecdsa avatar Oct 20 '24 13:10 ecdsa

commits were reworked and merged in #9264

ecdsa avatar Nov 26 '24 09:11 ecdsa