wallet: Fix ZapSelectTx to sync wallet spends #18850

bvbfan commented at 9:32 AM on May 2, 2020: contributor

Signed-off-by: Anthony Fieroni bvbfan@abv.bg

fanquake added the label Wallet on May 2, 2020

in src/wallet/wallet.cpp:3082 in a04387448f outdated

3075 | @@ -3076,9 +3076,11 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
3076 |  {
3077 |      AssertLockHeld(cs_wallet);
3078 |      DBErrors nZapSelectTxRet = WalletBatch(*database, "cr+").ZapSelectTx(vHashIn, vHashOut);
3079 | -    for (uint256 hash : vHashOut) {
3080 | +    for (const uint256& hash : vHashOut) {
3081 |          const auto& it = mapWallet.find(hash);
3082 |          wtxOrdered.erase(it->second.m_it_wtxOrdered);
3083 | +        for (auto& txin : it->second.tx->vin)

ryanofsky commented at 10:49 PM on May 6, 2020:

const auto& here would clarify txin isn't changing

Code style guidelines also ask for for loop braces: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

ryanofsky approved

ryanofsky commented at 11:09 PM on May 6, 2020: member

Code review ACK a04387448f0bf4eddd27b905f554608b6ba3ed00. Code change is simple, and thank you for writing a test!

Logically the change makes sense, but I'd be curious to know what motivation is for the PR (it would be good to expand PR description). It doesn't seem like this going to effect behavior in most cases because map is just keeping track of wallet transactions spending wallet coins, which shouldn't matter if the transactions and coins aren't in the wallet. If there were a python functional test showing where this made a difference, that might motivate and explain the change a little better.

Either way, the change does seem like it is reasonable.

Travis failure sync_mempools timeout in mempool_unbroadcast.py https://travis-ci.org/github/bitcoin/bitcoin/jobs/682269091#L3122 seems spurious and is probably unrelated

Fix ZapSelectTx to sync wallet spends

Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>

9c59f9c285

ryanofsky approved

ryanofsky commented at 9:02 PM on May 13, 2020: member

Code review ACK 9c59f9c285303659ee1beed7555bbb322e6e6981. Only change since last review tweaking the for loop as suggested

MarcoFalke renamed this:
~~Fix ZapSelectTx to sync wallet spends~~
wallet: Fix ZapSelectTx to sync wallet spends
on May 13, 2020

adamjonas commented at 7:05 PM on June 11, 2020: member

Compiled and ran tests. @bvbfan, it'd be great if you could expand the PR description to include the motivation and/or include a functional test to better explain this change as requested above.

jonatack commented at 8:28 PM on June 11, 2020: member

ACK 9c59f9c285303659ee1beed7555bbb322e6e6981 tested rebased on current master b33136b6ba9887f7d and the new unit test does indeed fail without the change.

wallet/test/wallet_tests.cpp(793): Entering test case "ZapSelectTx"
wallet/test/wallet_tests.cpp(819): error: in "wallet_tests/ZapSelectTx": check !wallet->HasWalletSpend(prev_hash) has failed

Logically the change makes sense, but I'd be curious to know what motivation is for the PR (it would be good to expand PR description). It doesn't seem like this going to effect behavior in most cases because map is just keeping track of wallet transactions spending wallet coins, which shouldn't matter if the transactions and coins aren't in the wallet. If there were a python functional test showing where this made a difference, that might motivate and explain the change a little better.

Agree with @ryanofsky and @adamjonas here.

bvbfan commented at 5:23 AM on June 12, 2020: contributor

Compiled and ran tests. @bvbfan, it'd be great if you could expand the PR description to include the motivation and/or include a functional test to better explain this change as requested above.

The motivation is mapWallet is used, in many places, without check (method at expects that hash presents) but that's not exactly true when mapTxSpends and mapWallet are out of sync. That can lead to crash/data loss in wallet.

ryanofsky commented at 2:28 PM on July 2, 2020: member

The motivation is mapWallet is used, in many places, without check (method at expects that hash presents) but that's not exactly true when mapTxSpends and mapWallet are out of sync. That can lead to crash/data loss in wallet.

Thanks, makes sense. It looks like something like this could happen in MarkConflicted: stray entry in mapTxSpends here:

https://github.com/bitcoin/bitcoin/blob/d77170d52687c3d708daffb7976c3e0d345ff7dc/src/wallet/wallet.cpp#L947

could lead to a crash here:

https://github.com/bitcoin/bitcoin/blob/d77170d52687c3d708daffb7976c3e0d345ff7dc/src/wallet/wallet.cpp#L1075

So I think it would be good to merge this PR