rpc: Allow walletprocesspsbt to sign without finalizing #22513

achow101 · 2021-07-21T02:16:25Z

It can be useful to sign an input with walletprocesspsbt but not finalize that input if it is complete. This PR adds another option to walletprocesspsbt to be able to do that. We will still finalize by default.

This does not materially change the PSBT workflow since finalizepsbt needs to be called in order to extract the tx for broadcast.

darosior

ACK 81a8c35

Here is a small test exercising the new logic: darosior@cef8042

DrahtBot · 2021-07-21T10:29:53Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23602 (wallet: Split stuff from rpcwallet by MarcoFalke)
#23578 (Add external signer taproot support by Sjors)
#22558 (psbt: Taproot fields for PSBT by achow101)
#22514 (psbt: Actually use SIGHASH_DEFAULT for PSBT signing by achow101)
#21283 (Implement BIP 370 PSBTv2 by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

theStack

Concept ACK

src/wallet/wallet.h

src/psbt.h

src/wallet/rpcwallet.cpp

laanwj · 2021-07-28T14:17:56Z

I'm not entirely sure whether it's better to use an options object here instead of the conventional interface with named parameters? The latter is easier to use from bitcoin-cli at least (can just use -named arg=value instead of having to wrap everything into JSON manually).

achow101 · 2021-07-28T18:05:57Z

I'm not entirely sure whether it's better to use an options object here instead of the conventional interface with named parameters? The latter is easier to use from bitcoin-cli at least (can just use -named arg=value instead of having to wrap everything into JSON manually).

I wonder if there is some syntactic sugar thing we can add that allows us to not have to write JSON manually.

ryanofsky · 2021-07-28T20:22:15Z

I wonder if there is some syntactic sugar thing we can add that allows us to not have to write JSON manually.

If by syntactic sugar you mean a client-only change, then bitcoin-cli could accept a lighter weight YAML-like syntax and turn it into JSON. But this seems it would be a headache to maintain and a new thing users would have to learn to take advantage of.

I think a better approach, given that we already have a syntax for positional arguments, and we already have a syntax for named arguments, is to just allow these existing syntaxes to be used together. #19762 is a client+server change which does this. And it benefits not just bitcoin-cli but also other RPC clients like the python client so you can call rpc.methodname(arg1, arg2, option=value) instead of rpc.methodname(arg1, arg2, {"option": value})

A third approach which Luke at one point liked and perhaps still likes is a server-only change which translates named parameters into options values: #11441 (IIRC similar behavior was also part of #11660).

achow101 · 2021-07-28T21:21:34Z

I've dropped the change to use the options object since it is orthogonal to the purpose of this PR. It seems like that will need more discussion too. I've opened issue #22575 for the discussion of what to do about RPCs with lots of positional arguments.

darosior

re-ACK 7f9310d

luke-jr

NACK with positional bool parameter. We shouldn't make the API worse than it already is.

We don't always want to finalize after signing, so make it possible to do that.

meshcollider

utACK a99ed89

meshcollider · 2021-09-30T01:55:47Z

test/functional/rpc_psbt.py

-        signed_tx = self.nodes[0].walletprocesspsbt(psbtx)['psbt']
+        signed_tx = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=False)['psbt']
+        finalized_tx = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=True)['psbt']
+        assert signed_tx != finalized_tx


Could also assert ['complete'] is not true before finalizing but is true afterwards

In that case the result documentation for complete should also be clarified: "If the transaction has a complete set of signatures and its inputs are finalized"

We don't always want to finalize after signing, so make it possible to do that. Github-Pull: bitcoin#22513 Rebased-From: a99ed89

Sjors

utACK a99ed89

Sjors · 2021-11-19T09:50:37Z

test/functional/rpc_psbt.py

-        signed_tx = self.nodes[0].walletprocesspsbt(psbtx)['psbt']
+        signed_tx = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=False)['psbt']
+        finalized_tx = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=True)['psbt']
+        assert signed_tx != finalized_tx


In that case the result documentation for complete should also be clarified: "If the transaction has a complete set of signatures and its inputs are finalized"

