Implement option_static_remotekey

[araspitzu]

Add support for the upcoming feature option_static_remotekey lightning/bolts#642

[araspitzu]

Commit a1934f5 introduces a new ChannelVersion to be able to "tag" the channel instance with STATIC_REMOTEKEY version. This was necessary because on reconnection we always overwrite local features but option_static_remotekey is sticky and if negotiated during channel opening it applies to all commitment transactions.

[pm47]

@sstone There is a new ChannelVersion in this PR

[araspitzu]

For those interested in testing i'm running 12bf3ec on a testnet node at 021eace881c1080ff4238e7e8b89aaac171c229f6e3e6650e21135cb4f3b398658@172.81.180.147:9737

[araspitzu]

At the current status the PR implements option_static_remotekey using the wallet keys in order to avoid doing a claim transaction for our main output, the key associated to our output is stored in the LocalParam for convenience during commitment computation. The option is sticky and once negotiated during channel opening it stays enabled (thus the new ChannelVersion bit), it also honors the option_data_loss_protect fields when reestablishing a connection to the remote peer. Feature support is enabled by default.

[t-bast]

Nice, this is shaping up!
Have you been able to test compatibility with LND and CL? I think they both have it on their master branch.

[araspitzu]

Nice, this is shaping up!
Have you been able to test compatibility with LND and CL? I think they both have it on their master branch.

I've done some testing with @rustyrussel's node in testnet, i will repeat them with LND now that is in master branch.

[t-bast]

I've done some testing with @rustyrussel's node in testnet, i will repeat them with LND now that is in master branch.

I think especially the channel reestablish case needs to be tested. From what I understood LND may be weird there, they requested us to add to the spec the requirement to keep sending a dummy but valid point in my_last_per_commitment_point so we should check they accept what we sent them.

[araspitzu]

I think especially the channel reestablish case needs to be tested. From what I understood LND may be weird there, they requested us to add to the spec the requirement to keep sending a dummy but valid point in my_last_per_commitment_point so we should check they accept what we sent them.

Are you referring to this comment? I purposely left the DLP fields in place in the ChannelReestablish.

[t-bast]

Exactly. By looking at the code it seems like this should work, but nothing beats a real E2E test ;)

[t-bast]

LGTM but I don't know enough of the channel management part of our codebase to safely sign-off, I'll let others chime in.

[sstone]

This should be a getFinalPubkey or getReceivePubkey without an address parameter otherwise it won't be consistent with the Electrum wallet.

[araspitzu]

I prefer getReceivePubkey as it tells that it's the receiving key/address of the wallet, i'll update getReceiveAddress too.

[araspitzu]

Commit 58a5da0 uses option_static_remotekey from global features, see lightning/bolts#680

[codecov-io]

Codecov Report

Merging #1141 into master will increase coverage by 0.78%.
The diff coverage is 95.18%.

@@            Coverage Diff             @@
##           master    #1141      +/-   ##
==========================================
+ Coverage    76.6%   77.38%   +0.78%     
==========================================
  Files         140      140              
  Lines        9700    10101     +401     
  Branches      397      401       +4     
==========================================
+ Hits         7431     7817     +386     
- Misses       2269     2284      +15

Impacted Files	Coverage Δ
...nq/eclair/blockchain/electrum/ElectrumWallet.scala	80.79% <0%> (+0.04%)	⬆️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	55.95% <0%> (ø)	⬆️
...air/blockchain/electrum/ElectrumEclairWallet.scala	0% <0%> (ø)	⬆️
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala	90.9% <100%> (+2.02%)	⬆️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	85.5% <100%> (+2.19%)	⬆️
...ain/scala/fr/acinq/eclair/wire/ChannelCodecs.scala	100% <100%> (ø)	⬆️
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala	94.77% <100%> (+0.18%)	⬆️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala	77.09% <100%> (+1.87%)	⬆️
...eclair/blockchain/bitcoind/BitcoinCoreWallet.scala	89.83% <100%> (+0.54%)	⬆️
...core/src/main/scala/fr/acinq/eclair/Features.scala	100% <100%> (ø)	⬆️
... and 18 more

[araspitzu]

After some offline discussion i've revived and simplified this PR 🎉 so here is a gist of the changes, their rationale and why we should merge them.

Option option_static_remotekey simplifies the commitment format by not rotating the key for the main output, to fully take advantage of this we want to have our main output spendable by the BIP32 wallet (bitcoin-core in our case) so we ask bitcoin-core to generate an address and get us the raw pubkey for it. Note that we don't need to do the claim-main-output-tx anymore.

Supporting this new option requires us to remember what features that were negotiated during channel opening (see comment) to achieve so we add a new ChannelVersion bit that tells us if this channel uses option_static_remotekey. The alternative was to store the byte vector of the features but it would be more impactful on the codecs and we already have the ChannelVersion so it comes natural to use that.

There is a new field in the LocalParams codecs this is necessary to remember the payment basepoint (our static key) that was used in the channel. There are some alternatives here:

• query bitcoin-core for the raw key given our toLocal address
• store the open_channel message that we sent during channel creation

Both options are more impactful on the code or end up doing more changes to the codecs.

We now carry the ChannelVersion down to Commitments and Helpers in order to make decisions on how to derive the local payment basepoint.

Note that those changes are also preparation work to support more commitment formats in the future, i expect that we add new ChannelVersions and use that to support the new formats (i.e dual-anchor). Because there are now multiple commitment formats in the network we might end up in a situation where some nodes reject eclair's connections because we don't support a certain commitment format.

[t-bast]

