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

Add Option Upfront Shutdown #3655

Merged

Conversation

carlaKC
Copy link
Collaborator

@carlaKC carlaKC commented Oct 31, 2019

This PR adds support for option_upfront_shutdown_commitment, which allows nodes to specify a script to which funds should be paid out in the event of a cooperative close during channel negotiation.

Upfront shutdown addresses are set by default for channels that we open or accept if the remote peer supports this feature.

Replaces #449

Copy link
Collaborator

@halseth halseth left a comment

Did a high level pass, things are coming together! Need to read up more on BOLT-2 before I can give it a more detailed look 😅

lnwire/lnwire.go Outdated Show resolved Hide resolved
lnwire/accept_channel.go Show resolved Hide resolved
channeldb/channel.go Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
discovery/mock_test.go Outdated Show resolved Hide resolved
peer.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
lnwire/features.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

@carlaKC solid PR! looks pretty complete to me, well tested and well documented :) completed an initial pass

if err != nil && err != io.EOF {
return err
}
return nil
Copy link
Collaborator

@cfromknecht cfromknecht Oct 31, 2019

Choose a reason for hiding this comment

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

note to self/spec: adding a TLV field after this seems difficult, as it requires knowing whether or not to expect the delivery address. we might accidentally parse a tlv stream into a delivery address, cc @Roasbeef @joostjager

lnwire/accept_channel.go Show resolved Hide resolved
lnwire/shutdown.go Show resolved Hide resolved
lnwire/features.go Outdated Show resolved Hide resolved
lnwire/features.go Outdated Show resolved Hide resolved
lnwire/accept_channel.go Show resolved Hide resolved
channeldb/channel.go Outdated Show resolved Hide resolved
channeldb/channel.go Outdated Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
@wpaulino wpaulino removed their request for review Oct 31, 2019
@carlaKC carlaKC added the v0.9.0 label Oct 31, 2019
@carlaKC carlaKC force-pushed the fundingmgr-optionupfrontshutdown branch from a212935 to 49aa391 Compare Nov 1, 2019
@carlaKC
Copy link
Collaborator Author

@carlaKC carlaKC commented Nov 1, 2019

Pushed some small fixes + the ability to enable upfront shutdown a with enable_upfront_shutdown flag.

@joostjager joostjager added this to Needs Review in v0.9.0-beta Nov 1, 2019
@joostjager joostjager added this to the 0.9.0 milestone Nov 1, 2019
@carlaKC carlaKC requested review from cfromknecht and halseth Nov 4, 2019
lnwire/open_channel.go Outdated Show resolved Hide resolved
return err
}

