wallet: ScanForWalletTransactions cleanup #19195

pstratem · 2020-06-06T21:20:30Z

consolidated findNextBlock calls and reorg handling

logic isn't changed except in the case that a reorg has occured we always set the FAILURE flag previously the FAILURE flag would only be set if a reorg occured and findBlock failed.

…tions

DrahtBot · 2020-06-09T10:53:47Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Get rid of more redundant chain methods #19425 (refactor: Get rid of more redundant chain methods by ryanofsky)

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.

ryanofsky

Concept ACK for this cleanup because ScanForWalletTransactions is more messy and complicated than it needs to be. But would be cautious about spending too much effort improving this, because I think longer term with ariard's changes #11756 (comment) we would like to move scanning outside the wallet and drop ScanForWalletTransactions entirely

ryanofsky · 2020-06-30T22:07:43Z

src/wallet/wallet.cpp

+        }
        if (chain().findBlock(block_hash, FoundBlock().data(block)) && !block.IsNull()) {
            LOCK(cs_wallet);
-            next_block = chain().findNextBlock(block_hash, block_height, FoundBlock().hash(next_block_hash), &reorg);


In commit "wallet: Simplify logic in ScanForWalletTransactions" (8894da2)

I don't think it's the best thing to move this findNextBlock before the findBlock call. Reason it was written this way is that findBlock call is slow (it requires reading and deserializing the block from disk). With this change, reorgs during findBlock will go undetected so this can wind up scanning a block that's been reorged out, or choosing a stale next block after a reorg and failing where it could have succeeded.

Maybe all of this matters less after #16426. After #16426, cs_main isn't held in this function so reorgs can happen any time, not just during findBlock. But it still seems more reliable to wait for the next block to actually be needed before looking it up, instead of looking it up early before a slow operation where it could get out of date and cause unnecessary failures.

EDIT: Fixed references to #16426 (not #15719)

ryanofsky · 2020-06-30T22:46:43Z

src/wallet/wallet.cpp

+            // break successfully when reorg occurs, reorg will scan blocks for transactions
            // https://github.com/bitcoin/bitcoin/pull/14711#issuecomment-458342518
-            result.last_failed_block = block_hash;
-            result.status = ScanResult::FAILURE;


In commit "wallet: resolve inconsistent ScanForWalletTransactions result" (e526bf7)

I think the TODO comment which this PR implements is actually wrong. The TODO comment says it's fine to return success instead of failure when there is a reorg, because if there was a reorg the later blocks after the reorg will have already been scanned.

But actually it's not sufficient if timing was that later blocks were scanned before earlier blocks. In that case, the wallet could miss transactions in the later blocks due to the IsFromMe check in AddToWalletIfInvolvingMe. So if there was a reorg here, this actually should return failure not success.

re: #19195 (comment)

I think the TODO comment which this PR implements is actually wrong.

Removing TODO comment in #19425

This just drops two interfaces::Chain methods replacing them with other calls. Motivation for removing these chain methods: - Need to get rid of findFirstBlockWithTimeAndHeight for bitcoin#10102, which doesn't support overloaded methods - Followup from bitcoin#16426 (comment) - phantomcircuit comments about findNextBlock test http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-06.html#l-214 Behavior is not changing in any way here. A TODO comment in ScanForWalletTransactions was removed, but just because it was invalid (see bitcoin#19195 (comment)), not because it was implemented.

This just drops three interfaces::Chain methods replacing them with other calls. Motivation for removing these chain methods: - Need to get rid of findFirstBlockWithTimeAndHeight for bitcoin#10102, which doesn't support overloaded methods - Followup from bitcoin#16426 (comment) - phantomcircuit comments about findNextBlock test http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-06.html#l-214 Behavior is not changing in any way here. A TODO comment in ScanForWalletTransactions was removed, but just because it was invalid (see bitcoin#19195 (comment)), not because it was implemented.

5baa88f test: Remove no longer needed MakeChain calls (Russell Yanofsky) 6965f13 refactor: Replace uses ChainActive() in interfaces/chain.cpp (Russell Yanofsky) 3fbbb9a refactor: Get rid of more redundant chain methods (Russell Yanofsky) Pull request description: This just drops three interfaces::Chain methods replacing them with other calls. Motivation for removing these chain methods: - Need to get rid of findFirstBlockWithTimeAndHeight for #10102, which doesn't support overloaded methods - Followup from bitcoin/bitcoin#16426 (comment) - phantomcircuit comments about findNextBlock test http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-06.html#l-214 Behavior is not changing in any way here. A TODO comment in ScanForWalletTransactions was removed, but just because it was invalid (see bitcoin/bitcoin#19195 (comment)), not because it was implemented. ACKs for top commit: MarcoFalke: re-ACK 5baa88f 🕶 dongcarl: ACK 5baa88f Tree-SHA512: d20a2a8cf8742e6100c6d3a4871ed67f184925712cf9e55d301abee60353bf3f74cd0d46a13be1715d1c2faef72102bb321654d08a128c2ef6880f72b72a6687

ryanofsky · 2020-12-11T15:48:57Z

Closing this PR. Can reopen if I'm missing something, but looking over this I think all changes here are no longer applicable. Cleanups were recently merged in #19425, and the scan result change #19195 (comment) wasn't correct and TODO was removed in #19425

pstratem added 2 commits June 6, 2020 17:08

wallet: Simplify logic in ScanForWalletTransactions

8894da2

logic isn't changed except in the case that a reorg has occured we always set the FAILURE flag previously the FAILURE flag would only be set if a reorg occured and findBlock failed.

wallet: resolve inconsistent ScanForWalletTransactions result

e526bf7

DrahtBot added the Wallet label Jun 6, 2020

wallet: fix test to be in line with results from ScanForWalletTransac…

8f64ee2

…tions

DrahtBot mentioned this pull request Jun 9, 2020

wallet: Remove first parameter to ScanForWalletTransactions start_hash #19216

Closed

pstratem marked this pull request as draft June 11, 2020 20:25

ryanofsky reviewed Jun 30, 2020

View reviewed changes

ryanofsky mentioned this pull request Jul 1, 2020

refactor: Get rid of more redundant chain methods #19425

Merged

ryanofsky closed this Dec 11, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: ScanForWalletTransactions cleanup #19195

wallet: ScanForWalletTransactions cleanup #19195

Uh oh!

pstratem commented Jun 6, 2020

Uh oh!

DrahtBot commented Jun 9, 2020 •

edited

Loading

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Jun 30, 2020 •

edited

Loading

Uh oh!

ryanofsky Jun 30, 2020

Uh oh!

ryanofsky Jul 1, 2020

Uh oh!

ryanofsky commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wallet: ScanForWalletTransactions cleanup #19195

wallet: ScanForWalletTransactions cleanup #19195

Uh oh!

Conversation

pstratem commented Jun 6, 2020

Uh oh!

DrahtBot commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DrahtBot commented Jun 9, 2020 •

edited

Loading

ryanofsky Jun 30, 2020 •

edited

Loading