Skip to content

Add Upfront Shutdown Address to OpenChannelRequest#3796

Merged
wpaulino merged 5 commits into
lightningnetwork:masterfrom
carlaKC:3786-setupfrontshutdown
Dec 18, 2019
Merged

Add Upfront Shutdown Address to OpenChannelRequest#3796
wpaulino merged 5 commits into
lightningnetwork:masterfrom
carlaKC:3786-setupfrontshutdown

Conversation

@carlaKC
Copy link
Copy Markdown
Collaborator

@carlaKC carlaKC commented Dec 5, 2019

This PR adds an upfront shutdown address to lnrpc and lncli. Cooperative close out to these addresses is enforced provided that the remote peer supports the feature bit. The request will fail if an address is set for a peer that does not support upfront shutdown.

It also exposes the features that peers support in a new rpc endpoint GetFeatures so that users can check whether the peer supports the feature before setting the field (and check which features their node supports, if desired), which has the following output:

{
    "features": [
        {
            "bit": 5,
            "name": "upfront-shutdown-script",
            "is_required": false,
            "is_known": true
        },
        {
            "bit": 7,
            "name": "gossip-queries",
            "is_required": false,
            "is_known": true
        },
        {
            "bit": 9,
            "name": "tlv-onion",
            "is_required": false,
            "is_known": true
        },
        {
            "bit": 13,
            "name": "static-remote-key",
            "is_required": false,
            "is_known": true
        },
        {
            "bit": 0,
            "name": "data-loss-protect",
            "is_required": true,
            "is_known": true
        }
    ]
}

Fixes #3786

@carlaKC carlaKC force-pushed the 3786-setupfrontshutdown branch 3 times, most recently from 389ac03 to c15529f Compare December 11, 2019 15:22
@carlaKC carlaKC marked this pull request as ready for review December 11, 2019 15:23
@carlaKC
Copy link
Copy Markdown
Collaborator Author

carlaKC commented Dec 12, 2019

While working on a first iteration of this PR which had feature bits in ListPeers I picked up a race condition in our itests between connecting to a peer and handleInitMessage because remoteFeatures are only set on receipt of init so tests would panic on calls to ListPeers which had nil remoteFeatures. Not going to be a real world issue, just making a note of it here.

@carlaKC carlaKC force-pushed the 3786-setupfrontshutdown branch 2 times, most recently from 730d686 to dd2c6eb Compare December 12, 2019 06:56
@carlaKC
Copy link
Copy Markdown
Collaborator Author

carlaKC commented Dec 12, 2019

Rebased on #3824 to use common method, so first two commits are not up for review.

@carlaKC carlaKC force-pushed the 3786-setupfrontshutdown branch from dd2c6eb to 669eb19 Compare December 12, 2019 07:11
Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

@carlaKC clean pr! first two commits are in master now so we can rebase.

excited that we'll now have some moree insight into our peer's features. i wonder if maybe we should add the remote init features to listpeers as well? other than that, only a few small comments left.

Comment thread feature/manager.go Outdated
Comment thread rpcserver.go Outdated
Comment thread rpcserver.go Outdated
Comment thread lnrpc/rpc.proto Outdated
Comment thread rpcserver.go Outdated
@cfromknecht
Copy link
Copy Markdown
Contributor

also note that #3837 proposes changing the rpc features to a map

@carlaKC
Copy link
Copy Markdown
Collaborator Author

carlaKC commented Dec 15, 2019

also note that #3837 proposes changing the rpc features to a map

Given the addition of features to ListPeers and GetNode in #3837, I'm just going to add our own features to GetInfo, since peer features are sufficiently exposed.

@carlaKC carlaKC force-pushed the 3786-setupfrontshutdown branch 2 times, most recently from a7ca7d8 to cc423fa Compare December 15, 2019 13:20
@carlaKC carlaKC requested a review from cfromknecht December 16, 2019 06:15
Copy link
Copy Markdown
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

LGTM. Needs a rebase due to a conflict and the commits from #3837 are still present in the branch.

Comment thread lnrpc/rpc.proto Outdated
@carlaKC carlaKC added the v0.9.0 label Dec 17, 2019
@carlaKC carlaKC force-pushed the 3786-setupfrontshutdown branch from cc423fa to 2c84620 Compare December 17, 2019 06:11
Copy link
Copy Markdown
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Confirmed in a test environment I was only able to coop close out to the upfront address.

@carlaKC carlaKC force-pushed the 3786-setupfrontshutdown branch from 2c84620 to 2482ee4 Compare December 17, 2019 19:59
@carlaKC
Copy link
Copy Markdown
Collaborator Author

carlaKC commented Dec 17, 2019

Rebased to address merge conflict 👮‍♀

Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM! 🥂 one small nit

Comment thread lnrpc/rpc.proto Outdated
@joostjager joostjager added this to the 0.9.0 milestone Dec 17, 2019
@cfromknecht
Copy link
Copy Markdown
Contributor

@carlaKC needs rebase

@carlaKC carlaKC force-pushed the 3786-setupfrontshutdown branch from 2482ee4 to eb3a2e6 Compare December 17, 2019 21:21
@wpaulino wpaulino merged commit 7f3771e into lightningnetwork:master Dec 18, 2019
@carlaKC carlaKC deleted the 3786-setupfrontshutdown branch April 3, 2020 09:21
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.

Set Upfront Shutdown Address via lnrpc/lncli

4 participants