Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from Feb 19, 2021
Merged

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

merged 2 commits into from Feb 19, 2021

Conversation

achow101
Copy link
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

@achow101 achow101 mentioned this pull request Feb 16, 2021
Closed
jonatack
jonatack reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@jonatack jonatack left a comment

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

jonatack
jonatack reviewed Feb 16, 2021
View reviewed changes
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@achow101
Copy link
Member Author

Included @jonatack's test

jonatack
jonatack reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK 6bfbc97

meshcollider
meshcollider reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

utACK 6bfbc97

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 17, 2021 •
edited

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.

This was referenced Feb 17, 2021
Merged
Merged
kristapsk
kristapsk approved these changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@kristapsk kristapsk left a comment

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
Copy link
Member

maflcko commented Feb 18, 2021

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
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
Copy link
Member

maflcko commented Feb 18, 2021

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
@achow101
Disallow sendtoaddress and sendmany when private keys disabled
d6b5eb5 
Github-Pull: bitcoin#21201
Rebased-From: 0997019
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Feb 19, 2021
@jonatack
test: disallow sendtoaddress/sendmany when private keys disabled
4ef1e4b 
Github-Pull: bitcoin#21201
Rebased-From: 6bfbc97
@maflcko
Copy link
Member

maflcko commented Feb 19, 2021

Backported in #20901

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 19, 2021
@meshcollider
Merge bitcoin#21201: rpc: Disallow sendtoaddress and sendmany when pr…
528500d 
…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.
Reviewers

@jonatack jonatack jonatack left review comments

@meshcollider meshcollider meshcollider left review comments

@kristapsk kristapsk kristapsk approved these changes

@Escobar181 Escobar181 Escobar181 approved these changes

Assignees
No one assigned
Labels
RPC/REST/ZMQ Wallet
Projects
None yet
Milestone
0.21.1
Development

Successfully merging this pull request may close these issues.

Watchonly descriptor wallets classifying funds as mine, are "spendable"
7 participants
@achow101 @DrahtBot @maflcko @jonatack @meshcollider @kristapsk @Escobar181