if len(a.UpfrontShutdownScript) == 0 {
Copy link
Collaborator

@halseth halseth Nov 4, 2019

Choose a reason for hiding this comment

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

The spec says

if both nodes advertised the option_upfront_shutdown_script feature:
MUST include either a valid shutdown_scriptpubkey as required by shutdown scriptpubkey, or a zero-length shutdown_scriptpubkey.`

I read this as we must write a zero length field in this case?

Copy link
Collaborator Author

@carlaKC carlaKC Nov 6, 2019

Choose a reason for hiding this comment

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

Checked this out and you're right about the 0 value 👍

So proceeding with the following:

  • Encode a 0 for empty shutdown address (fine for non upfront shutdown understanding lnd, will have to double check interop)
  • Encode len/addr when given a non-0 delivery address
  • Always try to decode with the EOF trick

lnwire/open_channel.go Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
@carlaKC carlaKC moved this from Needs Review to WIP in v0.9.0-beta Nov 5, 2019

errChan := make(chan error, 1)

r.wallet.msgChan <- &addUpfrontShutdown{
Copy link
Member

@Roasbeef Roasbeef Nov 6, 2019

Choose a reason for hiding this comment

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

As there's no real validation going on here (other than the script size), I don't think this needs to go into the wallet. Instead, we can extend the normal contributions to encompass this new piece of information, as after the first two funding flow messages, all the necessary information is known.

config.go Outdated Show resolved Hide resolved
// embed the peer interface so that mockPeer will implement lnpeer.Peer by
// default. Methods that need to be called in tests still need to be
// implemented.
lnpeer.Peer
Copy link
Member

@Roasbeef Roasbeef Nov 6, 2019

Choose a reason for hiding this comment

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

Not sure if this is a good move, as it may cause confusion when the interface is extended, but the tests aren't updated and fail. Also tbh, I didn't know this was possible :p

Copy link
Collaborator Author

@carlaKC carlaKC Nov 6, 2019

Choose a reason for hiding this comment

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

My argument for using this is that it results in tests failing loudly. If you extend an interface and a test uses the new function call it will panic, which you'll pick up and have to go see exactly what your change is affecting. If the tests need the new function, you will have to update the mock in your PR (as before), this just saves on some boilerplate :)

Copy link
Collaborator

@halseth halseth Nov 11, 2019

Choose a reason for hiding this comment

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

I like this boilerplate-free world :)

flags lnwire.FundingFlag,
tweaklessCommit bool) (*ChannelReservation, error) {
flags lnwire.FundingFlag, tweaklessCommit bool, localShutdown,
remoteShutdown lnwire.DeliveryAddress) (*ChannelReservation, error) {
Copy link
Member

@Roasbeef Roasbeef Nov 6, 2019

Choose a reason for hiding this comment

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

Following from my other comment, I think this can be handled as a contribution.

lnwallet/wallet.go Outdated Show resolved Hide resolved
v0.9.0-beta automation moved this from WIP to Needs Review Nov 6, 2019
@carlaKC carlaKC force-pushed the fundingmgr-optionupfrontshutdown branch from 49aa391 to a8a3a10 Compare Nov 6, 2019
@carlaKC carlaKC requested a review from joostjager as a code owner Nov 6, 2019
@carlaKC carlaKC force-pushed the fundingmgr-optionupfrontshutdown branch from a8a3a10 to 1b7c6e4 Compare Nov 6, 2019
@carlaKC carlaKC removed the request for review from joostjager Nov 6, 2019
fundingmanager_test.go Outdated Show resolved Hide resolved
@carlaKC carlaKC force-pushed the fundingmgr-optionupfrontshutdown branch 2 times, most recently from a24d0a2 to 3ff97a2 Compare Nov 8, 2019
@Roasbeef Roasbeef added enhancement security spec wire protocol labels Nov 9, 2019
@carlaKC carlaKC requested a review from halseth Nov 19, 2019
lnwire/lnwire_test.go Outdated Show resolved Hide resolved
chancloser.go Outdated Show resolved Hide resolved
peer.go Outdated Show resolved Hide resolved
peer.go Show resolved Hide resolved
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

@carlaKC latest version is 🔥well tested and easy to follow, only minor comments

lnwire/features.go Show resolved Hide resolved
lnwire/accept_channel_test.go Outdated Show resolved Hide resolved
peer.go Outdated Show resolved Hide resolved
chancloser.go Outdated
// Now that we know this is a valid shutdown message, we'll
// If the remote node opened the channel with option upfront shutdown
// script, check that the script they provided matches.
err := maybeMatchScript(
Copy link
Collaborator

@cfromknecht cfromknecht Nov 22, 2019

Choose a reason for hiding this comment

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

is it possible to pull this into a helper so that isn't duplicated?

chancloser_test.go Outdated Show resolved Hide resolved
expectedRemote lnwire.DeliveryAddress
}{
{
name: "No shutdown scripts",
Copy link
Collaborator

@cfromknecht cfromknecht Nov 22, 2019

Choose a reason for hiding this comment

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

typically we use all lower cases for test names, not a blocker tho

shutdown, err := getUpfrontShutdownScript(
msg.peer,
msg.openChanReq.shutdownScript,
func() (lnwire.DeliveryAddress, error) {
Copy link
Collaborator

@cfromknecht cfromknecht Nov 22, 2019

Choose a reason for hiding this comment

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

perhaps extract this closure as a member of the funding manager? can this pass it to both as, e.g., f.genDeliveryAddr

Copy link
Collaborator Author

@carlaKC carlaKC Nov 25, 2019

Choose a reason for hiding this comment

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

Peer already has a genDeliveryScript, so this (as is, and if added to funding manager) results in some duplication. An alternative would be to expose genDeliveryScript on the peer interface, or move all of the logic in getUpfrontShutdownScript into f.genUpfrontShutdown?

Copy link
Collaborator

@halseth halseth Dec 3, 2019

Choose a reason for hiding this comment

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

I think it is just to avoid some clutter and indentation here, instead having the method be

func(f *fundingmanager) genDeliveryAddr() (lnwire.DeliveryAddress, error) {
			addr, err := f.cfg.Wallet.NewAddress(lnwallet.WitnessPubKey, false)
			if err != nil {
				return nil, err
			}
			return txscript.PayToAddrScript(addr)
}

@carlaKC carlaKC force-pushed the fundingmgr-optionupfrontshutdown branch from adf00b8 to ef67638 Compare Nov 25, 2019
@carlaKC
Copy link
Collaborator Author

@carlaKC carlaKC commented Nov 27, 2019

Done some testing to make sure these changes work with other impls/ lnd as is.

lnd:
Branch -> Master: open and close channel ok, does not set upfront for peers that don't support it
Branch -> Branch: local and remote upfront preserved through restart, shutdown addresss checked when appropriate

c-lightning:
Supports upfront shutdown
Open channel from lndl with and without upfront shutdown ok.
Open channel from c-lightning with and without upfront shutdown ok.

C-Lightning does disconnect from us on channel close, but this occurs on master as well, so I'm assuming that's because they only support single channels so do cleanup on close. Will double check.

eclair:
TBD

Just want to clarify the meaning of "MUST fail the connection" in BOLT 2 before requesting review again. Specifically whether we should error out of the close channel interaction or disconnect from the peer entirely.

@carlaKC carlaKC force-pushed the fundingmgr-optionupfrontshutdown branch 2 times, most recently from 0792a15 to e2b0509 Compare Nov 28, 2019
@carlaKC
Copy link
Collaborator Author

@carlaKC carlaKC commented Nov 28, 2019

Ecalir interop done, they don't support upfront shutdown so I just checked that channel open/close work as before.

Updated disconnection function call to permanently disconnect from the violating peer by removing them from our persistent peers. We do still have a channel open with the peer, and they will most likely just reconnect to us anyway (since we don't have the ability to ban).

stevenroose and others added 5 commits Dec 3, 2019
This commit adds the feature bit and additional fields
required in `open_channel` and `accept_channel` wire
messages for `option_upfront_shutdown_script`.
This commit enables signalling for the optional
upfront shutdown script feature bit.
This commit sets our close addresss to the address
specified by option upfront shutdown, if specified,
and disconnects from peers that fail to provide
their upfront shutdown address for coopertaive closes
of channels that were opened with the option set.
This commit adds fields for upfront shutdown scripts set
by the local and remote peer to the OpenChannel struct.
These values are optional, so they are added with their
own keys in the chanBucket in the DB.
This commit gets upfront shutdown scripts from openchannel and
acceptchannel wire messages sent from our peer and sets upfront
shutdown scripts in our open and accept channel messages when
the remote peer supports option upfront shutdown and we have
the feature enabled.
@carlaKC carlaKC force-pushed the fundingmgr-optionupfrontshutdown branch from e2b0509 to 9b35c34 Compare Dec 3, 2019
halseth
halseth approved these changes Dec 3, 2019
Copy link
Collaborator

@halseth halseth left a comment

Awesome work, this LGTM now! 💯

Follow-up would be to enable the feature on channel opens. Maybe create an issue for that for someone to pick up? :)

Copy link
Collaborator

@cfromknecht cfromknecht left a comment

beautiful PR, LGTM 💯

@joostjager joostjager moved this from Needs Review to Approved in v0.9.0-beta Dec 3, 2019
@halseth halseth removed the request for review from Roasbeef Dec 4, 2019
@carlaKC carlaKC requested a review from Roasbeef Dec 4, 2019
Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 💸

@Roasbeef Roasbeef merged commit 183797f into lightningnetwork:master Dec 5, 2019
2 checks passed
v0.9.0-beta automation moved this from Approved to Done Dec 5, 2019
@carlaKC carlaKC deleted the fundingmgr-optionupfrontshutdown branch Apr 3, 2020
@GlenCooper
Copy link
Contributor

@GlenCooper GlenCooper commented Jun 13, 2020

👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement P2 security spec v0.9.0 wire protocol
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants