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

Atomic batch channel funding #5356

Merged
merged 8 commits into from
Sep 3, 2021

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Jun 3, 2021

Replaces/continues #4375.

The feature to safely/atomically open multiple channels in a single transaction was requested several times in the latest round tables.
Batch funding is already possible through the PSBT funding API but is quite cumbersome to use if the the internal wallet should fund those channels in the end.
It is also tricky to use the PSBT funding API safely in an atomic way if the transaction gets published by accident. Cleaning up in a failure case is also not very easy.

The new BatchChannelOpen RPC uses the PSBT funding API internally to safely and atomically open multiple channels to different nodes in the same (internal wallet funded) funding transaction.

@guggero guggero added enhancement Improvements to existing features / behaviour rpc Related to the RPC interface wallet The wallet (lnwallet) which LND uses funding Related to the opening of new channels with funding transactions on the blockchain labels Jun 3, 2021
@guggero guggero added this to the v0.14.0 milestone Jun 3, 2021
@guggero guggero requested review from bhandras and wpaulino June 3, 2021 20:38
@guggero guggero mentioned this pull request Jun 3, 2021
@alexbosworth
Copy link
Contributor

alexbosworth commented Jun 9, 2021

I'd be interested in another status emitted that reflects that the commitment transaction is ready (on regular open channel as well)

also probably out of scope but a way to get the temporary ids to allow for cleanly canceling at a later time in the event of an unexpected client side problem during the open flow where that state is lost, plus reflecting the open state

@joostjager
Copy link
Collaborator

If a new RPC is created now, I think it would be good to design it with resumability in mind from the start (#5329). Channel openings are a prime candidate for automation and being able to find out what lnd is doing after a severed rpc connection is important.

@Roasbeef Roasbeef added this to In progress in v0.14.0-beta via automation Jul 1, 2021
@Roasbeef Roasbeef moved this from In progress to Review in progress in v0.14.0-beta Jul 1, 2021
@wpaulino wpaulino removed their request for review August 16, 2021 17:22
@Roasbeef Roasbeef requested review from arshbot and removed request for arshbot August 17, 2021 04:41
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.

Amazing PR! Super happy to be able to ship first class support for this within lnd itself. Also really liked how al lthe components just fell into place, building on all of our prior work re PSBT and the shims, etc.

No major comments, but just completed an initial pass, will give things a spin on testnet as well. I wonder if there's a better way to accept things on the CLI vs the json string...I guess the user can always type it out in a file, then copy/paste into the CLI, and the SendCoins command also has a similar option for multi-send.

funding/batch.go Show resolved Hide resolved
funding/batch.go Show resolved Hide resolved
funding/batch.go Show resolved Hide resolved
err)
}

