rpc: sendmany and sendtoaddress return PSBT for wallets without private keys#18201
Closed
Sjors wants to merge 14 commits intobitcoin:masterfrom
Closed
rpc: sendmany and sendtoaddress return PSBT for wallets without private keys#18201Sjors wants to merge 14 commits intobitcoin:masterfrom
Sjors wants to merge 14 commits intobitcoin:masterfrom
Conversation
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.
…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
reviewed
Feb 24, 2020
| 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') |
Member
There was a problem hiding this comment.
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
reviewed
Feb 24, 2020
Member
instagibbs
left a comment
There was a problem hiding this comment.
concept ACK, will more closely review once it's not 3 PRs high :)
Member
|
Concept NACK, I think: don't we want such cases to call an external signer and behave as normally? |
Member
Author
|
After some IRC discussion, it seems better to create a new RPC after all, so I resurrected #16378 |
This was referenced Feb 24, 2020
Merged
2 tasks
meshcollider
added a commit
that referenced
this pull request
Jul 12, 2020
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
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 12, 2020
… 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
meshcollider
added a commit
that referenced
this pull request
Sep 15, 2020
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
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 15, 2020
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Similar to what #16373 did for
bumpfee, this PR letssendmanyandsendtoaddressreturn a PSBT for wallets without private keys:I consolidated code between these two RPC calls, since
sendtoaddressis essentiallysendmanywith 1 destination. Unless I overlooked something, the only behaviour change is that somesendtoaddresserror codes changed from-4to-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.