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

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:fix-sendmoney changing 2 files +13 −2

achow101 commented at 6:30 PM on February 16, 2021: 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 cross-referenced this on Feb 16, 2021 from issue Watchonly descriptor wallets classifying funds as `mine`, are "spendable" by niftynei
jonatack commented at 7:54 PM on February 16, 2021: contributor

ACK modulo test coverage. Here's a regression test commit that fails on master and passes with this patch https://github.com/jonatack/bitcoin/commit/15a9bcc75d5447d85b614e2e5595d7a9a8b423a6

in src/wallet/rpcwallet.cpp:418 in e19b3c803d outdated

 414 | @@ -409,7 +415,7 @@ UniValue SendMoney(CWallet* const pwallet, const CCoinControl &coin_control, std
 415 |      bilingual_str error;
 416 |      CTransactionRef tx;
 417 |      FeeCalculation fee_calc_out;
 418 | -    bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
 419 | +    bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, true);

jonatack commented at 7:56 PM on February 16, 2021:

nit while touching this line, feel free to ignore

    const bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, true);

achow101 commented at 8:49 PM on February 16, 2021:

Done

DrahtBot added the label RPC/REST/ZMQ on Feb 16, 2021
DrahtBot added the label Wallet on Feb 16, 2021
Disallow sendtoaddress and sendmany when private keys disabled 0997019e76
test: disallow sendtoaddress/sendmany when private keys disabled 6bfbc97d71
achow101 force-pushed on Feb 16, 2021
achow101 commented at 8:49 PM on February 16, 2021: member

Included @jonatack's test
jonatack commented at 9:08 PM on February 16, 2021: contributor

ACK 6bfbc97d716faad38c87603ac6049d222236d623
meshcollider commented at 10:06 PM on February 16, 2021: contributor

utACK 6bfbc97d716faad38c87603ac6049d222236d623
DrahtBot commented at 12:12 AM on February 17, 2021: 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:
- #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
- #20640 (wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack)
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.
DrahtBot cross-referenced this on Feb 17, 2021 from issue wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack
DrahtBot cross-referenced this on Feb 17, 2021 from issue refactor: Make CWalletTx sync state type-safe by ryanofsky
kristapsk approved
kristapsk commented at 12:24 AM on February 18, 2021: contributor

ACK 6bfbc97d716faad38c87603ac6049d222236d623. "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.
Escobar181 approved
MarcoFalke commented at 8:28 AM on February 18, 2021: 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 commented at 6:22 PM on February 18, 2021: 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.'.

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.
MarcoFalke added the label Needs backport (0.21) on Feb 18, 2021
MarcoFalke added this to the milestone 0.21.1 on Feb 18, 2021
MarcoFalke commented at 6:34 PM on February 18, 2021: member

Ah, ok thx.

Also, tagged for 0.21.1 backport. Let me know if this shouldn't be backported.
meshcollider merged this on Feb 19, 2021
meshcollider closed this on Feb 19, 2021
MarcoFalke referenced this in commit d6b5eb5fcc on Feb 19, 2021
MarcoFalke referenced this in commit 4ef1e4bd40 on Feb 19, 2021
MarcoFalke removed the label Needs backport (0.21) on Feb 19, 2021
MarcoFalke commented at 11:48 AM on February 19, 2021: member

Backported in #20901
sidhujag referenced this in commit 528500d884 on Feb 19, 2021
bitcoin locked this on Aug 16, 2022