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

option_static_remotekey #642

Merged

Conversation

@rustyrussell
Copy link
Collaborator

commented Jul 17, 2019

This is now just removing key rotation (no CSV symmetry). I've used the placeholder optnum 48/49, we can assign a proper one at the meeting if accepted.

I have implemented this in c-lightning as an EXPERIMENTAL_FEATURES option, but not yet merged. It seems to work, and I also have protocol tests for it.

Copy link
Collaborator

left a comment

Thanks @rustyrussell, definitely like the size of diff :)

Do we also want to modify the derivations described in BOLT 3 to say that that the payment_basepoint is not tweaked?

02-peer-protocol.md Outdated Show resolved Hide resolved
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2019

Thanks @rustyrussell, definitely like the size of diff :)

Do we also want to modify the derivations described in BOLT 3 to say that that the payment_basepoint is not tweaked?

Huh, somehow my rebase lost that part! I def did edit that, will push...

03-transactions.md Outdated Show resolved Hide resolved
removed, but the disclosure of previous secret still allows
fall-behind detection. An implementation can offer both, however, and
fall back to the `option_data_loss_protect` behavior if
`option_simplified_commitment` is not negotiated.

This comment has been minimized.

Copy link
@araspitzu

araspitzu Jul 19, 2019

Contributor
Suggested change
`option_simplified_commitment` is not negotiated.
`option_static_remotekey` is not negotiated.
02-peer-protocol.md Outdated Show resolved Hide resolved
02-peer-protocol.md Show resolved Hide resolved
09-features.md Outdated Show resolved Hide resolved
@t-bast t-bast moved this from Scheduled to Accepted in Specification Meeting Agenda Aug 19, 2019
@t-bast t-bast moved this from Accepted to Scheduled in Specification Meeting Agenda Aug 19, 2019
@rustyrussell rustyrussell force-pushed the rustyrussell:option_static_remotekey branch from 225ceab to 930a9b4 Aug 28, 2019
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 28, 2019

Rebased to fix minor conflicts, and added minor fixes in separate commit.

rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 29, 2019
Aka. BOLTVERSION=930a9b44076a8f25a8626b31b3d5a55c0888308c from
lightningnetwork/lightning-rfc#642

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 29, 2019
Aka. BOLTVERSION=930a9b44076a8f25a8626b31b3d5a55c0888308c from
lightningnetwork/lightning-rfc#642

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Aug 29, 2019
Aka. BOLTVERSION=930a9b44076a8f25a8626b31b3d5a55c0888308c from
lightningnetwork/lightning-rfc#642

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 30, 2019

OK, I am now running this on my two main test nodes:

mainnet: 024b9a1fa8e006f1e3937f65f66c408e6da8e1ca728ea43222a7381df1cc449605
testnet: 031a3478d481b92e3c28810228252898c5f0d82fc4d07f5210c4f34d4aba56b769

@rustyrussell rustyrussell referenced this pull request Sep 2, 2019
6 of 10 tasks complete
@rustyrussell rustyrussell changed the title option_static_remotekey: first draft. option_static_remotekey Sep 3, 2019
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Sep 7, 2019
Aka. BOLTVERSION=930a9b44076a8f25a8626b31b3d5a55c0888308c from
lightningnetwork/lightning-rfc#642

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Sep 7, 2019
Aka. BOLTVERSION=930a9b44076a8f25a8626b31b3d5a55c0888308c from
lightningnetwork/lightning-rfc#642

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell added a commit to rustyrussell/lightning that referenced this pull request Sep 10, 2019
Aka. BOLTVERSION=930a9b44076a8f25a8626b31b3d5a55c0888308c from
lightningnetwork/lightning-rfc#642

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
niftynei added a commit to ElementsProject/lightning that referenced this pull request Sep 10, 2019
Aka. BOLTVERSION=930a9b44076a8f25a8626b31b3d5a55c0888308c from
lightningnetwork/lightning-rfc#642

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@sstone

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2019

It just hit me that switching to static remote keys without adding a CSV delay, as for the to-local keys, is making incentives for using mutual vs force close worse than they are today.

