This change removes uses of chainActive and mapBlockIndex globals in wallet code. It is a refactoring change which does not affect external behavior.
This is the next step in the larger #10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in #10102.
Concept ACK. Though, I'd prefer if we first cleaned up the interface for ScanForWalletTransactions. I believe that right now it is relying on too much undefined behaviour that this refactoring could be meaningfully reviewed.
too much undefined behaviour that this refactoring could be meaningfully reviewed.
Are you talking about a small part of this PR or the whole thing? Most of the changes here don't have anything to do with ScanForWalletTransactions. I could even drop the rescan changes and save them for a different PR if you are only worried about them.
Concept ACK
Nice readability and robustness improvement
Concept ACK
Would be easier/safer to review if split up. Like Marco, I’d like to see ScanFor improved first.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
4172 | {
4173 | - CBlockIndex *block = chainActive.Tip();
4174 | - while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA) && block->pprev->nTx > 0 && pindexRescan != block)
4175 | - block = block->pprev;
4176 | + int block_height = *tip_height;
4177 | + while (block_height > 0 && locked_chain->haveBlockOnDisk(block_height - 1) && rescan_height != block_height) {
Why not use findPruned(rescan_height, *tip_height)?
re: #14711 (review)
Why not use findPruned
I used haveBlockOnDisk here because I was doing a very literal translation and findPruned doesn't have the block->pprev->nTx > 0 condition. If can drop that condition or add it to findPruned, though, if it would be an improvement.
1645 | - * defined by pindexStop 1646 | + * If stop_block is not null, the scan will stop at the block-index 1647 | + * defined by stop_block 1648 | * 1649 | - * Caller needs to make sure pindexStop (and the optional pindexStart) are on 1650 | + * Caller needs to make sure stop_block (and the optional start_block) are on
I think this is where the earlier confusion arose, it is stop_block which is optional not start_block, so the comment is wrong, the names need to be switched
re: #14711 (review)
the names need to be switched
Good catch. I'll probably just rewrite this comment if this isn't fixed by another of the rescan PRs first. This comment never made sense to me even when it was first added in #11281 (review) / #11281 (review).
365 | @@ -366,8 +366,7 @@ UniValue importprunedfunds(const JSONRPCRequest& request) 366 | if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) == merkleBlock.header.hashMerkleRoot) { 367 | 368 | auto locked_chain = pwallet->chain().lock(); 369 | - const CBlockIndex* pindex = LookupBlockIndex(merkleBlock.header.GetHash()); 370 | - if (!pindex || !chainActive.Contains(pindex)) { 371 | + if (!locked_chain->getBlockHeight(merkleBlock.header.GetHash())) {
This sounds wrong, as if it wants to discard genesis block (height == 0). Only got it after seeing the return type Optional<int>. IMO .contains(hash) would be preferable.
Could be more clear with a comparison with nullopt
1528 | @@ -1526,24 +1529,18 @@ static UniValue listsinceblock(const JSONRPCRequest& request) 1529 | auto locked_chain = pwallet->chain().lock(); 1530 | LOCK(pwallet->cs_wallet); 1531 | 1532 | - const CBlockIndex* pindex = nullptr; // Block index of the specified block or the common ancestor, if the block provided was in a deactivated chain. 1533 | - const CBlockIndex* paltindex = nullptr; // Block index of the specified block, even if it's in a deactivated chain. 1534 | + Optional<int> height; // Height of the specified block or the common ancestor, if the block provided was in a deactivated chain.
Isn't this change conceptually different? Even though the chain is locked, this can be copied and used without the lock and therefore can "point" to other block if a reorg occurs. This wouldn't occur with CBlockIndex.
Looks like you could either:
interfaces::BlockIndexCBlockIndex* with uint256.
re: #14711 (review)
Isn't this change conceptually different?
I think so, a height is conceptually different from a block pointer.
Looks like you could either:
When the chain is locked, a height uniquely identifies a block, so many locked_chain methods take height arguments. I think it would good to eliminate these calls entirely (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269) and not very useful to tweak data types and just hide the fact that the chain is locked.
But either way, I want the API to change in the future and become nicer, more minimal, and more useful over time. Right now the API just reflects how the wallet currently works, to keep changes to wallet code in this PR as minimal as possible.
Concept ACK.
892 | @@ -896,7 +893,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface 893 | void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex *pindex, const std::vector<CTransactionRef>& vtxConflicted) override; 894 | void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock) override; 895 | int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update); 896 | - CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, const WalletRescanReserver& reserver, bool fUpdate = false); 897 | + uint256 ScanForWalletTransactions(const uint256& start_block, const uint256& stop_block, const WalletRescanReserver &reserver, bool fUpdate);
Haven't though about this too much, but maybe you could split out the changes in the wallet header that change the interface from taking a block index pointer to taking a block hash to a separate commit? Unless I am mistaken those type changes have the side effect of fixing the "issue" I tried to fix in #14712.
116 | + CBlock* block = nullptr, 117 | + int64_t* time = nullptr, 118 | + int64_t* max_time = nullptr) = 0; 119 | + 120 | + //! Estimate fraction of total transactions verified if blocks up to 121 | + //! given height are verified.
doc-nit: height is not given (at least not directly)
Strong Concept ACK Plans to review...
@ryanofsky I went ahead and split this up into a few commits, found it easier to review. Built and ran tests on each commit. https://github.com/Empact/bitcoin/tree/pr/wchain2
11 | +#include <uint256.h> 12 | #include <util/system.h> 13 | #include <validation.h> 14 | 15 | #include <memory> 16 | +#include <unordered_map>
nit: I believe this belongs in validation.h, as unordered_map isn't used directly here.
re: #14711 (review)
nit: I believe this belongs in validation.h, as unordered_map isn't used directly here.
This was added by IWYU because unordered map methods are called here. In theory this allows validation.h to switch to forward declarations and drop its unordered_map include without this file having to change. I don't think IWYU rationale is completely airtight (particularly in this case where validation.h might change mapBlockIndex into a different type of map with the same methods), but I like the IWYU tool, and don't think its worth spending a lot of time to analyze its decisions absent a compelling reason.
149 | @@ -35,6 +150,34 @@ class ChainImpl : public Chain 150 | return std::move(result); 151 | } 152 | std::unique_ptr<Chain::Lock> assumeLocked() override { return MakeUnique<LockImpl>(); } 153 | + bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override
nit: none of the callers use more than one of the out args, so splitting this up would make the call sites simpler/clearer.
re: #14711 (review)
nit: none of the callers use more than one of the out args, so splitting this up would make the call sites simpler/clearer.
There are many ways to design this API, and I just opted for one here that I thought would keep chain.h a little shorter and more organized, at the expense of being more verbose at call sites. If you have a specific alternative in mind and want to convince me or another reviewer that it's better, I'd be happy to adopt it here, or review a followup PR.
3442 | 3443 | - const CBlockIndex *failed_block, *stopBlock; 3444 | + uint256 failed_block, stopBlock; 3445 | CWallet::ScanResult result = 3446 | - pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, failed_block, stopBlock, true); 3447 | + pwallet->ScanForWalletTransactions(start_block, stop_block, reserver, failed_block, stopBlock, true /* prune */);
stop_block and stopBlock are confusingly similar.
Thanks @Empact! I reset the PR to your branch.
Rebased d568bdb15173dd8dd7bf95cb0bd9c9b55dbb97f0 -> 5cad675a8e8beb0a97f95860a3017fd581bcc1e5 (pr/wchain2.1 -> pr/wchain2.2) due to conflict with #14380 Rebased 5cad675a8e8beb0a97f95860a3017fd581bcc1e5 -> 591c2c85e928441bf1f5b1f58c132e72ad4eb185 (pr/wchain2.2 -> pr/wchain2.3) due to conflict with #13076 Rebased 591c2c85e928441bf1f5b1f58c132e72ad4eb185 -> 939a400f3d632330e470c4b3e71ee822ae7be4c4 (pr/wchain2.3 -> pr/wchain2.4) due to conflict with #14957 Split 939a400f3d632330e470c4b3e71ee822ae7be4c4 -> 7a8727256c99dfbf739448fc72082ec568e14056 (pr/wchain2.4 -> pr/wchain2.5) resetting to Empact:pr/wchain2 (no diffs) Rebased 7a8727256c99dfbf739448fc72082ec568e14056 -> ca76bcc917dbc53a1b639acbdbb5d2faf28bbc52 (pr/wchain2.5 -> pr/wchain2.6) due to conflict with #15039
Updated ca76bcc917dbc53a1b639acbdbb5d2faf28bbc52 -> b5c5b202757e6b9322d28f4a1ac533622ab4db00 (pr/wchain2.6 -> pr/wchain2.7) with suggestions from review comments Rebased b5c5b202757e6b9322d28f4a1ac533622ab4db00 -> 5bc30a5083507fbc10691c7c26683afe7e768c88 (pr/wchain2.7 -> pr/wchain2.8) due to conflict with #14556.
utACK https://github.com/bitcoin/bitcoin/commit/b5c5b202757e6b9322d28f4a1ac533622ab4db00
My previous comments on a superset of this PR still hold:
Reviewers will notice that dealing in the more primitive datatypes returned by chain methods (block heights and hashes as opposed to, say, full CBlockIndex values) results in more fiddling at callsites (null checks, arithmetic, subsequent lookups, etc.). This is an obvious drawback, but if we imagine that these calls are happening over a process boundary - as, god willing, they someday will - then the simpler the datatype the better. The stripped-down return types also more tightly accommodate the data requirements of the callers. For these two reasons, this changeset is a big step towards system decomposition IMO.
5f71b83448 Add height, depth, and hash methods to the Chain interface
909557ac65 Add time methods to the Chain interface
7669e12075 Add findFork and findBlock to the Chain interface
628f97b97e Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis
Note for reviewers: "block" in Chain::Lock method names refers to a height. A height is assumed to uniquely identify a given block for the duration of Chain::Lock's lifetime.
b5c5b20275 Remove remaining chainActive references from CWallet
I'll do some manual testing soon.
And use them to remove uses of chainActive and mapBlockIndex in wallet code
This commit does not change behavior.
Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
And use them to remove uses of chainActive and mapBlockIndex in wallet code
This commit does not change behavior.
Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
And use them to remove uses of chainActive and mapBlockIndex in wallet code
This commit does not change behavior.
Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
Only change in behavior is "Rescan started from block <height>" message
replaced by "Rescan started from block <hash>" message in
ScanForWalletTransactions.
Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
This commit does not change behavior.
Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
30 | + return nullopt; 31 | + } 32 | + Optional<int> getBlockHeight(const uint256& hash) override 33 | + { 34 | + auto it = ::mapBlockIndex.find(hash); 35 | + if (it != ::mapBlockIndex.end() && it->second && ::chainActive.Contains(it->second)) {
in commit 13bc578ca9 Add height, depth, and hash methods to the Chain interface
It seems confusing to check for nullptr if when the block hash was found in the map, since the value should never be a nullptr? Anyway, a neat way to avoid that is by avoiding ::mapBlockIndex and calling LookupBlockIndex?
re: #14711 (review)
Switched to LookupBlockIndex
1575 | @@ -1573,24 +1576,18 @@ static UniValue listsinceblock(const JSONRPCRequest& request) 1576 | auto locked_chain = pwallet->chain().lock(); 1577 | LOCK(pwallet->cs_wallet); 1578 | 1579 | - const CBlockIndex* pindex = nullptr; // Block index of the specified block or the common ancestor, if the block provided was in a deactivated chain. 1580 | - const CBlockIndex* paltindex = nullptr; // Block index of the specified block, even if it's in a deactivated chain. 1581 | + Optional<int> height; // Height of the specified block or the common ancestor, if the block provided was in a deactivated chain.
nit in commit 13bc578ca9 Add height, depth, and hash methods to the Chain interface
height is added and not used in this commit
(Hunk should probably be moved to commit 22abf4a61a Add findFork and findBlock to the Chain interface)
re: #14711 (review)
height is added and not used in this commit
Seems to have been a harmless artifact from splitting this PR up. Moved to the right place now.
99 | + Optional<int> findFork(const uint256& hash, Optional<int>* height) override 100 | + { 101 | + const CBlockIndex *block{nullptr}, *fork{nullptr}; 102 | + auto it = ::mapBlockIndex.find(hash); 103 | + if (it != ::mapBlockIndex.end()) { 104 | + block = it->second;
in commit 22abf4a61a Add findFork and findBlock to the Chain interface
Doesn't check for nullptr here? Seems inconsistent with getBlockHeight. Anyway, a neat way to avoid that is by avoiding ::mapBlockIndex and calling LookupBlockIndex?
re: #14711 (review)
Switched to LookupBlockIndex
157 | + LOCK(cs_main); 158 | + auto it = ::mapBlockIndex.find(hash); 159 | + if (it == ::mapBlockIndex.end()) { 160 | + return false; 161 | + } 162 | + index = it->second;
in commit 22abf4a61a Add findFork and findBlock to the Chain interface
Same here (use LookupBlockIndex)
re: #14711 (review)
Switched to LookupBlockIndex
1619 | @@ -1622,9 +1620,9 @@ static UniValue listsinceblock(const JSONRPCRequest& request) 1620 | // when a reorg'd block is requested, we also list any relevant transactions 1621 | // in the blocks of the chain that was detached 1622 | UniValue removed(UniValue::VARR); 1623 | - while (include_removed && paltindex && paltindex != pindex) { 1624 | + while (include_removed && altheight && *altheight > height.value_or(-1)) {
in commit 22abf4a61a Add findFork and findBlock to the Chain interface
Assuming that height can not be set makes no sense, since you moved the FindFork call to before the !height check. I'd prefer to write *height here.
1603 | @@ -1607,7 +1604,8 @@ static UniValue listsinceblock(const JSONRPCRequest& request) 1604 | 1605 | bool include_removed = (request.params[3].isNull() || request.params[3].get_bool()); 1606 | 1607 | - int depth = pindex ? (1 + chainActive.Height() - pindex->nHeight) : -1; 1608 | + const Optional<int> tip_height = locked_chain->getHeight(); 1609 | + int depth = tip_height && height ? (1 + *tip_height - *height) : -1;
in commit 22abf4a Add findFork and findBlock to the Chain interface
Same (I'd prefer to not check for height again, since that has already been done above as you moved the FindFork to before the height check.)
re: #14711 (review)
I'd prefer to not check for height again
Height can be unset here since the blockhash parameter is optional.
Right, my bad.
174 | + } 175 | + double guessVerificationProgress(const uint256& block_hash) override 176 | + { 177 | + LOCK(cs_main); 178 | + auto it = ::mapBlockIndex.find(block_hash); 179 | + return GuessVerificationProgress(Params().TxData(), it != ::mapBlockIndex.end() ? it->second : nullptr);
in commit 65861dff7c Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis
LookupBlockIndex instead of access to the global mapBlockIndex?
re: #14711 (review)
Switched to LookupBlockIndex
utACK 22abf4a61a
Some (I think) new compile warnings with this PR:
/.../bitcoin/src/wallet/rpcwallet.cpp: In function ‘UniValue rescanblockchain(const JSONRPCRequest&)’:
/.../bitcoin/src/wallet/rpcwallet.cpp:3408:49: warning: ‘*((void*)& tip_height +4)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
if (*stop_height < 0 || !tip_height || *stop_height > *tip_height) {
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /.../bitcoin/src/rpc/mining.h:10:0,
from /.../bitcoin/src/wallet/rpcwallet.cpp:21:
/.../bitcoin/src/univalue/include/univalue.h:59:42: warning: ‘*((void*)& stop_height +4)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
bool setInt(int val_) { return setInt((int64_t)val_); }
~~~~~~^~~~~~~~~~~~~~~
/.../bitcoin/src/wallet/rpcwallet.cpp:3391:19: note: ‘*((void*)& stop_height +4)’ was declared here
Optional<int> stop_height;
^~~~~~~~~~~
/.../bitcoin/src/wallet/rpcwallet.cpp: In function ‘UniValue listsinceblock(const JSONRPCRequest&)’:
/.../bitcoin/src/wallet/rpcwallet.cpp:1579:19: warning: ‘*((void*)& height +4)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Optional<int> height; // Height of the specified block or the common ancestor, if the block provided was in a deactivated chain.
^~~~~~
/.../bitcoin/src/wallet/wallet.cpp: In member function ‘CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256&, const uint256&, const WalletRescanReserver&, uint256&, uint256&, bool)’:
/.../bitcoin/src/wallet/wallet.cpp:1709:58: warning: ‘*((void*)& block_height +4)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
block_height = prev_block_height + 1;
~~~~~~~~~~~~~~~~~~^~~
utACK 5bc30a508, this really improves interfaces::Chain. I'm also curious why LookupBlockIndex is avoided.
Updated 5bc30a5083507fbc10691c7c26683afe7e768c88 -> 1da8b4648e6b5f01a90acfe487bcda6f7a5a0424 (pr/wchain2.8 -> pr/wchain2.9) to resolve comments.
re: #14711 (comment)
Some (I think) new compile warnings with this PR:
These are spurious and should be fixed if you use a newer compiler: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47679
It might also be possible to work around them (but awkward): https://stackoverflow.com/questions/21755206/how-to-get-around-gcc-void-b-4-may-be-used-uninitialized-in-this-funct
re: #14711 (comment)
I'm also curious why LookupBlockIndex is avoided.
Wasn't really avoided, it just didn't exist when this code was written: #10973 < #11041.
3461 | } 3462 | UniValue response(UniValue::VOBJ); 3463 | - response.pushKV("start_height", pindexStart->nHeight); 3464 | - response.pushKV("stop_height", stopBlock->nHeight); 3465 | + response.pushKV("start_height", start_height); 3466 | + response.pushKV("stop_height", stop_height ? *stop_height : UniValue());
I think you want stop_block here. stop_height will carry the height that we sought to scan to, stop_block will carry the block that it actually reached.
re: #14711 (review)
It would be nice to include stop_block in the future, since it's a hash not a height, but this is not changing the behavior at all. stop_height will always contain the height scanned to because if the scan fails or aborts there will have been an exception earlier.
IRC mention
[12:19:55] <wumpus> #14711 seems almost ready for merge for a while, really just waiting for ryanofsky to respond to empact's comment on the test
http://www.erisian.com.au/bitcoin-core-dev/log-2019-01-22.html#l-146
110 | + //! or contents. 111 | + //! 112 | + //! If a block pointer is provided to retrieve the block contents, and the 113 | + //! block exists but doesn't have data (for example due to pruning), the 114 | + //! block will be empty and all fields set to null. 115 | + virtual bool findBlock(const uint256& hash,
Below you call findBlock while the chain is locked. So should this be in the Chain::Lock? Or will there be a new mutex for mapBlockIndex?
I see that the idea is to move away from locked chain so ignore my question above: https://github.com/bitcoin/bitcoin/blob/82cf6813a4ef1b4a5439eb6cddb1ab426f3c31a2/src/interfaces/chain.h#L23-L26
Looks like we're in good shape for merge.
80 | + //! nothing if no block in the range is pruned. Range is inclusive. 81 | + virtual Optional<int> findPruned(int start_height = 0, Optional<int> stop_height = nullopt) = 0; 82 | + 83 | + //! Return height of the highest block on the chain that is an ancestor 84 | + //! of the specified block. Also return the height of the specified 85 | + //! block as an optional output parameter.
Nit: returning the height of the specified block is a bit of an odd interface, but saves a second lookup in ChainActive that getHeight() would require. Perhaps add that reasoning to this comment?
47 | + 48 | + //! Get block depth. Returns 1 for chain tip, 2 for preceding block, and 49 | + //! so on. Returns 0 for a block not included in the current chain. 50 | + virtual int getBlockDepth(const uint256& hash) = 0; 51 | + 52 | + //! Get block hash.
Nit: add comment that this function will assert if provided a height that isn't in the active chain (same for getBlockTime() and getMedianTimePast()).
79 | + //! Return height of last block in the specified range which is pruned, or 80 | + //! nothing if no block in the range is pruned. Range is inclusive. 81 | + virtual Optional<int> findPruned(int start_height = 0, Optional<int> stop_height = nullopt) = 0; 82 | + 83 | + //! Return height of the highest block on the chain that is an ancestor 84 | + //! of the specified block. Also return the height of the specified
Nit: Perhaps add or nullopt if no common ancestor is found to "Return height of the highest block on the chain that is an ancestor of the specified block"
33 | @@ -28,6 +34,67 @@ class Chain 34 | { 35 | public: 36 | virtual ~Lock() {} 37 | + 38 | + //! Get current chain height, not including genesis block (returns 0 if 39 | + //! chain only contains genesis block, nothing if chain does not contain
Nit: Slight preference for s/nothing/nullopt/ for all of these comments to be more precise.
69 | + 70 | + //! Return height of the first block in the chain with timestamp equal 71 | + //! or greater than the given time and height equal or greater than the 72 | + //! given height, or nothing if there is no such block. 73 | + //! 74 | + //! Calling this with height 0 is equivalent to calling
Nit: seems like CChain::FindEarliestAtLeast() could quite easily be updated to take a height parameter and use it in the std::lower_bound() call.
Perhaps a future PR could combine findFirstBlockWithTime() and findFirstBlockWithTimeAndHeight() and remove some of the logic from the interface?
re: #14711 (review)
Perhaps a future PR could combine findFirstBlockWithTime() and findFirstBlockWithTimeAndHeight()
This is a good suggestion. I just added a TODO to avoid introducing CChain changes in this PR, but I'm also happy to make the change here if you prefer.
146 | @@ -146,13 +147,12 @@ void TestGUI() 147 | auto locked_chain = wallet->chain().lock(); 148 | WalletRescanReserver reserver(wallet.get()); 149 | reserver.reserve(); 150 | - const CBlockIndex* const null_block = nullptr; 151 | - const CBlockIndex *stop_block, *failed_block; 152 | + uint256 stop_block, failed_block; 153 | QCOMPARE( 154 | - wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, failed_block, stop_block, true /* fUpdate */), 155 | + wallet->ScanForWalletTransactions(locked_chain->getBlockHash(0), {} /* stop block */, reserver, failed_block, stop_block, true /* fUpdate */),
nit: comment here should be /* last block */
1593 | - startBlock = chainActive.FindEarliestAtLeast(startTime - TIMESTAMP_WINDOW); 1594 | - WalletLogPrintf("%s: Rescanning last %i blocks\n", __func__, startBlock ? chainActive.Height() - startBlock->nHeight + 1 : 0); 1595 | + const Optional<int> start_height = locked_chain->findFirstBlockWithTime(startTime - TIMESTAMP_WINDOW); 1596 | + const Optional<int> tip_height = locked_chain->getHeight(); 1597 | + WalletLogPrintf("%s: Rescanning last %i blocks\n", __func__, tip_height && start_height ? *tip_height - *start_height + 1 : 0); 1598 | + if (start_height) start_block = locked_chain->getBlockHash(*start_height);
Nit: could you save a second chainActive lookup here by returning both the height and hash from findFirstBlockWithTime()?
re: #14711 (review)
Nit: could you save a second chainActive lookup here by returning both the height and hash from findFirstBlockWithTime()?
done
1603 | + if (!start_block.IsNull()) { 1604 | + uint256 failed_block, stop_block; 1605 | // TODO: this should take into account failure by ScanResult::USER_ABORT 1606 | - if (ScanResult::FAILURE == ScanForWalletTransactions(startBlock, nullptr, reserver, failedBlock, stop_block, update)) { 1607 | - return failedBlock->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1; 1608 | + if (ScanResult::FAILURE == ScanForWalletTransactions(start_block, {} /* stop block */, reserver, failed_block, stop_block, update)) {
Nit: comment should be /* last block */
1648 | + failed_block.SetNull(); 1649 | + stop_block.SetNull(); 1650 | + ScanResult result = ScanResult::SUCCESS; 1651 | 1652 | - if (pindex) WalletLogPrintf("Rescan started from block %d...\n", pindex->nHeight); 1653 | + WalletLogPrintf("Rescan started from block %s...\n", first_block.ToString());
Behaviour change: This changes the log message from logging the height to logging the block hash. Is that intentional?
Behaviour change: This changes the log message from logging the height to logging the block hash. Is that intentional?
This seemed like the easiest thing to print without acquiring cs_main. Added a note about the behavior change in the commit message, but could update the print instead if you think that's a better idea.
1699 | - if (pindex && !chainActive.Contains(pindex)) { 1700 | + if (!locked_chain->getBlockHeight(block_hash)) { 1701 | // Abort scan if current block is no longer active, to prevent 1702 | // marking transactions as coming from the wrong block. 1703 | - failed_block = pindex; 1704 | + failed_block = block_hash;
Behaviour change: I think in this case the function will return ScanResult::SUCCESS where previously it returned ScanResult::FAILURE
re: #14711 (review)
Behaviour change: I think in this case the function will return ScanResult::SUCCESS where previously it returned ScanResult::FAILURE
Great catch, this should be fixed now. I added a todo to change this to SUCCESS in the future though, since I think that is right thing to do (see #14711 (comment)).
3446 | 3447 | - const CBlockIndex *failed_block, *stopBlock; 3448 | + uint256 failed_block, stop_block; 3449 | CWallet::ScanResult result = 3450 | - pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, failed_block, stopBlock, true); 3451 | + pwallet->ScanForWalletTransactions(first_block, last_block, reserver, failed_block, stop_block, true /* prune */);
Nit: comment should be /* fUpdate */
3438 | + throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height."); 3439 | + } 3440 | + 3441 | + if (tip_height) { 3442 | + first_block = locked_chain->getBlockHash(start_height); 3443 | + last_block = locked_chain->getBlockHash(stop_height.value_or(*tip_height));
Behaviour change: previously, calling rescanblockchain() without a stop_height would cause ScanForWalletTransactions() to be called without a pIndexStop. Since ScanForWalletTransactions() releases cs_main after every block scanned, if the tip advances during rescan, then ScanForWalletTransactions() will scan to the new tip.
This change sets the last_block to the tip at the time that the RPC is called. That means that the rescan will not go all the way to the tip if the tip advances during rescan.
re: #14711 (review)
the rescan will not go all the way to the tip if the tip advances during rescan
Fixed, but after this change, it also wasn't right to return the original tip_height as stop_height, so I changed ScanForWalletTransactions to return a stop_height, and moved all its return values into a struct to avoid having too many output parameters.
1733 | - pindex = chainActive.Next(pindex); 1734 | - progress_current = GuessVerificationProgress(chainParams.TxData(), pindex); 1735 | - if (pindexStop == nullptr && tip != chainActive.Tip()) { 1736 | - tip = chainActive.Tip(); 1737 | + if (Optional<int> tip_height = locked_chain->getHeight()) { 1738 | + if (locked_chain->getBlockHeight(prev_block_hash) && prev_block_height < *tip_height) {
Suggestion: invert the logic and add an explicit break here. It's a bit convoluted that block_height is reset above, and then if these conditions hold, it's updated, and then in the while condition if it's still null, then we break. I think this is clearer:
if (!locked_chain->getBlockHeight(prev_block_hash) || !prev_block_height < *tip_height) {
// rescan has reached the tip
break;
}
re: #14711 (review)
Suggestion: invert the logic and add an explicit break here
Done, good suggestion
56 | + virtual int64_t getBlockTime(int height) = 0; 57 | + 58 | + //! Get block median time past. 59 | + virtual int64_t getBlockMedianTimePast(int height) = 0; 60 | + 61 | + //! Check that the full block is available on disk (ie has not been
Nit: remove the word 'full' here
91 | + virtual bool isPotentialTip(const uint256& hash) = 0; 92 | + 93 | + //! Get locator for the current chain tip. 94 | + virtual CBlockLocator getLocator() = 0; 95 | + 96 | + //! Return height of block on the chain using locator.
Nit: This comment isn't quite right. The function returns "the latest block common to locator and chain", which is guaranteed to be an ancestor of the block on the active chain.
A bunch of nits, which can be ignored or addressed in a follow-up PR.
I do think you've changed the behaviour in a couple of places, notably in rescanblockchain(). That should be addressed before merge.
Will review further after #14711 (review) has been fixed. Also, I clarified the documentation of the rpc in that we follow the chain in case it "runs ahead" during the rpc (https://github.com/bitcoin/bitcoin/pull/15279)
John's comment #14711 (review) made me realize there seems to be a minor race condition and inconsistent behavior in the current implementation of ScanForWalletTransactions. If there is a reorg while cs_main is unlocked, there are two different checks to handle the case that pindex is no longer on the new chain. One of these checks will return ScanResult::SUCCESS while the other will return ScanResult::FAILURE, and which check is hit will depend the on timing of ReadBlockFromDisk and SyncTransactions calls.
The check after the ReadBlockFromDisk call leads to ScanResult::FAILURE:
The check after the SyncTransactions loop leads to ScanResult::SUCCESS:
https://github.com/bitcoin/bitcoin/blob/d6e700e40f861ddd6743f4d13f0d6f6bc19093c2/src/wallet/wallet.cpp#L1697 https://github.com/bitcoin/bitcoin/blob/d6e700e40f861ddd6743f4d13f0d6f6bc19093c2/src/wallet/wallet.cpp#L1664
It seems like the better thing to do would be return SUCCESS in both cases, since any blocks attached in a reorg should already be scanned for wallet transactions. Returning SUCCESS in both cases is the behavior currently implemented in this PR, but I think I should add some comments about it to clarify and maybe open a separate PR to avoid changing the behavior here.
I'd prefer to keep this refactoring-only and would suggest to open a pr for that either before or after this one is merged.
Updated 1da8b4648e6b5f01a90acfe487bcda6f7a5a0424 -> 44de1561aaf7556bb8ae8b582c233742ff76767d (pr/wchain2.9 -> pr/wchain2.10, compare) implementing suggestions from John.
re: #14711 (comment)
I'd prefer to keep this refactoring-only and would suggest to open a pr for that either before or after this one is merged.
Agreed. I just added a TODO referencing #14711 (comment). I was going to open a new PR too, but it turns out to be difficult to write a test for the new behavior, so I am putting it off for now.
3432 | + // We can't rescan beyond non-pruned blocks, stop and throw an error 3433 | + if (locked_chain->findPruned(start_height, stop_height)) { 3434 | + throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height."); 3435 | + } 3436 | + 3437 | + if (tip_height) {
In commit db21f02648 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis
nit: Not sure why you check for this. In current operation tip_height is never nullopt, and when that should change in the future, you might as well return with an early error instead of calling into ScanForWalletTransactions, whose documentation states that the start block is required to be set. (I know that it probably will pass through just fine even if it is not provided, but we shouldn't rely on that. I also know that there is a unit test to check that behaviour, but it was probably added because of a misunderstanding of the documentation of ScanForWalletTransactions).
re: #14711 (review)
nit: Not sure why you check for this
In this case even though "documentation states" and "In current operation" the chain will never be empty, I think handling a rescan of an empty chain by just returning success is more natural than making assumptions about when the chain is initialized or what other code expects it to be initialized. If you think adding the assumption will simplify things, though, I'd be happy to take a specific suggestion or review a change.
I think handling a rescan of an empty chain by just returning success ...
It would also report that the rescan started at block 0, which does not exist, since the chain was not initialized.
A user calling this function assumes that the chain was initialized and should be notified that it wasn't instead of returning {start_height=0, stop_height=null}, indicating success.
Also, we already throw an error if the chain wasn't initialized and the user supplies at least one of the optional parameters.
I'd suggest the following code change:
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index c96a9b0aff..ba8d3750b5 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3432,22 +3432,27 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
uint256 start_block, stop_block;
{
auto locked_chain = pwallet->chain().lock();
- Optional<int> tip_height = locked_chain->getHeight();
+ int tip_height;
+ if (Optional<int> tip_height_opt = locked_chain->getHeight()) {
+ tip_height = *tip_height_opt;
+ } else {
+ throw JSONRPCError(RPC_MISC_ERROR, "Chain not initialized, nothing to rescan");
+ }
if (!request.params[0].isNull()) {
start_height = request.params[0].get_int();
- if (start_height < 0 || !tip_height || start_height > *tip_height) {
- throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid start_height");
- }
+ }
+ if (start_height < 0 || start_height > tip_height) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid start_height");
}
Optional<int> stop_height;
if (!request.params[1].isNull()) {
stop_height = request.params[1].get_int();
- if (*stop_height < 0 || !tip_height || *stop_height > *tip_height) {
+ if (*stop_height < 0 || *stop_height > tip_height) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid stop_height");
}
- else if (*stop_height < start_height) {
+ if (*stop_height < start_height) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "stop_height must be greater than start_height");
}
}
@@ -3457,11 +3462,9 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.");
}
- if (tip_height) {
- start_block = locked_chain->getBlockHash(start_height);
- if (stop_height) {
- stop_block = locked_chain->getBlockHash(*stop_height);
- }
+ start_block = locked_chain->getBlockHash(start_height);
+ if (stop_height) {
+ stop_block = locked_chain->getBlockHash(*stop_height);
}
}
3460 | } 3461 | UniValue response(UniValue::VOBJ); 3462 | - response.pushKV("start_height", pindexStart->nHeight); 3463 | - response.pushKV("stop_height", stopBlock->nHeight); 3464 | + response.pushKV("start_height", start_height); 3465 | + response.pushKV("stop_height", result.stop_height ? *result.stop_height : UniValue());
db21f02648 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis
nit: In line with my previous comment, I'd prefer if we either assumed that stop_height is always set or we return an error. An alternative would be to adjust the documentation of the rpc result to mention that in (whatever obscure circumstances) stop_height might be null, but I'd prefer to throw an rpc error for such circumstances.
re: #14711 (review)
An alternative would be to adjust the documentation of the rpc result to mention that in (whatever obscure circumstances) stop_height might be null
Updated documentation, this could happen rarely during a reorg
1606 | - return failedBlock->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1; 1607 | + ScanResult result = ScanForWalletTransactions(start_block, {} /* stop_block */, reserver, update); 1608 | + if (result.status == ScanResult::FAILURE) { 1609 | + int64_t time_max; 1610 | + if (!chain().findBlock(result.failed_block, nullptr /* block */, nullptr /* time */, &time_max)) { 1611 | + throw std::logic_error("ScanForWalletTransactions returned invalid block hash");
in commit db21f02648 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis
unrelated request: I'd prefer if we had some guideline on when to use assert vs std::logic_error. This seems like a condition that can never be hit unless there is a coding error, in which case an assert appears more appropriate.
re: #14711 (review)
This seems like a condition that can never be hit unless there is a coding error, in which case an assert appears more appropriate.
I don't think logic_error means anything different from assert. https://en.cppreference.com/w/cpp/error/logic_error at least says "It reports errors that are a consequence of faulty logic within the program." I would use an assert over a logic error only in cases where it seemed safer to abort the entire process than to just abort the current request. Or maybe to match preexisting code.
1626 | - * @param[out] failed_block if FAILURE is returned, the most recent block 1627 | - * that could not be scanned, otherwise nullptr 1628 | - * @param[out] stop_block the most recent block that could be scanned, 1629 | - * otherwise nullptr if no block could be scanned 1630 | + * @param[in] start_block if not null, the scan will start at this block instead 1631 | + * of the genesis block
in commit db21f02648 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis
Is this correct? It seems that without a start_block, we'd just "fall through" and return instead of starting at the genesis block.
1640 | + * @pre Caller needs to make sure start_block (and the optional stop_block) are on 1641 | * the main chain after to the addition of any new keys you want to detect 1642 | * transactions for. 1643 | */ 1644 | -CWallet::ScanResult CWallet::ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const WalletRescanReserver& reserver, const CBlockIndex*& failed_block, const CBlockIndex*& stop_block, bool fUpdate) 1645 | +CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_block, const uint256& stop_block, const WalletRescanReserver& reserver, bool fUpdate)
With this rename of the second parameter (last_block -> stop_block) we now have a parameter named stop_block and a member in the return object called stop_block which refer to different things. I think this should change back to last_block to match the header file, and the several inline comments should be updated to say last_block.
EDIT: I see the RPC params are called start_block and stop_block, so this parameter naming does make sense. Could you rename the members of the return object and update the parameter names in the header file?
re: #14711 (review)
Could you rename the members of the return object and update the parameter names in the header file?
done
1629 | + * @param[in] start_block if not null, the scan will start at this block instead 1630 | + * of the genesis block 1631 | + * @param[in] stop_block if not null, the scan will stop at this block instead 1632 | + * of the chain tip 1633 | * 1634 | * @return ScanResult indicating success or failure of the scan. SUCCESS if
Needs updating now that ScanResult is an object.
1765 | - return ScanResult::USER_ABORT; 1766 | - } else if (pindex && ShutdownRequested()) { 1767 | - WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", pindex->nHeight, progress_current); 1768 | - return ScanResult::USER_ABORT; 1769 | + if (block_height && fAbortRescan) { 1770 | + WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", block_height.value_or(0), progress_current);
in commit db21f02648 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis
nit: the condition checks for block_height, so the value_or is dead code and can be removed?
1768 | - return ScanResult::USER_ABORT; 1769 | + if (block_height && fAbortRescan) { 1770 | + WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", block_height.value_or(0), progress_current); 1771 | + result.status = ScanResult::USER_ABORT; 1772 | + } else if (block_height && ShutdownRequested()) { 1773 | + WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", block_height.value_or(0), progress_current);
same
3434 | + throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height."); 3435 | + } 3436 | + 3437 | + if (tip_height) { 3438 | + start_block = locked_chain->getBlockHash(start_height); 3439 | + if (stop_height) {
A comment here could prevent other people from falling into the same trap. Something like:
// If called with a stop_height, set the stop_height here to trigger a rescan to that height.
// If called without a stop height, leave stop_height as null here so rescan continues to the tip (even if the tip advances during rescan).
re: #14711 (review)
A comment here could prevent other people from falling into the same trap. Something like:
added
121 | + { 122 | + if (::chainActive.Tip()->GetBlockHash() == hash) return true; 123 | + CBlockIndex* block = LookupBlockIndex(hash); 124 | + return block && block->GetAncestor(::chainActive.Height()) == ::chainActive.Tip(); 125 | + } 126 | + CBlockLocator getLocator() override { return ::chainActive.GetLocator(); }
in commit 44de1561aa
Could say getTipLocator()?
(reviewing 44de1561aaf7556bb8ae8b582c233742ff76767d)
Looks great. Thanks for being so receptive to my review comments! I have just three more inline, all to do with comments and naming.
utACK 44de1561aaf7556bb8ae8b582c233742ff76767d. Comments can be cleaned up in a follow-up PR if necessary.
utACK 44de1561aa
Couldn't find any issues with the code, just some questions on documentation and code-style.
Agreed, any comments/nits can be addressed in followup
Milestone
0.18.0