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.

Sign up for GitHub

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

Jump to bottom

PSBT: add no_publish flag for safe batch channel opens v2 #4455

Merged
merged 6 commits into from
Jul 20, 2020
Merged

PSBT: add no_publish flag for safe batch channel opens v2 #4455

merged 6 commits into from
Jul 20, 2020

Conversation

cfromknecht
Copy link
Contributor

To make it safer to open multiple channels in one batch, we add the --no_publish flag to lncli openchannel (called no_publish in the PsbtShim in RPC).

When opening multiple channels in the same transaction, the --no_publish flag should be set for every channel but the last one. That way only after the last channel is negotiated successfully the batch transaction is published to the network. If that isn't done, it could lead to loss of funds if one of the peers involved times out and no commitment transactions were signed for that channel.

Shepherding #4417 since @guggero is offline for now.

@cfromknecht cfromknecht added this to the 0.11.0 milestone Jul 9, 2020
@cfromknecht cfromknecht mentioned this pull request Jul 9, 2020
Closed
@cfromknecht cfromknecht added cli Related to the command line interface psbt rpc Related to the RPC interface safety General label for issues/PRs related to the safety of using the software labels Jul 9, 2020
@cfromknecht cfromknecht requested a review from joostjager July 9, 2020 00:11
guggero added 3 commits July 8, 2020 17:12
@guggero @cfromknecht
lnrpc: add no_publish flag to PSBT shim
3a818b9
@guggero @cfromknecht
lnwallet+rpcserver: add no_publish flag to PSBT assembler
2589962
@guggero @cfromknecht
lnwallet: don't publish funding TX if flag set on assembler
56b916a 
The first channels of a batch shouldn't publish the batch TX
to avoid problems if some of the funding flows can't be completed.
Only the last channel of a batch should publish. We set the channel flag
accordingly depending on the flag in the assembler.
Roasbeef
Roasbeef approved these changes Jul 9, 2020
View reviewed changes
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🐝

cmd/lncli/cmd_open_channel.go Outdated Show resolved Hide resolved
@cfromknecht cfromknecht requested a review from Roasbeef July 9, 2020 05:26
yyforyongyu
yyforyongyu reviewed Jul 9, 2020
View reviewed changes
docs/psbt.md Outdated Show resolved Hide resolved
@yaslama
Copy link
Contributor

yaslama commented Jul 9, 2020

@cfromknecht There is a use case for the --no_publish flag even outside the context of PSBT: when using "zeroconf" channels (#4424).
Alice wants to pay to Carole, but Carole has no channel with sufficient capacity. So she sends an invoice to Alice with routing hints with a dummy channel between Bob and Carole. When Bob receives the HTLCs of the payment from Alice, he creates a zeroconf channel and forward the HTLCs to Carole. The problem is that there are cases when only part of the HTLCs reaches Bob, so Bob wants to "really" open the channel only when all the amount was forwarded to Carole. On way to do that is to open the channel with a "--no_publish" option and publish it only after all the HTLCs are forwarded to Carole.
So what do you think about extending the usage --no-publish for all channel creation.

@Roasbeef
Copy link
Member

Roasbeef commented Jul 10, 2020 •
edited
Loading

So what do you think about extending the usage --no-publish for all channel creation.

Seems useful, but out of the scope of this PR, as we just want to modify the existing behavior for the PSBT case specifically.

@cfromknecht
Copy link
Contributor Author

cc @joostjager PTAL

@Roasbeef Roasbeef requested review from guggero and removed request for joostjager July 18, 2020 04:03
guggero
guggero approved these changes Jul 20, 2020
View reviewed changes
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM ✔️
Not sure if my approval should fully count as I wrote the PR originally.

@halseth halseth removed their request for review July 20, 2020 11:04
@Roasbeef Roasbeef merged commit 14a047f into lightningnetwork:master Jul 20, 2020
@cfromknecht cfromknecht deleted the psbt-no-publish branch July 20, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yyforyongyu yyforyongyu yyforyongyu left review comments

@guggero guggero guggero approved these changes

@Roasbeef Roasbeef Roasbeef approved these changes

Assignees
No one assigned
Labels
cli Related to the command line interface psbt rpc Related to the RPC interface safety General label for issues/PRs related to the safety of using the software
Projects
None yet
Milestone
0.11.0
Development

Successfully merging this pull request may close these issues.
5 participants
@cfromknecht @yaslama @Roasbeef @guggero @yyforyongyu