rpc: sendmany and sendtoaddress return PSBT for wallets without private keys #18201

Sjors · 2020-02-24T16:34:05Z

Similar to what #16373 did for bumpfee, this PR lets sendmany and sendtoaddress return a PSBT for wallets without private keys:

Result:
"txid"                  (string) The transaction id.
{
  "psbt":    "psbt"      (string) The base64-encoded unsigned PSBT of the new transaction. Only returned when wallet private keys are disabled.
}

I consolidated code between these two RPC calls, since sendtoaddress is essentially sendmany with 1 destination. Unless I overlooked something, the only behaviour change is that some sendtoaddress error codes changed from -4 to -6. The release note mentions this.

The PSBT code is prepared for hardware wallet support (e.g. #16546). It checks if the PSBT is complete, which currently can't happen with watch-only wallets.

Closes #18180. Depends on #18115.

The logic of verifying a message was duplicated in 2 places: src/qt/signverifymessagedialog.cpp SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked() src/rpc/misc.cpp verifymessage() with the only difference being the result handling. Move the logic into a dedicated src/util/message.cpp MessageVerify() which returns a set of result codes, call it from the 2 places and just handle the results differently in the callers.

The logic of signing a message was duplicated in 3 places: src/qt/signverifymessagedialog.cpp SignVerifyMessageDialog::on_signMessageButton_SM_clicked() src/rpc/misc.cpp signmessagewithprivkey() src/wallet/rpcwallet.cpp signmessage() Move the logic into src/util/message.cpp MessageSign() and call it from all the 3 places.

And add unit test for it. The purpose of using a preamble or "magic" text as part of signing and verifying a message was not given when the code was repeated in a few locations. Make a test showing how it is used to prevent inadvertently signing a transaction.

…n function

…yMan

…tion

…ionwithwallet Instead of duplicating signing code, just use the function we already have.

…iptPubKeyMan::FillPSBT Instead of fetching a SigningProvider from ScriptPubKeyMan in order to fill and sign the keys and scripts for a PSBT, just pass that PSBT to a new FillPSBT function that does all that for us.

…allet and ScriptPubKeyMan Instead of getting a SigningProvider and then going to MessageSign, have ScriptPubKeyMan handle the message signing internally.

Not all ScriptPubKeyMans will be able to provide private keys, but pubkeys and scripts should be. So only provide public-only SigningProviders, i.e. ones that can help with Solving.

Make sure that there are no errors set for an input after it is signed. This is useful for when there are multiple ScriptPubKeyMans. Some may fail to sign, but one may be able to sign, and after it does, we don't want there to be any more errors there.

The only new behavior is some error codes are changed from -4 to -6.

In addition we check to see if the PSBT is complete, in preperation for a ScriptPubKeyMan that can use an external signer. PSBT for sendtoaddress

instagibbs · 2020-02-24T17:08:59Z

test/functional/wallet_watchonly.py

@@ -94,6 +98,14 @@ def run_test(self):
        assert_equal("psbt" in result, True)
        assert_raises_rpc_error(-4, "Insufficient funds", wo_wallet.walletcreatefundedpsbt, inputs, outputs, 0, no_wo_options)

+        self.log.info('Testing sendmany watch-only')


you should test that wallet balances aren't changing here(accidentally committing the transaction internally or something) and that it would enter the mempool properly

instagibbs

concept ACK, will more closely review once it's not 3 PRs high :)

luke-jr · 2020-02-24T17:35:20Z

Concept NACK, I think: don't we want such cases to call an external signer and behave as normally?

Sjors · 2020-02-24T18:34:33Z

After some IRC discussion, it seems better to create a new RPC after all, so I resurrected #16378

08fc6f6 [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost) Pull request description: I consolidated code between these two RPC calls, since `sendtoaddress` is essentially `sendmany` with 1 destination. Unless I overlooked something, the only behaviour change is that some `sendtoaddress` error codes changed from `-4` to `-6`. The release note mentions this. Salvaged from #18201. ACKs for top commit: fjahr: Code review ACK 08fc6f6 jonatack: ACK 08fc6f6 meshcollider: Code review & functional test run ACK 08fc6f6 Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909

… code 08fc6f6 [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost) Pull request description: I consolidated code between these two RPC calls, since `sendtoaddress` is essentially `sendmany` with 1 destination. Unless I overlooked something, the only behaviour change is that some `sendtoaddress` error codes changed from `-4` to `-6`. The release note mentions this. Salvaged from bitcoin#18201. ACKs for top commit: fjahr: Code review ACK 08fc6f6 jonatack: ACK 08fc6f6 meshcollider: Code review & functional test run ACK 08fc6f6 Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909

92326d8 [rpc] add send method (Sjors Provoost) 2c2a144 [rpc] add snake case aliases for transaction methods (Sjors Provoost) 1bc8d0f [rpc] walletcreatefundedpsbt: allow inputs to be null (Sjors Provoost) Pull request description: `walletcreatefundedpsbt` has some interesting features that `sendtoaddress` and `sendmany` don't have: * manual coin selection * outputting a PSBT (it was controversial to add this, see #18201) * create a transaction without adding to wallet (which leads to broadcasting, unless `-walletbroadcast=0`) At the same time `walletcreatefundedpsbt` can't broadcast a transaction, which is inconvenient for simple use cases. This PR introduces a new `send` RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can't sign all inputs, it outputs a PSBT. If `add_to_wallet` is set to `false` it will return the transaction in both PSBT and hex format. Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see #16546). For `bitcoin-cli` users, it tries to keep the simplest use case easy to use: ```sh bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b ``` This paves the way for deprecating `sendtoaddress` and `sendmany` though there's no rush. The only missing feature compared to these older methods is adding labels to a destination address. Depends on: - [x] #16377 (`[rpc] don't automatically append inputs in walletcreatefundedpsbt`) - [x] #11413 (`[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option`) - [x] #18244 (`[rpc] have lockUnspents also lock manually selected coins`) ACKs for top commit: meshcollider: Light re-utACK 92326d8 achow101: ACK 92326d8 Reviewed code and test, ran tests. kallewoof: utACK 92326d8 Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f

92326d8 [rpc] add send method (Sjors Provoost) 2c2a144 [rpc] add snake case aliases for transaction methods (Sjors Provoost) 1bc8d0f [rpc] walletcreatefundedpsbt: allow inputs to be null (Sjors Provoost) Pull request description: `walletcreatefundedpsbt` has some interesting features that `sendtoaddress` and `sendmany` don't have: * manual coin selection * outputting a PSBT (it was controversial to add this, see bitcoin#18201) * create a transaction without adding to wallet (which leads to broadcasting, unless `-walletbroadcast=0`) At the same time `walletcreatefundedpsbt` can't broadcast a transaction, which is inconvenient for simple use cases. This PR introduces a new `send` RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can't sign all inputs, it outputs a PSBT. If `add_to_wallet` is set to `false` it will return the transaction in both PSBT and hex format. Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see bitcoin#16546). For `bitcoin-cli` users, it tries to keep the simplest use case easy to use: ```sh bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b ``` This paves the way for deprecating `sendtoaddress` and `sendmany` though there's no rush. The only missing feature compared to these older methods is adding labels to a destination address. Depends on: - [x] bitcoin#16377 (`[rpc] don't automatically append inputs in walletcreatefundedpsbt`) - [x] bitcoin#11413 (`[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option`) - [x] bitcoin#18244 (`[rpc] have lockUnspents also lock manually selected coins`) ACKs for top commit: meshcollider: Light re-utACK 92326d8 achow101: ACK 92326d8 Reviewed code and test, ran tests. kallewoof: utACK 92326d8 Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f

vasild and others added 14 commits February 14, 2020 11:01

Refactor rawtransaction's SignTransaction into generic SignTransactio…

8b865be

…n function

Add SignTransaction function to ScriptPubKeyMan and LegacyScriptPubKe…

5ec7ec3

…yMan

Implement CWallet::SignTransaction using ScriptPubKeyMan::SignTransac…

1bb7939

…tion

Use CWallet::SignTransaction in CreateTransaction and signrawtransact…

6be1281

…ionwithwallet Instead of duplicating signing code, just use the function we already have.

Move FillPSBT to be a member of CWallet

e4314bf

Move direct calls to MessageSign into new SignMessage functions in CW…

d7df7b8

…allet and ScriptPubKeyMan Instead of getting a SigningProvider and then going to MessageSign, have ScriptPubKeyMan handle the message signing internally.

Replace GetSigningProvider with GetSolvingProvider

f2bd919

Not all ScriptPubKeyMans will be able to provide private keys, but pubkeys and scripts should be. So only provide public-only SigningProviders, i.e. ones that can help with Solving.

[rpc] refactor: consolidate sendmany and sendtoaddress code

7061c2c

The only new behavior is some error codes are changed from -4 to -6.

[rpc] sendmany, sendtoaddress: PSBT for wallets without privkeys

7421500

In addition we check to see if the PSBT is complete, in preperation for a ScriptPubKeyMan that can use an external signer. PSBT for sendtoaddress

Sjors requested a review from instagibbs February 24, 2020 17:02

instagibbs reviewed Feb 24, 2020

View reviewed changes

Sjors closed this Feb 24, 2020

This was referenced Feb 24, 2020

refactor: consolidate sendmany and sendtoaddress code #18202

Merged

The ultimate send RPC #16378

Merged

Sjors mentioned this pull request Apr 27, 2020

External signer support - Wallet Box edition #16546

Merged

2 tasks

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rpc: sendmany and sendtoaddress return PSBT for wallets without private keys #18201

rpc: sendmany and sendtoaddress return PSBT for wallets without private keys #18201

Uh oh!

Sjors commented Feb 24, 2020

Uh oh!

instagibbs Feb 24, 2020

Uh oh!

instagibbs left a comment

Uh oh!

luke-jr commented Feb 24, 2020

Uh oh!

Sjors commented Feb 24, 2020 •

edited

Loading

Uh oh!

Uh oh!

rpc: sendmany and sendtoaddress return PSBT for wallets without private keys #18201

rpc: sendmany and sendtoaddress return PSBT for wallets without private keys #18201

Uh oh!

Conversation

Sjors commented Feb 24, 2020

Uh oh!

instagibbs Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Feb 24, 2020

Uh oh!

Sjors commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Sjors commented Feb 24, 2020 •

edited

Loading