Improve ScanForWalletTransactions return value #9827
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paveljanik
reviewed
Feb 22, 2017
| // Verify ScanForWalletTransactions does not return null when the scan is | ||
| // elided due to the nTimeFirstKey optimization. | ||
| { | ||
| CWallet wallet; |
There was a problem hiding this comment.
1 wallet/test/wallet_tests.cpp:421:17: warning: declaration shadows a variable in namespace 'wallet_tests' [-Wshadow]
There was a problem hiding this comment.
I think here we have a missing LOCK(wallet.cs_wallet);?, don't we?
There was a problem hiding this comment.
Thanks, guess travis is still useful for some things. Fixed in d7e96f8
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 22, 2017
Warnings introduced by commit e2e2f4c "Return errors from importmulti if complete rescans are not successful" and reported by Pavel Janík <Pavel@Janik.cz> in bitcoin#9773 and bitcoin#9827 wallet/test/wallet_tests.cpp: In member function ‘void wallet_tests::rescan::test_method()’: wallet/test/wallet_tests.cpp:377:17: warning: declaration of ‘wallet’ shadows a global declaration [-Wshadow] CWallet wallet;
d0bc700 to
b75d1b8
Compare
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 23, 2017
Fix missing cs_wallet lock suggested bitcoin#9827 (comment)
d7e96f8 to
d09410a
Compare
ryanofsky
commented
Feb 23, 2017
| // Verify ScanForWalletTransactions does not return null when the scan is | ||
| // elided due to the nTimeFirstKey optimization. | ||
| { | ||
| CWallet wallet; |
There was a problem hiding this comment.
Thanks, guess travis is still useful for some things. Fixed in d7e96f8
|
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. |
|
Yeah, that's the type of API improvement I meant in "I'm planning to make a more substantial API improvement in the future." |
|
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? |
|
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. |
|
utACK d09410a |
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.
|
utACK 30abce7 |
laanwj
added a commit
that referenced
this pull request
Apr 19, 2017
30abce7 Improve ScanForWalletTransactions return value (Russell Yanofsky) Tree-SHA512: 195028553b103052a842b6a37e67410118a20c32475b80f7fd22d6d8622f92eca1c2d84f291d1092bef2161d3187d91052799b533e1e265b7613d51955490b8d
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
May 10, 2019
30abce7 Improve ScanForWalletTransactions return value (Russell Yanofsky) Tree-SHA512: 195028553b103052a842b6a37e67410118a20c32475b80f7fd22d6d8622f92eca1c2d84f291d1092bef2161d3187d91052799b533e1e265b7613d51955490b8d
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
May 15, 2019
30abce7 Improve ScanForWalletTransactions return value (Russell Yanofsky) Tree-SHA512: 195028553b103052a842b6a37e67410118a20c32475b80f7fd22d6d8622f92eca1c2d84f291d1092bef2161d3187d91052799b533e1e265b7613d51955490b8d
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
May 20, 2019
30abce7 Improve ScanForWalletTransactions return value (Russell Yanofsky) Tree-SHA512: 195028553b103052a842b6a37e67410118a20c32475b80f7fd22d6d8622f92eca1c2d84f291d1092bef2161d3187d91052799b533e1e265b7613d51955490b8d
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
May 21, 2019
30abce7 Improve ScanForWalletTransactions return value (Russell Yanofsky) Tree-SHA512: 195028553b103052a842b6a37e67410118a20c32475b80f7fd22d6d8622f92eca1c2d84f291d1092bef2161d3187d91052799b533e1e265b7613d51955490b8d
barrystyle
pushed a commit
to PACGlobalOfficial/PAC
that referenced
this pull request
Jan 22, 2020
30abce7 Improve ScanForWalletTransactions return value (Russell Yanofsky) Tree-SHA512: 195028553b103052a842b6a37e67410118a20c32475b80f7fd22d6d8622f92eca1c2d84f291d1092bef2161d3187d91052799b533e1e265b7613d51955490b8d
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.