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 optional close address #3702

Merged
merged 3 commits into from Dec 10, 2019
Merged

Conversation

@carlaKC
Copy link
Collaborator

carlaKC commented Nov 11, 2019

This PR adds a delivery_address field to lnrpc and lncli's CloseChannel request which allows users to specify an address to close out to on cooperative closure of a channel. If an upfront shutdown address was set on channel open, requesting to close the channel out to a specific address will fail.

This PR replaces #1462.

This PR is rebased on the open upfront shutdown PR #3655, as much of the logic is shared.
Only the last 3 commits in this PR are relevant for review.

@carlaKC carlaKC removed request for Roasbeef and wpaulino Nov 11, 2019
@carlaKC carlaKC added the v0.9.0 label Nov 11, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Nov 12, 2019
@joostjager joostjager added this to the 0.9.0 milestone Nov 12, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator

joostjager commented Nov 12, 2019

Needs rebase

@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Nov 12, 2019
@carlaKC carlaKC force-pushed the carlaKC:lnrpc-addcloseaddress branch from 30168ae to 377e730 Nov 12, 2019
@Roasbeef Roasbeef requested review from bhandras and removed request for cfromknecht Nov 13, 2019
lnrpc/rpc.swagger.json Outdated Show resolved Hide resolved
Copy link
Collaborator

bhandras left a comment

Really nice PR!

htlcswitch/switch.go Show resolved Hide resolved
chancloser.go Outdated Show resolved Hide resolved
// checkRemoteDeliveryAddress checks that the address provided by the remote
// peer matches the upfront shutdown address, if it is set. It does not perform
// any validation on the address provided.
func checkRemoteDeliveryAddress(upfront, address lnwire.DeliveryAddress) error {

This comment has been minimized.

Copy link
@bhandras

bhandras Nov 14, 2019

Collaborator

I'd probably call this function sg like: Matches and maybe move it under lnwire.DeliveryAddress (and add a unit test). WDYT?

This comment has been minimized.

Copy link
@carlaKC

carlaKC Nov 19, 2019

Author Collaborator

+1 for the unit test, see comment below for context on this check :)