fundingReq, err := b.cfg.RequestParser(&lnrpc.OpenChannelRequest{
Copy link
Member

Choose a reason for hiding this comment

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

I would think we can just use the chan request as is given there appear to be no extra fields? Other than the requirement of no-publish for the PSBt state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean use lnrpc.OpenChannelRequest instead of adding a new lnrpc.BatchOpenChannel type?
My goal was to also create a non-confusing API with just the fields per channel that are really needed, minus the deprecated ones.

funding/batch.go Show resolved Hide resolved
funding/batch.go Show resolved Hide resolved
// BatchFund starts the atomic batch channel funding process.
//
// NOTE: This method should only be called once per instance.
func (b *Batcher) BatchFund(ctx context.Context,
Copy link
Member

Choose a reason for hiding this comment

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

Really dig the control flow in this method, the sequential + even-driven layout makes the whole process really easy to follow. Also great how we were able to re-use all the singular PSBT logic as we originally envisioned.

funding/batch.go Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
lntest/itest/lnd_funding_test.go Show resolved Hide resolved
@Roasbeef Roasbeef requested review from Roasbeef and arshbot and removed request for bhandras August 18, 2021 21:56
Copy link
Contributor

@arshbot arshbot left a comment

Choose a reason for hiding this comment

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

Great PR, I do love the simplicity of this, however I think the single funding tx for all channels should be surfaced a little higher so those who may have relevant concerns would know

rpcserver.go Outdated Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
MinConfs: firstReq.MinConfs,
SpendUnconfirmed: firstReq.MinConfs == 0,
}
fundPsbtResp, err := b.cfg.WalletKitServer.FundPsbt(ctx, fundPsbtReq)
Copy link
Contributor

Choose a reason for hiding this comment

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

The funding tx for all batched channels is the same tx with different outputs? Could this be a privacy concern?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can be a privacy concern but also a privacy win, depending on how you use this. Since you already have the same with Pool, using this functionality will make your channel open transactions look like Pool batches which could also be a good thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd err on the side of documenting this behavior purely so it's properly surfaced to concerned parties.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I added a section in the psbt.md document.

@guggero
Copy link
Collaborator Author

guggero commented Aug 24, 2021

Rebased, addressed all comments and also added release notes.

This is the safer variant of using PSBTs to manually fund a batch of
channels through the OpenChannel RPC.
*/
rpc BatchOpenChannel (BatchOpenChannelRequest)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is your thought on this earlier comment?

#5356 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think this really applies to this RPC. The operation is really atomic, so it either goes through (and the channels will be pending) or it doesn't (and everything is cleaned up and canceled), no matter what happens to the RPC request.
Or do you see a case where things aren't fully reverted with the current code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The case that I see is that you launch this call and then crash, lose connection, etc. How does the caller find out whether the funding process is in progress or not? In the happy flow, the channels eventually show up as pending. But should the caller wait for a safe amount of time (how long?) before retrying the batch call? I am not sure what happens if funding fails, but perhaps nothing is seen other than messages in the log?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I agree that the case you describe isn't easy to handle. But I don't think there is anything in this particular PR that we need to add to fix the issue in #5329. What we need is a new RPC that returns any channel that is in the state where only a (non-persisted) reservation exists for it, including its pending channel ID. And possibly also add an RPC to cancel a reservation by its ID.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, my point was just to think through how this v2 channel open rpc can be used if resumability is added later. To avoid needing to create v3.

Your suggestion to create a listchannels for pre-pending - would that be sufficient? Suppose two applications connected to lnd would both do the exact same batchopen call. One of them fails, but that application will think it succeeded based on the listchannels output that shows the actions of the other batchopen call.

In #5329 I suggested a call identifier so that it will always be possible to track progress later. But that is something that can be added later on and doesn't require any changes right now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good question. I haven't tried that but it's possible that we check if a channel with the pending ID already exists.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope that at the minimum nothing bad happens in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I tried this with the example you mentioned here. If you use the same pending_chan_id, you get an error as expected Unknown desc = batch funding failed: error batch opening channel, initial negotiation failed: pendingChannelID(3f55d436d4f7d24aeffb1b976647380f22ebf9e74390e8c76dcff9fea0093b7a) already present.

BUT: You need to check the pending channels first! If the channel went through and the channel is actually pending, then using the same pending_chan_id won't result in an error (because we never store that ID anywhere other than in the reservation, which is removed in handleFundingCreated and handleFundingSigned).

I think this isn't perfect but a bit better than what you currently get with the normal channel open flow. So I won't change anything more in this PR and leave further optimizations for the PR that fixes #5329.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that is much better already! Only a tiny gap left between checking pending channels and re-trying the channel open.

So if the channel is already pending and you open a new channel with the same id, you will just get two channels without any other undesired side effects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So if the channel is already pending and you open a new channel with the same id, you will just get two channels without any other undesired side effects?

Yes. The pending channel ID is only used as the main in-memory key for the reservation, it is not used for anything later on. The "real" channel ID that is used for persisting the channel is derived from the channel point.

@guggero guggero force-pushed the batch-channel-open branch 2 times, most recently from 7bf6737 to 50674dc Compare August 24, 2021 14:45
@Roasbeef Roasbeef added the P2 should be fixed if one has time label Aug 31, 2021
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 ⚱️

One small comment here re threading through the new CommitmentType field that was added as part of the explicit channel funding feature.

state.
*/
bytes pending_chan_id = 8;
}
Copy link
Member

Choose a reason for hiding this comment

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

We should also add the new commitment_type field here: https://github.com/lightningnetwork/lnd/blob/master/lnrpc/lightning.proto#L1871

Since it would let a caller open heterogeneous channel types (for w/e reason).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, added the field and threaded it through 👍

funding/batch.go Show resolved Hide resolved
v0.14.0-beta automation moved this from Review in progress to Reviewer approved Sep 2, 2021
guggero and others added 5 commits September 2, 2021 12:11
We'll want to re-use the PSBT funding functionality from the wallet kit
sub server in the main RPC server. To be able to dynamically obtain an
instance of the wallet kit server we need to know its name under which
it registers itself in the list of sub server. We export the name so we
don't have to hard code it in the main server.
We'll want to re-use the abandon channel functionality for the batch
funding, as a cleanup in case the funding is aborted before publishing
any transaction.
@orijbot
Copy link

orijbot commented Sep 3, 2021

@guggero guggero merged commit e76c4c0 into lightningnetwork:master Sep 3, 2021
v0.14.0-beta automation moved this from Reviewer approved to Done Sep 3, 2021
@guggero guggero deleted the batch-channel-open branch September 3, 2021 17:56
@odeke-em
Copy link

odeke-em commented Sep 3, 2021

Congrats @guggero, your change didn't add any noticeable overhead after the merge, per https://dashboard.github.orijtech.com/benchmark/d38ad35aaf974ebb9665d688c1633988
image
image

cc @cuonglm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features / behaviour funding Related to the opening of new channels with funding transactions on the blockchain P2 should be fixed if one has time rpc Related to the RPC interface wallet The wallet (lnwallet) which LND uses
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants