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

PSBT RPC calls #3775

Merged
merged 8 commits into from
Jun 29, 2020
Merged

PSBT RPC calls #3775

merged 8 commits into from
Jun 29, 2020

Conversation

niftynei
Copy link
Collaborator

@niftynei niftynei commented Jun 16, 2020

These are the base RPC calls that we'll need for constructing a transaction iteratively. As a PR, this is built ontop of #3762. New commits start at 569efe8

Added RPC methods:

  • reserveinputs
  • unreserveinputs
  • signpsbt
  • sendpsbt

@niftynei
Copy link
Collaborator Author

Travis failures likely due to ElementsProject/libwally-core#203

@cdecker
Copy link
Member

cdecker commented Jun 23, 2020

Rebased on top of master since #3762 was merged.

Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

Quite a nice change, making the internal wallet a lot more like bitcoind's. Can we deprecate txprepare and build on this instead?

tests/test_wallet.py Show resolved Hide resolved
wallet/wallet.h Show resolved Hide resolved
wallet/walletrpc.c Outdated Show resolved Hide resolved
doc/lightning-reserveinputs.7.md Outdated Show resolved Hide resolved
doc/lightning-reserveinputs.7.md Outdated Show resolved Hide resolved
doc/lightning-unreserveinputs.7.md Outdated Show resolved Hide resolved
tal_hex(tmpctx, msg));

response = json_stream_success(cmd);
json_add_psbt(response, "signed_psbt", signed_psbt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add abool indiciating if it's ready for sendpsbt? (i.e. finalizable)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do later.

bitcoin/tx.c Show resolved Hide resolved
@rustyrussell
Copy link
Contributor

Ack, but needs rebase:

  1. Empty whitespace-only commit?
  2. Commit already in master which just adds a test?

But some of this may be GH weirdness...

@rustyrussell
Copy link
Contributor

Quite a nice change, making the internal wallet a lot more like bitcoind's. Can we deprecate txprepare and build on this instead?

We should do that once we have dual funding, because I'd rather not change interfaces twice...

Unused here, but we'll use it in the next commit so that we can always
pass back the effective / used feerate to the caller of `reserveinputs`

This makes opening a channel much easier if we've internally determined
the feerate
Reserve and unreserve wallet UTXOs using a PSBT which includes those
inputs.

Note that currently we unreserve inputs everytime the node restarts.
This will be addressed in a future commit.

Changelog-Added: JSON-RPC: Adds two new rpc methods, `reserveinputs` and `unreserveinputs`, which allow for reserving or unreserving wallet UTXOs
the bitcoin_tx version is basically a wrapper for the wally_tx script
extraction -- here we pull it apart so we can easily get a tal'd script
for a wally_tx_output
With the incursion of PSBTs, we're moving away from bitcoin_tx
Changelog-Added: JSON-RPC: new call `signpsbt` which will add the wallet's signatures to a provided psbt
Changelog-Added: JSON-RPC: new call `sendpsbt` which will finalize and send a signed PSBT
when attempting to calculate the fees for a tx where we don't own all of
the outputs, we can overshoot the feerate
Our existing coin_moves tracking logic assumed that any tx we had an
input in belonged to *all* of our wallet (not a bad assumption as long
as there was no way to update a tx that spends our wallets)

Now that we've got `signpsbt` implemented, however, we need to be
careful about how we account for withdrawals. For now we do a best guess
at what the feerate is, and lump all of our spent outputs as a
'withdrawal' when it's impossible to disambiguate
@niftynei
Copy link
Collaborator Author

Ack, but needs rebase:

Empty whitespace-only commit?
Commit already in master which just adds a test?

Ok, updated requested changes and removed/fixed the two commit weirdness that happened due to master rebase. (one removed, the other folded into the appropriate commit).

@cdecker
Copy link
Member

cdecker commented Jun 29, 2020

Looks like all the requested changes were implemented, except the cosmetic change to add fully signed flag in the signpsbt result. I'll nudge Travis to completion and merge asap.

ACK e101bc8

@cdecker cdecker merged commit 2e9c387 into ElementsProject:master Jun 29, 2020
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.

None yet

3 participants