Thanks, I agree that the code is actually quite simple now (the tests take much more work than the actual code, but I think that's expected since we want to replicate many existing tests to the static_remotekey case).

I think we should go forward with this. We've seen users losing backups (even very recently) and it takes a lot of time and effort to get their funds back. With this change it's really simple and we're also not "wasting" a sweep transaction (which is nice too in terms of fees). Once my comments are addressed, I think we also need @sstone to review (and we'll do interop tests with other implementations). I think it shouldn't take too long to finalize and merge this so it's worth doing. WDYT @sstone?

[t-bast]

Great, only a couple nit comments remaining. From the code's point of view it looks good to me, I'll need to spend time doing interop tests though.

[t-bast]

LGTM, just waiting for another review and interop tests after that and we should be good to go!

[araspitzu]

Rebased on master

[araspitzu]

I've done some E2E tests with other implementations and I've simulated the upgrade from an eclair node running the latest master branch, the test were successful and no errors were encountered.

Test 1: running option_static_remotekey open a channel to LND (make sure you activated the feature in your conf), then process a few payments to make the commit index increase and check that the main outputs don't change scriptPubKey across commitments. Then force close the channel from the remote side and check that eclair doesn't spend/redeem its main output but instead it should be immediately available to bitcoind. Repeat the test initiating the channel from the remote side (LND).

In test 2 we want to make sure it's safe to upgrade from a node currently running master with several channels already opened or in closing states.
Running master open a few channels (both as funder and fundee) then force-close one of them and open a new one leaving it waiting for the funding to be locked. Now stop and restart eclair upgrading to option_static_remotekey, on restart all channels should function normally.

Note for reviewers: to reproduce my tests easily here are some go-to instructions to run LND in your local machine (assumes docker + ubuntu):

1 git clone github.com:lightningnetwork/lnd && cd lnd
2 docker build -t lnd-image-master .
// adjust to your local bitcoind instance
3 docker create --net=host --name lnd lnd-image-master  --bitcoin.active --bitcoin.node=bitcoind --bitcoin.regtest --bitcoind.rpchost=127.0.0.1:19234 --bitcoind.rpcuser=foobaz --bitcoind.rpcpass=barbaz --bitcoind.zmqpubrawblock=127.0.0.1:29001 --bitcoind.zmqpubrawtx=127.0.0.1:29002 --no-macaroons
4 docker start lnd
5 docker exec -it lnd lncli --no-macaroons create 
// create invoice from remote
6 docker exec -it lnd lncli --no-macaroons addinvoice
// force-close the channel from remote
7 docker exec -it lnd lncli --no-macaroons closechannel --force --funding_txid <funding_txid>
// check on bitcoind that funds arrived from the commit tx 
8 bitcoin-cli listunspent | jq '.[] | select(.txid == "<commit_txid>")'

[t-bast]

I think it would be great to finalize this PR, maybe we can then start doing some work on anchor outputs since this is where spec efforts to fix the eclipse attacks are focusing (rust-lightning said they will always require static_remotekey).

@sstone will you have some time to review this PR?
I'll then reproduce the E2E tests before we merge.

[araspitzu]

I did some extra E2E test with c-lightning v0.8.2.1 and eclair 0.3.4.

The test with c-lightning reproduced what i did previously with LND, I opened a channel with the static_remotekey commitment type and checked that across different commit tx(s) we use the same to_remote address. Then I force closed the channel from the remote side and checked that our funds are sent straight into bitcoin-core bip32 wallet via a p2wkh output.
Here are some quick steps to run c-lightning in your local system and reproduce the test (assumes docker+ubuntu):

1. git clone -b v0.8.2.1 https://github.com/ElementsProject/lightning && cd lightning
2. docker build -t cl-image .
3. docker run --net=host cl-image --network=regtest --bitcoin-rpcconnect=127.0.0.1:19234 --bitcoin-rpcuser=foobaz --bitcoin-rpcpassword=barbaz // adjust to your local bitcoind instance
4. docker exec -it jolly_mcclintock lightning-cli --network=regtest getinfo // replace 'jolly_mcclintock' with your container name
// open channel from eclair
// ...
// create invoice from remote side
5. docker exec -it jolly_mcclintock lightning-cli --network=regtest invoice msatoshi=5000000 label="" description="
// force close channel from remote
6. docker exec -it jolly_mcclintock lightning-cli --network=regtest close <channel_id> 1
// check on bitcoind that funds arrived from the commit tx 
7. bitcoin-cli listunspent | jq '.[] | select(.txid == "<commit_txid>")'

The second test with an older version of eclair aimed and checking we're still backward compatible, the test comprised of opening channels, making payments and closing the channels (both as funder and fundee). This test was successful since no errors was encountered (note there isn't anything specific to option_static_remotekey to be checked)

Summary of interop test so far:

	force_close_by_local	force_close_by_remote
remote_side LND	✔️	✔️
remote_side CL	✔️	✔️

[sstone]

@araspitzu we should keep the "non static remote key" test vectors and and new ones with the option set instead of replacing them

[sstone]

I tested with lnd and c-lightning, and checked that it can be merged and work on Android as well (static_remote_key will be disabled for now on our android apps) and everything is fine. I think that once we have both test vectors and have a second approval we're good to go!

[t-bast]

I think that once we have both test vectors and have a second approval we're good to go!

Great, I'll test and review today!

[araspitzu]

Pushed last commit with both versions of test vectors being used in the tests 🚀

[t-bast]

LGTM!
I've done some E2E testing with lnd 0.10.1-beta, didn't spot anything weird.

[Roasbeef]

🚀

Super happy about this! We're actually looking at making the feature bit start to be required in a release or so.

[t-bast]

Sounds good, in that case we'll probably align and at least turn it on optionally by default!
We still need some work done on mobile to fully support it (Bech32 support and stuff) which is why it took a bit long and still isn't enabled in our default config.