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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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,
nit: could also add doxygen documentation (a few lines above) for the newly introduced parameter
Done
Concept ACK
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);
Please take more care when changing function signatures. This could silently cast a SignatureData* to a bool when merging another PR created prior to the API change.
Changed to the last parameter
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"},
Can you make an options Object instead of another series of booleans?
Done
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'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).
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.
re-ACK 7f9310db58d77571adc04c6e6d48f5d11fa44c8c
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.
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
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"
utACK a99ed8986554fa1ecc854e43ea373d957e598db8
utACK a99ed89