wallet: ScanForWalletTransactions cleanup

pstratem commented at 9:20 pm on June 6, 2020: contributor

consolidated findNextBlock calls and reorg handling

wallet: Simplify logic in ScanForWalletTransactions

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.

8894da2059

wallet: resolve inconsistent ScanForWalletTransactions result e526bf7080

DrahtBot added the label Wallet on Jun 6, 2020

wallet: fix test to be in line with results from ScanForWalletTransactions 8f64ee27ef

DrahtBot commented at 10:53 am on June 9, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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.

pstratem marked this as a draft on Jun 11, 2020

in src/wallet/wallet.cpp:1705 in 8894da2059 outdated

1711+            result.status = ScanResult::FAILURE;
1712+            break;
1713+        }
1714         if (chain().findBlock(block_hash, FoundBlock().data(block)) && !block.IsNull()) {
1715             LOCK(cs_wallet);
1716-            next_block = chain().findNextBlock(block_hash, block_height, FoundBlock().hash(next_block_hash), &reorg);

ryanofsky commented at 10:07 pm on June 30, 2020:

In commit “wallet: Simplify logic in ScanForWalletTransactions” (8894da2059a3abd4789b8396749f915114ead687)

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)

in src/wallet/wallet.cpp:1711 in e526bf7080 outdated

1704@@ -1705,10 +1705,8 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1705         if (reorg) {
1706             // Abort scan if current block is no longer active, to prevent
1707             // marking transactions as coming from the wrong block.
1708-            // TODO: This should return success instead of failure, see
1709+            // break successfully when reorg occurs, reorg will scan blocks for transactions
1710             // https://github.com/bitcoin/bitcoin/pull/14711#issuecomment-458342518
1711-            result.last_failed_block = block_hash;
1712-            result.status = ScanResult::FAILURE;

ryanofsky commented at 10:46 pm on June 30, 2020:

In commit “wallet: resolve inconsistent ScanForWalletTransactions result” (e526bf7080af94ad0d810908c5a249b719b97707)

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.

ryanofsky commented at 5:41 pm on July 1, 2020:

re: #19195 (review)

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

Removing TODO comment in #19425

ryanofsky commented at 11:18 pm on June 30, 2020: member

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 referenced this in commit 87bc7618cf on Jul 1, 2020

ryanofsky referenced this in commit 91ac0388e8 on Jul 10, 2020

ryanofsky referenced this in commit 3e54d36b17 on Jul 21, 2020

ryanofsky referenced this in commit fd96326d31 on Aug 7, 2020

ryanofsky referenced this in commit 53fae31e49 on Aug 7, 2020

ryanofsky referenced this in commit 4e7906269c on Sep 25, 2020

ryanofsky referenced this in commit 97e683d1a1 on Sep 25, 2020

ryanofsky referenced this in commit d0abe4e56d on Dec 8, 2020

ryanofsky referenced this in commit 3fbbb9a640 on Dec 8, 2020

MarcoFalke referenced this in commit e98d1d6740 on Dec 8, 2020

ryanofsky commented at 3:48 pm on December 11, 2020: member

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 (review) wasn’t correct and TODO was removed in #19425

ryanofsky closed this on Dec 11, 2020

DrahtBot locked this on Feb 15, 2022

wallet: ScanForWalletTransactions cleanup #19195

Conflicts