…alizing a99ed89 psbt: sign without finalizing (Andrew Chow) Pull request description: It can be useful to sign an input with `walletprocesspsbt` but not finalize that input if it is complete. This PR adds another option to `walletprocesspsbt` to be able to do that. We will still finalize by default. This does not materially change the PSBT workflow since `finalizepsbt` needs to be called in order to extract the tx for broadcast. ACKs for top commit: meshcollider: utACK a99ed89 Sjors: utACK a99ed89 Tree-SHA512: c88e5d3222109c5f4e763b1b9d97ce4655f68f2985a4509caab2d4e7f5bac5047328fd69696e82a330f5c5a333e0312568ae293515689b77a4747ca2f17caca6

…hout finalizing 46e07e2 psbt: sign without finalizing (Andrew Chow) Pull request description: It can be useful to sign an input with `walletprocesspsbt` but not finalize that input if it is complete. This PR adds another option to `walletprocesspsbt` to be able to do that. We will still finalize by default. This does not materially change the PSBT workflow since `finalizepsbt` needs to be called in order to extract the tx for broadcast. ACKs for top commit: meshcollider: utACK 46e07e2 Sjors: utACK 46e07e2 Tree-SHA512: c88e5d3222109c5f4e763b1b9d97ce4655f68f2985a4509caab2d4e7f5bac5047328fd69696e82a330f5c5a333e0312568ae293515689b77a4747ca2f17caca6

fanquake added the PSBT label Jul 21, 2021

darosior approved these changes Jul 21, 2021

View reviewed changes

DrahtBot mentioned this pull request Jul 21, 2021

psbt: Actually use SIGHASH_DEFAULT for PSBT signing #22514

Merged

theStack reviewed Jul 21, 2021

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jul 21, 2021

Implement BIP 370 PSBTv2 #21283

Open

luke-jr reviewed Jul 22, 2021

View reviewed changes

src/psbt.h Outdated Show resolved Hide resolved

luke-jr reviewed Jul 22, 2021

View reviewed changes

src/wallet/rpcwallet.cpp Show resolved Hide resolved

achow101 force-pushed the psbt-no-finalize branch 2 times, most recently from ae935a7 to 081de78 Compare July 24, 2021 02:31

This was referenced Jul 24, 2021

rpc, gui: bumpfee signer support #21576

Merged

Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

Merged

achow101 mentioned this pull request Jul 26, 2021

psbt: Taproot fields for PSBT #22558

Merged

achow101 force-pushed the psbt-no-finalize branch from 081de78 to 7f9310d Compare July 28, 2021 21:20

darosior approved these changes Jul 29, 2021

View reviewed changes

DrahtBot mentioned this pull request Aug 10, 2021

rpc: Add function walletestimatefee #22673

Closed

luke-jr suggested changes Sep 11, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 27, 2021

Ensure wallet is unlocked before signing PSBT with walletprocesspsbt and GUI #23106

Merged

DrahtBot added the Needs rebase label Sep 28, 2021

psbt: sign without finalizing

a99ed89

We don't always want to finalize after signing, so make it possible to do that.

achow101 force-pushed the psbt-no-finalize branch from 7f9310d to a99ed89 Compare September 28, 2021 23:13

DrahtBot removed the Needs rebase label Sep 29, 2021

meshcollider reviewed Sep 30, 2021

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021

psbt: sign without finalizing

ee19f41

We don't always want to finalize after signing, so make it possible to do that. Github-Pull: bitcoin#22513 Rebased-From: a99ed89

DrahtBot mentioned this pull request Nov 14, 2021

wallet: Avoid underpaying transaction fees when signing taproot spends #23502

Closed

Sjors reviewed Nov 19, 2021

View reviewed changes

This was referenced Nov 23, 2021

Add external signer taproot support #23578

Merged

wallet: Split stuff from rpcwallet #23602

Merged

laanwj merged commit 383d350 into bitcoin:master Nov 29, 2021

bitcoin locked and limited conversation to collaborators Nov 29, 2022

rpc: Allow walletprocesspsbt to sign without finalizing #22513

rpc: Allow walletprocesspsbt to sign without finalizing #22513

Uh oh!

Conversation

achow101 commented Jul 21, 2021

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laanwj commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jul 28, 2021

Uh oh!

ryanofsky commented Jul 28, 2021

Uh oh!

achow101 commented Jul 28, 2021

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jul 21, 2021 •

edited

Loading

laanwj commented Jul 28, 2021 •

edited

Loading