Improve ScanForWalletTransactions return value #9827

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/scanret changing 2 files +15 −3

ryanofsky commented at 6:34 PM on February 22, 2017: contributor

Change ScanForWalletTransactions return value so it is possible to distinguish scans that skip reading every block (due to the nTimeFirstKey optimization) from scans that fail while reading the chainActive.Tip() block. Return value is now non-null in the non-failing case.

This change doesn't affect any user-visible behavior, it is only an internal API improvement. The only code currently using the ScanForWalletTransactions return value is in importmulti, and importmulti always calls ScanForWalletTransactions with a pindex pointing to the first block in chainActive whose block time is >= (nLowestTimestamp - 7200), while ScanForWalletTransactions would only return null without reading blocks when pindex and every block after it had a block time < (nTimeFirstKey - 7200). These conditions could never happen at the same time because nTimeFirstKey <= nLowestTimestamp.

I'm planning to make a more substantial API improvement in the future (making ScanForWalletTransactions private and exposing a higher level rescan method to RPC code), but @TheBlueMatt pointed out this odd behavior introduced by #9773 yesterday, so I'm following up now to get rid of badness introduced by that merge.

in src/wallet/test/wallet_tests.cpp:None in d0bc70063b outdated

 413 | @@ -413,6 +414,14 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
 414 |          UniValue response = importmulti(request);
 415 |          BOOST_CHECK_EQUAL(response.write(), strprintf("[{\"success\":false,\"error\":{\"code\":-1,\"message\":\"Failed to rescan before time %d, transactions may be missing.\"}}]", newTip->GetBlockTimeMax()));
 416 |      }
 417 | +
 418 | +    // Verify ScanForWalletTransactions does not return null when the scan is
 419 | +    // elided due to the nTimeFirstKey optimization.
 420 | +    {
 421 | +        CWallet wallet;

paveljanik commented at 6:40 PM on February 22, 2017:

1 wallet/test/wallet_tests.cpp:421:17: warning: declaration shadows a variable in namespace 'wallet_tests' [-Wshadow]

jonasschnelli commented at 7:44 PM on February 23, 2017:

I think here we have a missing LOCK(wallet.cs_wallet);?, don't we?

ryanofsky commented at 7:54 PM on February 23, 2017:

Thanks, guess travis is still useful for some things. Fixed in d7e96f8562680ec30e1ca38f72206179693ab753

ryanofsky referenced this in commit 09fe346a4f on Feb 22, 2017
ryanofsky cross-referenced this on Feb 22, 2017 from issue Avoid -Wshadow warnings in wallet_tests by ryanofsky
fanquake added the label Wallet on Feb 22, 2017
ryanofsky force-pushed on Feb 23, 2017
ryanofsky referenced this in commit d7e96f8562 on Feb 23, 2017
ryanofsky force-pushed on Feb 23, 2017
ryanofsky commented at 7:58 PM on February 23, 2017: contributor

Squashed d7e96f8562680ec30e1ca38f72206179693ab753 -> d09410a7ac326cadff6e46df078d6bca013290cc (scanret.2 -> scanret.3)
TheBlueMatt commented at 9:14 PM on February 23, 2017: contributor

Hmm, I think this API really sucks....how about ScanForWalletTransaction(nLowestKeyJustAdded, fUpdate) instead of a CBlockIndex*, and then return the timestamp. That way we dont have duplicated logic calculating the block back 7200 seconds to use, and can use the timestamp in importmulti, which is really what it wants.
ryanofsky commented at 9:20 PM on February 23, 2017: contributor

Yeah, that's the type of API improvement I meant in "I'm planning to make a more substantial API improvement in the future."
TheBlueMatt commented at 9:23 PM on February 23, 2017: contributor

I mean if you're gonna rewrite it, might as well do that instead of merging a smaller fix that gets overwritten a week later in the same release?
ryanofsky commented at 9:29 PM on February 23, 2017: contributor

This is the change I have right now. I don't think anything will get overwritten in a week. All I'm saying is I think I will probably do more cleanup in this area, vaguely along the lines you are suggesting.
TheBlueMatt commented at 3:27 PM on February 24, 2017: contributor

utACK d09410a7ac326cadff6e46df078d6bca013290cc

Improve ScanForWalletTransactions return value

Change ScanForWalletTransactions return value so it is possible to distinguish
scans that skip reading every block (due to the nTimeFirstKey optimization)
from scans that fail while reading the chainActive.Tip() block. Return value is
now non-null in the non-failing case.

This change doesn't affect any user-visible behavior, it is only an internal
API improvement. The only code currently using the ScanForWalletTransactions
return value is in importmulti, and importmulti always calls
ScanForWalletTransactions with a pindex pointing to the first block in
chainActive whose block time is >= (nLowestTimestamp - 7200), while
ScanForWalletTransactions would only return null without reading blocks when
pindex and every block after it had a block time < (nTimeFirstKey - 7200).
These conditions could never happen at the same time because nTimeFirstKey <=
nLowestTimestamp.

I'm planning to make a more substantial API improvement in the future (making
ScanForWalletTransactions private and exposing a higher level rescan method to
RPC code), but Matt Corallo <git@bluematt.me> pointed out this odd behavior
introduced by e2e2f4c "Return errors from importmulti if complete rescans are
not successful" yesterday, so I'm following up now to get rid of badness
introduced by that merge.

30abce7a99

ryanofsky force-pushed on Mar 1, 2017
ryanofsky commented at 10:22 AM on March 1, 2017: contributor

Rebased d09410a7ac326cadff6e46df078d6bca013290cc -> 30abce7a998181e28e2f93256c159f259513e1a7 (scanret.3 -> scanret.4) to avoid conflict with one of the recent changes in wallet_tests.cpp.
jonasschnelli commented at 3:06 PM on March 17, 2017: contributor

utACK 30abce7a998181e28e2f93256c159f259513e1a7
laanwj merged this on Apr 19, 2017
laanwj closed this on Apr 19, 2017
laanwj referenced this in commit c91ca0ace9 on Apr 19, 2017
practicalswift referenced this in commit cf0bbf42c4 on Apr 27, 2017
ryanofsky cross-referenced this on May 16, 2017 from issue Improve wallet rescan API by ryanofsky
promag cross-referenced this on Jan 26, 2018 from issue Improve ScanForWalletTransactions return value by ryanofsky
PastaPastaPasta referenced this in commit 3c0488a2da on May 10, 2019
PastaPastaPasta referenced this in commit 8ab09b3d19 on May 15, 2019
PastaPastaPasta referenced this in commit a089c9325e on May 20, 2019
PastaPastaPasta referenced this in commit 706961087f on May 21, 2019
barrystyle referenced this in commit b24dcd2451 on Jan 22, 2020
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9773 Return errors from importmulti if complete rescans are not successful #9828 Avoid -Wshadow warnings in wallet_tests #10412 Improve wallet rescan API #12275 Improve ScanForWalletTransactions return value