Coin selection choosing prevouts spent in just-mined block

[buck54321]

There appears to be a race somewhere when it comes to coin locking and coin selection (CWallet::SelectCoins, I think).

Steps to reproduce

1. zcashd -regtest
2. zcash-cli -regtest generate 150
3. ADDR=`zcash-cli -regtest getnewaddress`; zcash-cli -regtest sendtoaddress $ADDR 1; zcash-cli -regtest generate 1; zcash-cli -regtest sendtoaddress $ADDR 1;

You may need to run step 3 a couple of times.

The error message is

Error: The transaction was rejected! This might happen if some of the coins in your wallet were already spent, such as if you used a copy of the wallet and coins were spent in the copy but not marked as spent here.

And I've managed to further identify the error as originating from

if (!view.HaveInputs(tx)) {
    return state.Invalid(false, REJECT_DUPLICATE, "bad-txns-inputs-spent");
}

and have additionally verified that the same outputs are being selected for the second tx as for the just-mined first tx.

On the other hand, if you put in a delay.

ADDR=`zcash-cli -regtest getnewaddress`; zcash-cli -regtest sendtoaddress $ADDR 1; zcash-cli -regtest generate 1; sleep 1s; zcash-cli -regtest sendtoaddress $ADDR 1;

it sends without error every time.

[buck54321]

A little more info about the issue.

It actually has nothing to do with coin "locking". Outputs spent in mempool are unselectable by AvailableCoins because there's a check

int nDepth = pcoin->GetDepthInMainChain();
if (nDepth < nMinDepth)
    continue;

where nMinDepth >= 0 by assertion and nDepth will be 0 because

if (!pindex || !chainActive.Contains(pindex))
    return 0;

Txs are removed from mempool in the ThreadMessageHandler in ConnectTip, right before chainActive.SetTip is called via UpdateTip.

That timing is important, because the transaction is marked as spent in the ThreadNotifyWallets, which does a 1 second poll for new blocks via the chainActive.Tip(). So the tx is guaranteed to be removed from mempool before ThreadNotifyWallets can see the block. And in the interim, which can be up to 1 second, the spent output will be selectable.

[str4d]

The problem here is that the wallet and node states are not synchronized at the time the wallet tries to select coins, and the reason they are not synchronized is that we explicitly do not want them synchronized, as that leads to a remote oracle attack on the wallet's shielded viewing keys (fixing this attack is why we introduced ThreadNotifyWallets in the first place).

There is always going to be some possible race condition between a wallet's state and the node's state, even without ThreadNotifyWallets, because the same spending keys can exist in multiple zcashd / wallet processes. The problem here occurs more easily however because the wallet, at coin selection time, is locking on both the wallet and node states, and previously could assume that doing so would ensure their states were synchronized. I don't see a way to fix this without introducing an entirely parallel "node state view" that the wallet maintains, so it can identify differences between that state and the node's current state. This is effectively what the light clients already implicitly have (zcash_client_sqlite stores chain and mempool state locally, which is periodically updated via communication with lightwalletd), but the current wallet architecture inherited from Bitcoin Core has no such concept. It might be the case that upstream have made some improvements here (#4949 is about backporting the interface they defined between the two, but I think that still assumed synchronicity), but I suspect that the work required to properly fix this is on the scale of building a wallet that can run in a separate process from zcashd.