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

1. 
   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

2. 
   jonatack commented at 7:54 PM on February 16, 2021: member

   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

3. 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

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

   Included @jonatack's test

10. 
    jonatack commented at 9:08 PM on February 16, 2021: member

    ACK 6bfbc97d716faad38c87603ac6049d222236d623

11. 
    meshcollider commented at 10:06 PM on February 16, 2021: contributor

    utACK 6bfbc97d716faad38c87603ac6049d222236d623

12. 
    DrahtBot commented at 12:12 AM on February 17, 2021: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

13. kristapsk approved
14. 
    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.

15. Escobar181 approved
16. 
    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.'.

17. 
    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.

18. MarcoFalke added the label Needs backport (0.21) on Feb 18, 2021
19. MarcoFalke added this to the milestone 0.21.1 on Feb 18, 2021
20. 
    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.

21. meshcollider merged this on Feb 19, 2021
22. meshcollider closed this on Feb 19, 2021

    ---

23. MarcoFalke referenced this in commit d6b5eb5fcc on Feb 19, 2021
24. MarcoFalke referenced this in commit 4ef1e4bd40 on Feb 19, 2021
25. MarcoFalke removed the label Needs backport (0.21) on Feb 19, 2021
26. 
    MarcoFalke commented at 11:48 AM on February 19, 2021: member

    Backported in #20901

27. sidhujag referenced this in commit 528500d884 on Feb 19, 2021
28. DrahtBot locked this on Aug 16, 2022

Contributors

achow101jonatackmeshcolliderDrahtBotkristapskEscobar181MarcoFalke

Milestone
0.21.1