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.
sendall should be using a bitwise AND for sendall's IsMine check rather
than an equality as IsMine will never return ISMINE_ALL.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Does this need backporting?
Yes
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))) {
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.
0diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py
1--- a/test/functional/wallet_sendall.py (revision d442101d85e1f89b072145345ea0bcf60d6ceacc)
2+++ b/test/functional/wallet_sendall.py (date 1666283169723)
3@@ -294,6 +294,7 @@
4 else:
5 watchonly.importmulti(import_req)
6
7+ utxo["vout"] = 10
8 sendall_tx_receipt = watchonly.sendall(recipients=[self.remainder_target], options={"inputs": [utxo]})
9 psbt = sendall_tx_receipt["psbt"]
10 decoded = self.nodes[0].decodepsbt(psbt)
Code review ACK d442101d
With an extra crash finding that doesn’t necessarily need to be included here.
ACK https://github.com/bitcoin/bitcoin/pull/26344/commits/315fd4dbabb6b631b755811742a3bdf93e1241bf
Commits can be squashed.
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.
