Skip to content

rpc: Disallow sendtoaddress and sendmany when private keys disabled#21201

Merged
meshcollider merged 2 commits into
bitcoin:masterfrom
achow101:fix-sendmoney
Feb 19, 2021
Merged

rpc: Disallow sendtoaddress and sendmany when private keys disabled#21201
meshcollider merged 2 commits into
bitcoin:masterfrom
achow101:fix-sendmoney

Conversation

@achow101

Copy link
Copy Markdown
Member

Since sendtoaddress and sendmany (which use the SendMoney function) create and commit a transaction, they should not do anything when the wallet does not have private keys. Otherwise a valid transaction cannot be made.

Fixes #21104

@jonatack jonatack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK modulo test coverage. Here's a regression test commit that fails on master and passes with this patch jonatack@15a9bcc

Comment thread src/wallet/rpcwallet.cpp Outdated
@achow101

Copy link
Copy Markdown
Member Author

Included @jonatack's test

@jonatack jonatack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6bfbc97

@meshcollider meshcollider left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 6bfbc97

@DrahtBot

DrahtBot commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

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

Conflicts

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.

@kristapsk kristapsk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6bfbc97. "Error: Private keys are disabled for this wallet" is definitely a better error message than "Insufficient funds" here. Hopefully change of error code from -6 to -4 doesn't break any software using Bitcoin JSON-RPC API.

@maflcko

maflcko commented Feb 18, 2021

Copy link
Copy Markdown
Member

You forgot to fix send?

Currently prints error message: 'Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.'.

@achow101

Copy link
Copy Markdown
Member Author

You forgot to fix send?

Currently prints error message: 'Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.'.

No, that's intended and expected if there are no change addresses. send is supposed to work when private keys are disabled; in that case, it will create and output a psbt.

@maflcko maflcko added this to the 0.21.1 milestone Feb 18, 2021
@maflcko

maflcko commented Feb 18, 2021

Copy link
Copy Markdown
Member

Ah, ok thx.

Also, tagged for 0.21.1 backport. Let me know if this shouldn't be backported.

@meshcollider meshcollider merged commit 3a2d5bf into bitcoin:master Feb 19, 2021
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Feb 19, 2021
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Feb 19, 2021
@maflcko

maflcko commented Feb 19, 2021

Copy link
Copy Markdown
Member

Backported in #20901

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 19, 2021
…ivate keys disabled

6bfbc97 test: disallow sendtoaddress/sendmany when private keys disabled (Jon Atack)
0997019 Disallow sendtoaddress and sendmany when private keys disabled (Andrew Chow)

Pull request description:

  Since `sendtoaddress` and `sendmany` (which use the `SendMoney` function) create and commit a transaction, they should not do anything when the wallet does not have private keys. Otherwise a valid transaction cannot be made.

  Fixes bitcoin#21104

ACKs for top commit:
  jonatack:
    ACK 6bfbc97
  meshcollider:
    utACK 6bfbc97
  kristapsk:
    ACK 6bfbc97. "Error: Private keys are disabled for this wallet" is definitely a better error message than "Insufficient funds" here. Hopefully change of error code from -6 to -4 doesn't break any software using Bitcoin JSON-RPC API.

Tree-SHA512: f277d6b5252e43942d568614032596f2c0827f00cd0cb71e44ffcb9822bfb15a71730a3e3688f31e59ba4eb7d275250c4e65ad4b6b3e96be6314c56a672432fb
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Watchonly descriptor wallets classifying funds as mine, are "spendable"

7 participants