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.
ACK 81a8c35855
Here is a small test exercising the new logic: https://github.com/darosior/bitcoin/commit/cef80424312f159671977d5152444af4bfbbef41
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
583@@ -584,6 +584,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
584 int sighash_type = 1 /* SIGHASH_ALL */,
585 bool sign = true,
586 bool bip32derivs = true,
587+ bool finalize = true,
577@@ -578,7 +578,7 @@ bool PSBTInputSigned(const PSBTInput& input);
578 * txdata should be the output of PrecomputePSBTData (which can be shared across
579 * multiple SignPSBTInput calls). If it is nullptr, a dummy signature will be created.
580 **/
581-bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash = SIGHASH_ALL, SignatureData* out_sigdata = nullptr);
582+bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction& psbt, int index, const PrecomputedTransactionData* txdata, int sighash = SIGHASH_ALL, bool finalize = true, SignatureData* out_sigdata = nullptr);
SignatureData* to a bool when merging another PR created prior to the API change.
4314@@ -4315,6 +4315,7 @@ static RPCHelpMan walletprocesspsbt()
4315 " \"NONE|ANYONECANPAY\"\n"
4316 " \"SINGLE|ANYONECANPAY\""},
4317 {"bip32derivs", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include BIP 32 derivation paths for public keys if we know them"},
4318+ {"finalize", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also finalize inputs if possible"},
options Object instead of another series of booleans?
bitcoin-cli at least (can just use -named arg=value instead of having to wrap everything into JSON manually).
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-cliat least (can just use-named arg=valueinstead 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.
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).
We don't always want to finalize after signing, so make it possible to
do that.
118@@ -119,7 +119,9 @@ def run_test(self):
119 self.nodes[0].walletpassphrase(passphrase="password", timeout=1000000)
120
121 # Sign the transaction and send
122- signed_tx = self.nodes[0].walletprocesspsbt(psbtx)['psbt']
123+ signed_tx = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=False)['psbt']
124+ finalized_tx = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=True)['psbt']
125+ assert signed_tx != finalized_tx
['complete'] is not true before finalizing but is true afterwards
complete should also be clarified: “If the transaction has a complete set of signatures and its inputs are finalized”