With the current specs, not having a delay on the to-remote is already a problem: if you can get your peer to publish their commit tx, you get a UTXO that you can spend right away, while they have to wait to spend theirs. However, most implementations will use BIP32 wallets and will want spend this output (and pay onchain fees) and send it to a wallet key, as it makes backup/restore of onchain funds much easier (you just need your seed). So it's still better to negotiate a mutual close whenever possible, as it will return your funds directly to your BIP32 wallet.

With this proposal, without adding a CSV delay to the to-remote output, implementation will most likely set their static remote key to one of their BIP32 wallet key, which means that in practice getting your peers to publish their commit tx will always be the fastest solution, and could also be the cheapest if fees are going up and you don't want to negotiate.

This is why I would strongly prefer that we add a CSV delay to the to-remote output: users would then have an incentive to mutual close channels whenever possible, and not play the "get your peer to close first" game.

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

@sstone

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2019

Adding a CSV value makes the recovery case less self contained.

What does that mean ?

Having you peer include in its commit tx an output that sends directly to your wallet feels just plain wrong. What is the point of even negotiating a mutual close then ?

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

@SomberNight

This comment has been minimized.

Copy link

commented Sep 15, 2019

With this proposal, without adding a CSV delay to the to-remote output, implementation will most likely set their static remote key to one of their BIP32 wallet key
Having you peer include in its commit tx an output that sends directly to your wallet

Note that the to-remote-output pays to a p2wpkh script; and longer term there will be wallets who would not by default derive p2wpkh addresses for their main on-chain addresses. E.g. when witness v1 gets soft forked in, some wallets might want to only have witness v1 addresses, and those will then need to sweep the to-remote outputs, at which point a cooperative close paying directly to one of their main witness v1 addresses (as soon as will be allowed for a cooperative close to do so, as it is not atm) would be cheaper.

My point is that it's not necessarily a true assumption that the commitment tx having a static p2wpkh output can always directly pay to a main wallet address.

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

@SomberNight

This comment has been minimized.

Copy link

commented Sep 15, 2019

There are already wallets that only derive witness v0 outputs, e.g. Wasabi and Electrum.

edit; and there is no need for a transition period between v0 and v1, as in theory anyone who can pay to v0 can also do so to v1.

@sstone

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2019

@Roasbeef I don't see why adding a delay here prevents you from recovering your funds ? My point is that not having such a delay was bad before and we're making it worse.

The fact that you currently need you peer's commit point is a different problem. the to-remote key could have been derived from the remote commit point (I don't think it makes security/privacy worse) and it would have made recovery much simpler.

@SomberNight well we'll have eltoo by then won't we :) but it may not be soon...

@t-bast t-bast moved this from Scheduled to Accepted in Specification Meeting Agenda Sep 16, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

I don't see why adding a delay here prevents you from recovering your funds ?

It doesn't prevent it, it makes it less self contained: recovery logic now needs additional data (to reconstruct the script).

My point is that not having such a delay was bad before and we're making it worse.

With my response being that the real incentive issue here is the initiator always paying the fees whenever a channel closes.

The fact that you currently need you peer's commit point is a different problem. the to-remote key could have been derived from the remote commit point (I don't think it makes security/privacy worse) and it would have made recovery much simpler.

This is the very problem that's being fixed with this change.

@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 19, 2019

@Roasbeef Meeting nodes say this is pending your ack to merge? Ping!

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Actually, the current text looks fine, scratch my last comment, doing one final pass over everything.

Copy link
Member

left a comment

LGTM 🍕

@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2019

needs rebase!!

This separates out the static remotekey changes from the more ambitious
option_simplified_commitment (which also included pushme outputs and
bring-your-own-fee for HTLC outputs).

As per http://www.erisian.com.au/meetbot/lightning-dev/2019/lightning-dev.2019-09-02-20.06.html

Thanks to everyone for feedback: @araspitzu @Roasbeef @bitconner

Suggested-by: @Roasbeef
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the rustyrussell:option_static_remotekey branch from 531c8d7 to ae477fd Sep 26, 2019
@rustyrussell

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 26, 2019

Trivial rebase, and collapsed into a single commit.

@rustyrussell rustyrussell merged commit 2afe355 into lightningnetwork:master Sep 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Xekyo

This comment has been minimized.

Copy link

commented Oct 9, 2019

This appears to allow to self-select an address from the Lightning Client's wallet only. Is anything planned that would require channels to close to an arbitrary user-selected addresses in the force-close scenario?

@Roasbeef

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
You can’t perform that action at this time.