wallet: Fix ZapSelectTx to sync wallet spends#18850
wallet: Fix ZapSelectTx to sync wallet spends#18850meshcollider merged 1 commit intobitcoin:masterfrom bvbfan:zapfix
Conversation
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
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
Signed-off-by: Anthony Fieroni <[email protected]>
maflcko
changed the title
|
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. |
There was a problem hiding this comment.
ACK 9c59f9c tested rebased on current master b33136b 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.
The motivation is |
Thanks, makes sense. It looks like something like this could happen in MarkConflicted: stray entry in mapTxSpends here: Line 947 in d77170d could lead to a crash here: Line 1075 in d77170d So I think it would be good to merge this PR |
|
ACK 9c59f9c |
9c59f9c Fix ZapSelectTx to sync wallet spends (Anthony Fieroni) Pull request description: Signed-off-by: Anthony Fieroni <[email protected]> ACKs for top commit: achow101: ACK 9c59f9c ryanofsky: Code review ACK 9c59f9c. Only change since last review tweaking the for loop as suggested jonatack: ACK 9c59f9c tested rebased on current master b33136b and the new unit test does indeed fail without the change. meshcollider: utACK 9c59f9c Tree-SHA512: 71672a5ab0c659550c3a40577614ea896412b79566b5672636ab18765e4c71b9d0a990d94dc6b6e623b03a05737022b04026b5699438809c7c54782d0fd0a5d2
Signed-off-by: Anthony Fieroni <[email protected]> Github-Pull: bitcoin#18850 Rebased-From: 9c59f9c
Summary: PR18850: > 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. PR19493 fixes clang build on Mac OS This is a backport of [[bitcoin/bitcoin#18850 | core#18850]] and [[bitcoin/bitcoin#19493 | core#19493]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9989
7 participants
Signed-off-by: Anthony Fieroni [email protected]