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

walletrpc: add new PSBT creation+signing RPCs #4389

Merged
merged 10 commits into from
Oct 3, 2020

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Jun 18, 2020

Fixes #3938.
Fixes #4400.

Depends on btcsuite/btcutil#178 and btcsuite/btcwallet#711.

EDIT: Depends on btcsuite/btcwallet#721.

@guggero guggero requested review from Roasbeef and wpaulino June 18, 2020 14:16
@guggero guggero force-pushed the psbt-signing branch 3 times, most recently from 1881600 to b20bc35 Compare June 22, 2020 16:58
lnwallet/chanfunding/psbt_assembler.go Show resolved Hide resolved
lnwallet/interface.go Show resolved Hide resolved
lnrpc/rest-annotations.yaml Show resolved Hide resolved
"locket UTXO", idx)
}

// Everything's OK, we found the UTXO and can add its
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this already done within the btcwallet methods? If not, then why not?

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're right, most of this is already done by the wallet. The only thing to check here is that the inputs aren't spent or locked yet.

lnrpc/walletrpc/walletkit.proto Show resolved Hide resolved
cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
cmd/lncli/walletrpc_active.go Show resolved Hide resolved
lntest/itest/psbt.go Show resolved Hide resolved
@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour psbt rpc Related to the RPC interface wallet The wallet (lnwallet) which LND uses labels Jun 23, 2020
@guggero guggero force-pushed the psbt-signing branch 4 times, most recently from 0040f5c to 41f780a Compare June 26, 2020 17:13
@guggero guggero marked this pull request as ready for review June 26, 2020 17:14
@guggero guggero requested a review from halseth as a code owner June 26, 2020 17:14
@guggero guggero added this to the 0.11.0 milestone Jun 26, 2020
@guggero guggero added the v0.11 label Jun 26, 2020
@guggero guggero removed the request for review from halseth June 26, 2020 17:15
@Roasbeef Roasbeef modified the milestones: 0.11.0, 0.12.0 Jun 30, 2020
@wpaulino wpaulino added v0.12 and removed v0.11 labels Jul 2, 2020
@Roasbeef Roasbeef requested a review from cfromknecht as a code owner July 13, 2020 23:06
Copy link
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.

Looks pretty good! Will begin playing around with it locally and report any issues.

lnrpc/walletrpc/walletkit.proto Show resolved Hide resolved

expiration, err := w.LeaseOutput(lock.lockID, lock.outpoint)
if err != nil {
return nil, fmt.Errorf("could not lease a lock on "+
Copy link
Contributor

Choose a reason for hiding this comment

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

What @Roasbeef was referring to was to defer at the level of the caller of lockInputs, as an error can occur in some other method after the fact and the inputs wouldn't be released. There isn't a method that's called after lockInputs that returns an error at the moment, but it's probably still worth making the change to prevent any potential bugs in the future.

@guggero guggero force-pushed the psbt-signing branch 3 times, most recently from 0412ffd to 1e460b7 Compare September 16, 2020 13:51
@wpaulino wpaulino self-requested a review September 17, 2020 22:43
Copy link
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.

@guggero nice work! lot of good stuff in here :)

lnrpc/walletrpc/walletkit_server.go Show resolved Hide resolved

expiration, err := w.LeaseOutput(lock.lockID, lock.outpoint)
if err != nil {
return nil, fmt.Errorf("could not lease a lock on "+
Copy link
Contributor

Choose a reason for hiding this comment

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

another option would be to make a LeaseOutputs and ReleaseOutputs that lock all of them atomically

cmd/lncli/walletrpc_active.go Outdated Show resolved Hide resolved
lntest/itest/lnd_psbt_test.go Outdated Show resolved Hide resolved
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.

Only thing I think this is missing is some light doc updates, but that's totally non-blocking, and this PR has been in stasis for quite a while.

LGTM 🎨

cmd/lncli/walletrpc_active.go Show resolved Hide resolved
@guggero
Copy link
Collaborator Author

guggero commented Oct 1, 2020

DON'T MERGE, just discovered a bug in the fee estimation if more than one input is selected (good thing I started writing the docs for this and coming up with useful examples 😅 )

EDIT: Fix is up: btcsuite/btcwallet#721.

@guggero
Copy link
Collaborator Author

guggero commented Oct 1, 2020

Added some documentation with examples and fixed the change amount bug.

Copy link
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.

🎉

@wpaulino
Copy link
Contributor

wpaulino commented Oct 2, 2020

@guggero needs a rebase, but it should be good to merge once addressed.

guggero added 10 commits October 3, 2020 10:34
The internal lock ID that the wallet kit subserver uses to lock inputs
for itself shouldn't be allowed to be used when locking inputs manually
over the RPC.
We rewrite the test to use the require library to make it a
bit more condensed.
Now that we have all functions that we need to complete the whole
PSBT channel funding flow, we change the itest to use Dave's wallet
to fund the channel from Carol to Dave through a PSBT.
@guggero guggero merged commit f8e3b41 into lightningnetwork:master Oct 3, 2020
@guggero guggero deleted the psbt-signing branch October 3, 2020 09:48
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 psbt rpc Related to the RPC interface v0.12 wallet The wallet (lnwallet) which LND uses
Projects
None yet
4 participants