// any validation on the address provided.
func checkRemoteDeliveryAddress(upfront, address lnwire.DeliveryAddress) error {
// If an upfront shutdown address is empty, return early.
if len(upfront) == 0 {

This comment has been minimized.

Copy link
@bhandras

bhandras Nov 14, 2019

Collaborator

Does this check add anything (if yes: should emptyness checked for address)

This comment has been minimized.

Copy link
@carlaKC

carlaKC Nov 19, 2019

Author Collaborator

So here we are checking whether the address our peer has provided matches the pre-negotiated upfront address. There are two cases:

  1. They did not negotiate an upfront address (it's an optional feature), so we do not need to enforce close out to a specific address
  2. They did negotiate an upfront address, and we need to check that they're sticking to it; if not, we need to disconnect as per the spec's instruction because they may be comporomised
chancloser_test.go Outdated Show resolved Hide resolved
fundingmanager_test.go Outdated Show resolved Hide resolved
r.Lock()
defer r.Unlock()

r.ourContribution.UpfrontShutdown = shutdown

This comment has been minimized.

Copy link
@bhandras

bhandras Nov 14, 2019

Collaborator

nit: As a reader it's really confusing to me that sometimes it's Shutdown, sometimes ShutdownScript. I guess this is because I'm not familiar with the code base yet.

This comment has been minimized.

Copy link
@carlaKC

carlaKC Nov 19, 2019

Author Collaborator

That's an inconsistency on my side, trying to keep var names brief. I'll settle on one or the other,

lnwire/accept_channel_test.go Outdated Show resolved Hide resolved
lnwire/shutdown.go Outdated Show resolved Hide resolved
peer_test.go Outdated Show resolved Hide resolved
@carlaKC

This comment has been minimized.

Copy link
Collaborator Author

carlaKC commented Nov 19, 2019

Thanks for the review @bhandras!
Since this PR is rebased on #3655, only the last two commits are relevant for review in this PR, sorry for not making that clearer! I'm not going to reply to the comments on the original PR commits, because that's going to add noise to this PR, but I will address them on 3655.

@carlaKC carlaKC force-pushed the carlaKC:lnrpc-addcloseaddress branch from 377e730 to 38acea3 Nov 20, 2019
rpcserver.go Outdated Show resolved Hide resolved
@carlaKC

This comment has been minimized.

Copy link
Collaborator Author

carlaKC commented Nov 20, 2019

Updated to address comments and rebased on #3655.

@halseth halseth requested a review from bhandras Nov 21, 2019

// Finally, add optional shutdown scripts for the local and remote peer if
// they are present.
if err := putOptionalUpfrontShutdownScript(

This comment has been minimized.

Copy link
@bhandras

bhandras Nov 26, 2019

Collaborator

After taking a closer look, it's always inside a Bolt transaction and therefore atomic.

lnrpc/rpc.swagger.json Outdated Show resolved Hide resolved
if err != nil {
return nil, err
}
return txscript.PayToAddrScript(addr)

This comment has been minimized.

Copy link
@bhandras

bhandras Nov 26, 2019

Collaborator

Duh :) Sorry, didn't pay attention to what txscropt.PayToAddrScript(addr) returns :)

@carlaKC carlaKC force-pushed the carlaKC:lnrpc-addcloseaddress branch from 38acea3 to ee36c82 Dec 3, 2019
@carlaKC

This comment has been minimized.

Copy link
Collaborator Author

carlaKC commented Dec 3, 2019

Rebased, but going to hold off on review for this one until parent PR is in.

@carlaKC carlaKC force-pushed the carlaKC:lnrpc-addcloseaddress branch from ee36c82 to c128b6b Dec 5, 2019
@carlaKC carlaKC requested a review from bhandras Dec 5, 2019
Copy link
Collaborator

bhandras left a comment

Only nits, other than these: LGTM 💯

lnrpc/rpc.proto Outdated Show resolved Hide resolved
chancloser.go Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
server.go Show resolved Hide resolved
@Roasbeef Roasbeef requested review from wpaulino and removed request for halseth Dec 5, 2019
Copy link
Collaborator

wpaulino left a comment

Nice to finally have this land! Just a few nits from me, also needs a rebase due to a proto conflict.

EDIT: I think it'd be nice to give the original author of the PR credit in the other commits as well.

cmd/lncli/commands.go Outdated Show resolved Hide resolved
htlcswitch/switch.go Outdated Show resolved Hide resolved
peer.go Outdated Show resolved Hide resolved
peer_test.go Outdated Show resolved Hide resolved
@carlaKC carlaKC force-pushed the carlaKC:lnrpc-addcloseaddress branch 3 times, most recently from e1b0b81 to 3ff4ba9 Dec 6, 2019
@carlaKC carlaKC requested a review from bhandras Dec 8, 2019
rpcserver.go Outdated Show resolved Hide resolved
server.go Show resolved Hide resolved
v0.9.0-beta automation moved this from Needs Review to Approved Dec 9, 2019
@carlaKC carlaKC force-pushed the carlaKC:lnrpc-addcloseaddress branch from 3ff4ba9 to 6b3280c Dec 9, 2019
Bluetegu and others added 3 commits Dec 9, 2019
This commit is adapted from @Bluetegu's original
pull request #1462.
This commit is adapted from @Bluetegu's original
pull request #1462.
This commit is adapted from @Bluetegu's original
pull request #1462.

This commit reads an optional address to pay funds out to
from a user iniitiated close channel address. If the channel
already has a shutdown script set, the request will fail if
an address is provided. Otherwise, the cooperative close will
pay out to the address provided.
@carlaKC carlaKC force-pushed the carlaKC:lnrpc-addcloseaddress branch from 6b3280c to 94d3eb6 Dec 10, 2019
@carlaKC

This comment has been minimized.

Copy link
Collaborator Author

carlaKC commented Dec 10, 2019

Rebased to clear proto conflicts :)

@wpaulino wpaulino merged commit 3c6be62 into lightningnetwork:master Dec 10, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on lnrpc-addcloseaddress at 62.205%
Details
v0.9.0-beta automation moved this from Approved to Done Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
6 participants
You can’t perform that action at this time.