wallet: Fix sendall with watchonly wallets and specified inputs #26344

1. 
   achow101 commented at 7:42 PM on October 19, 2022: member

   The sendall RPC would previously fail when used with a watchonly wallet and specified inputs. This failure was caused by checking isminetype equality with ISMINE_ALL rather than a bitwise AND as IsMine can never return ISMINE_ALL.

   Also added a test.

2. wallet: Correctly check ismine for sendall

   sendall should be using a bitwise AND for sendall's IsMine check rather
   than an equality as IsMine will never return ISMINE_ALL.

   6bcd7e2a3b
3. achow101 requested review from murchandamus on Oct 19, 2022
4. DrahtBot added the label Wallet on Oct 19, 2022
5. 
   DrahtBot commented at 9:49 PM on October 19, 2022: contributor

   <!--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:

   • #25375 (rpc: add minconf/maxconf options to sendall and fund transaction calls by ishaanam)

   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.

6. 
   fanquake commented at 12:29 AM on October 20, 2022: member

   Does this need backporting?

7. 
   achow101 commented at 12:56 AM on October 20, 2022: member

   Does this need backporting?

   Yes

8. achow101 added this to the milestone 24.0 on Oct 20, 2022
9. maflcko added the label Needs backport (24.x) on Oct 20, 2022
10. in src/wallet/rpc/spend.cpp:1382 in 6bcd7e2a3b outdated

    1378 | @@ -1379,7 +1379,7 @@ RPCHelpMan sendall()
    1379 |                          throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Input not available. UTXO (%s:%d) was already spent.", input.prevout.hash.ToString(), input.prevout.n));
    1380 |                      }
    1381 |                      const CWalletTx* tx{pwallet->GetWalletTx(input.prevout.hash)};
    1382 | -                    if (!tx || pwallet->IsMine(tx->tx->vout[input.prevout.n]) != (coin_control.fAllowWatchOnly ? ISMINE_ALL : ISMINE_SPENDABLE)) {
    1383 | +                    if (!tx || !(pwallet->IsMine(tx->tx->vout[input.prevout.n]) & (coin_control.fAllowWatchOnly ? ISMINE_ALL : ISMINE_SPENDABLE))) {
    

    ---

    ---

    

    furszy commented at 4:27 PM on October 20, 2022:

    Not related to this PR, but.. pretty sure that we can make the node crash by providing an out-of-bounds input prevout index.

    e.g.

    diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py
    --- a/test/functional/wallet_sendall.py	(revision d442101d85e1f89b072145345ea0bcf60d6ceacc)
    +++ b/test/functional/wallet_sendall.py	(date 1666283169723)
    @@ -294,6 +294,7 @@
             else:
                 watchonly.importmulti(import_req)
     
    +        utxo["vout"] = 10
             sendall_tx_receipt = watchonly.sendall(recipients=[self.remainder_target], options={"inputs": [utxo]})
             psbt = sendall_tx_receipt["psbt"]
             decoded = self.nodes[0].decodepsbt(psbt)
    

    ---

    

    achow101 commented at 5:26 PM on October 20, 2022:

    Fixed and added a test.

11. furszy approved
12. 
    furszy commented at 4:30 PM on October 20, 2022: member

    Code review ACK d442101d

    With an extra crash finding that doesn't necessarily need to be included here.

13. test: Test that sendall works with watchonly spending specific utxos 708b72b715
14. wallet: Check utxo prevout index out of bounds in sendall b132c85650
15. test: Test for out of bounds vout in sendall 315fd4dbab
16. achow101 force-pushed on Oct 20, 2022
17. w0xlt approved
18. 
    w0xlt commented at 5:35 PM on October 20, 2022: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/26344/commits/315fd4dbabb6b631b755811742a3bdf93e1241bf

    Commits can be squashed.

19. 
    achow101 commented at 6:39 PM on October 20, 2022: member

    Commits can be squashed.

    I prefer to keep implementation separate from test so that tests can be easily cherry-picked onto master to verify that they fail.

20. furszy approved
21. 
    furszy commented at 8:10 PM on October 20, 2022: member

    ACK 315fd4db

22. fanquake merged this on Oct 21, 2022
23. fanquake closed this on Oct 21, 2022

    ---

24. sidhujag referenced this in commit 710f2043a2 on Oct 23, 2022
25. fanquake referenced this in commit bbe864a13a on Oct 28, 2022
26. fanquake referenced this in commit 931db785ee on Oct 28, 2022
27. fanquake referenced this in commit dedee6af57 on Oct 28, 2022
28. fanquake referenced this in commit b04f5f9608 on Oct 28, 2022
29. fanquake removed the label Needs backport (24.x) on Oct 28, 2022
30. 
    fanquake commented at 10:09 AM on October 28, 2022: member

    Backported in #26410.

31. fanquake referenced this in commit 2e8880abc0 on Oct 31, 2022
32. bitcoin locked this on Oct 28, 2023

Contributors

achow101DrahtBotfanquakefurszyw0xlt

Review Requested
murchandamus

Milestone
24.0