A RPC rescan command is much more flexible for the following reasons:
- You can define the start and end-height
- It can be called during runtime
- It can work in multiwallet environment
A RPC rescan command is much more flexible for the following reasons:
I'd rather have it that the API is such that an explicit rescan is never needed. Wasn't there some work on a multi-import w/ timestamps?
Yes. There is a PR (see PR description). I agree that it would be better to avoid rescans at all, although it might be complicated to catch all edge-cases and a manual trigger can help in situations where someone needs to deal with multiple/complex imports (you only want to do one rescan).
And I think some people will cancel a rescan because they want to do other stuff and/or had not considered that a rescan can take a long time. Afterward calling -rescan (from genesis) is a time consuming option.
But manually specifying a block # to rescan from is extremely fragile... it's very easy to get this wrong.
Also, rescanning doesn't interact with pruning which will be more and more common in the future.
How about we call this rescanfromheight instead, to make it possible to add a rescanfromtime later if users demand? Equally, some kind of RPC call that finds the first block with a nTime after a specific time might be useful here.
Do we have a way of querying what block # is the oldest non-pruned block?
It also occurs to me that for this usecase we might instead want to have pruning not happen automatically, but rather be an on-demand thing where the user specifies the oldest time they're interested in.
So the biggest negative I personally see here is that it furthers this misunderstanding that rescan is some thing users generally need to be doing. Until we added these non-rescan imports a user initiated rescan is something that never should have been needed (and indicated a serious bug we'd like to know about if it was). As a result of the -rescan argument there is now this whole cargo cult of people that rescan every time they're scarred by a shadow. I hate to further that.
But I can't deny how really useful this will be.
Hm. this could also take a stop argument, allowing you to scan single blocks or avoid rescan overlap. Also, I think all the wallet re-scanning should traverse its interval backwards-- for more instant gratification; though this would dork with the wallet transaction ordering... actually import at all breaks that, I should go talk to luke-jr about that.
537 | + pIndexRescan = chainActive.Genesis(); 538 | + 539 | + //We can't rescan beyond non-pruned blocks, stop and throw an error 540 | + if (fPruneMode) 541 | + { 542 | + if (fPruneMode)
Remove?
Oops. A rebase issue. Thanks for point out. Fixed.
agree with gmaxwell that this should scan a range of blocks
Agree with the stop parameter. Working on a implementation....
I would still prefer an approach that imports with birthdate instead of explicit rescanning.
Added a commit that allows providing a optional parameter with a height where the rescan should stop. @sipa: I agree that rescan height over a key/address birthday would be nice to have (see #6570). But a explicit rescan RPC call can be useful IMO. It's trivial to maintain and it can save lots of rescan-time on the user side. But agree, it has to be considered as "experts" feature.
What about implementing a threshold for autodetecting wether the parameter is a blockheight or timestamp (similar to LOCKTIME_THRESHOLD)?
@jonasschnelli see #6570 (comment) regarding your last comment.
ACK
If this is still moving forward - I tested it a bit (including pruned mode) and it looks fine to me. One suggestion is to make the start-height a required parameter so that the user specifies "rescanblockchain 1" to scan from genesis. If the start-height is < 1 or higher than current height, throw an error. Otherwise, ACK.
1086 | @@ -1084,6 +1087,8 @@ int CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate) 1087 | if (AddToWalletIfInvolvingMe(tx, &block, fUpdate)) 1088 | ret++; 1089 | } 1090 | + if (pindex == pindexStop)
Move to while condition?
Because it's inclusive?
Rebased.
I think there are still reasons to consider that PR. At the moment, it would really be useful. Even once we have #7551 (importmulti) it could see use cases for rescanblockchain.
I agree. Ideally, there will be no need to rescan. But in practice, rescans are sometimes required (I guess everyone who gave some users support has encountered that). IMO a rpc rescan commend with an optional hight is much more flexible then -rescan as a startup argument.
I think we should work on importmulti instead (or at least an importprivkey that takes a key birthdate as parameter), not on more ways to rescan.
I think we should work on importmulti instead (or at least an importprivkey that takes a key birthdate as parameter), not on more ways to rescan.
Yes. I can agree with that.
Another thing where this could be useful is restoring hd wallets
Since importmulti #7551 is merged, some use cases are covered by that.
I think we should work on importmulti instead (or at least an importprivkey that takes a key birthdate as parameter), not on more ways to rescan.
Tend to agree. Closing this for now.
IMO something like that would be very handy if you rescan a HD wallet (old backup). -rescan does not allow direct user feedback IMO rescanning an old HD backup should by default start at the height where we introduces HD (optional down to genesis).
Reopened and overhauled.
This now does replace the -rescan startup argument with a new RPC call rescanblockchain. The reasons for that are:
Using -rescan will cancel the startup with an error referring to the new RPC call.
366 | @@ -367,7 +367,7 @@ std::string HelpMessage(HelpMessageMode mode) 367 | #ifndef WIN32 368 | strUsage += HelpMessageOpt("-pid=<file>", strprintf(_("Specify pid file (default: %s)"), BITCOIN_PID_FILENAME)); 369 | #endif 370 | - strUsage += HelpMessageOpt("-prune=<n>", strprintf(_("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex and -rescan. " 371 | + strUsage += HelpMessageOpt("-prune=<n>", strprintf(_("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex"
Maybe still worth mentioning that pruning can get in the way of successfully importing wallet keys & rescanning.
1159 | + if (request.fHelp || request.params.size() > 2) 1160 | + throw std::runtime_error( 1161 | + "rescanblockchain (\"start-height\") (\"stop-height\")\n" 1162 | + "\nRescan the local blockchain for wallet related transactions.\n" 1163 | + "\nArguments:\n" 1164 | + "1. \"start-height\" (number, optional) blockheight where the rescan should start\n"
Maybe this should take either times or heights like the pruneblockchain RPC: https://github.com/bitcoin/bitcoin/blob/46771514fa86b9a5a0e0af34c1abfa1da22212f7/src/rpc/blockchain.cpp#L838
suggest you change the argument names to startheight and stopheight. No other rpcs use - as word delimiter.
Indeed. Fixed.
1187 | + CBlockIndex *block = chainActive.Tip(); 1188 | + while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) 1189 | + block = block->pprev; 1190 | + 1191 | + if (pIndexStart->nHeight < block->nHeight) 1192 | + throw JSONRPCError(RPC_WALLET_ERROR, "Can't rescan beyond pruned data.");
Might be helpful if error message could mention getblockchaininfo RPC for getting pruned height.
Fixed.
1180 | + 1181 | + if (!pIndexStart) 1182 | + pIndexStart = chainActive.Genesis(); 1183 | + 1184 | + //We can't rescan beyond non-pruned blocks, stop and throw an error 1185 | + if (fPruneMode)
Style might need to be updated for this code (moving opening brace to same line here, adding missing braces other lines)
Fixed.
89 | @@ -90,7 +90,7 @@ Reduce storage requirements by enabling pruning (deleting) of old 90 | blocks. This allows the pruneblockchain RPC to be called to 91 | delete specific blocks, and enables automatic pruning of old 92 | blocks if a target size in MiB is provided. This mode is 93 | -incompatible with \fB\-txindex\fR and \fB\-rescan\fR. Warning: Reverting this 94 | +incompatible with \fB\-txindex\fR. Warning: Reverting this
I think these files are automatically generated from -help output and could be reverted in the PR.
Fixed.
3870 | @@ -3865,7 +3871,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) 3871 | RegisterValidationInterface(walletInstance); 3872 | 3873 | CBlockIndex *pindexRescan = chainActive.Genesis(); 3874 | - if (!GetBoolArg("-rescan", false))
Is there any advantage to getting rid of the -rescan option if we still have keep all the rescan logic below? Is there a future cleanup or new feature that will be easier to implement with the option gone?
The -rescan option is global while this PR changes it (partially) to per-wallet. Still, -zapwallettx, etc. need to also be moved to per-wallet basis (I don't know how right now).
But removing the global -rescan option has to be done sooner or later if we want proper multiwallet.
But removing the global -rescan option has to be done sooner or later if we want proper multiwallet.
Do -zapwallettx, etc also need to be removed to support proper multiwallet?
IMO Yes. It's pure db utility. Either we move it to RPC or to a new wallet/db tool.
This is confusing: the if statement requires !needsRescan. Shouldn't that be the other way around? If needsRescan is true, we should rescan. I think the if statement should be:
if (needsRescan || GetBoolArg("-salvagewallet", true) || GetBoolArg("-zapwallettxes", true))
re: zapwallet and salvagewallet. I agree that they should be changed to RPCs for multiwallet.
No. I don't think so.
If that if statement is true, we load the wallet best block which prevents a complete rescan (so it's the opposite).
So we only load the wallet best block (== no rescan) if needsRescan is false and -salvagewallet and --zapwallettxes` have not been set.
Yes, you're right.
1511 | @@ -1508,6 +1512,9 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f 1512 | } else { 1513 | ret = nullptr; 1514 | } 1515 | + if (pindex == pindexStop) {
I don't understand what the use-case is for the stop argument. Can you describe a scenario where you would want to provide it? I can see how it might have been desirable before there was an abortrescan RPC to break a big scan up into little scans, but I don't see why you'd want it now.
See the discussion about the stop argument: #7061 (comment)
Should add some test coverage for this. One easy way might be to add a new Rescan.manual enum value here: https://github.com/bitcoin/bitcoin/blob/46771514fa86b9a5a0e0af34c1abfa1da22212f7/test/functional/import-rescan.py#L32, then put
if self.rescan == Rescan.manual:
self.node.rescanblockchain()
at the end of the do_import method.
1467 | @@ -1468,11 +1468,15 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
1468 | * before CWallet::nTimeFirstKey). Returns null if there is no such range, or
nit: update function comment to say that the rescan is up to pindexStop if it is non-null
Fixed.
1488 | @@ -1485,7 +1489,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f 1489 | while (pindex && nTimeFirstKey && (pindex->GetBlockTime() < (nTimeFirstKey - TIMESTAMP_WINDOW))) 1490 | pindex = chainActive.Next(pindex); 1491 | 1492 | - ShowProgress(_("Rescanning..."), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup 1493 | + ShowProgress(_("Rescanning..."), 0); // show rescan progress in GUI as dialog or on splashscreen
Need to remove more of this comment, so that it just reads:
// show rescan progress in GUI as dialog
But can't it also happens during startup when the GUI will show the progress only in the splash-screen and not in a dialog?
1170 | + 1171 | + LOCK2(cs_main, pwallet->cs_wallet); 1172 | + 1173 | + CBlockIndex *pIndexStart = NULL; 1174 | + CBlockIndex *pIndexStop = NULL; 1175 | + if (request.params.size() > 0 && request.params[0].isNum())
This should throw an error if the argument isn't a number, rather than continue. It should allow the argument to be Null. I think the if test you need is:
if (request.params.size() > 0 && !request.params[0].isNull()) {
1193 | + } 1194 | + 1195 | + if (pwallet) 1196 | + pwallet->ScanForWalletTransactions(pIndexStart, pIndexStop, true); 1197 | + 1198 | + return NullUniValue;
This RPC should return a message to the user if it is successful, eg "Blockchain rescanned from block <hash> height <height> to block <hash> height <height>. <numtx> transactions added to wallet". Otherwise it's impossible for the user to know whether the call was successful or not.
Should at least throw an error this isn't successful (the ScanForWalletTransactions call will return null on success, otherwise a pointer to the last failing block, so this should be pretty easy).
1173 | + CBlockIndex *pIndexStart = NULL; 1174 | + CBlockIndex *pIndexStop = NULL; 1175 | + if (request.params.size() > 0 && request.params[0].isNum()) 1176 | + pIndexStart = chainActive[request.params[0].get_int()]; 1177 | + 1178 | + if (request.params.size() > 1 && request.params[1].isNum())
should this return an error if heightstop is lower than heightstart? If heightstop is higher than the tip height?
166 | @@ -167,8 +167,8 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco 167 | // Call Salvage with fAggressive=true to 168 | // get as much data as possible. 169 | // Rewrite salvaged data to fresh wallet file 170 | - // Set -rescan so any missing transactions will be 171 | - // found. 172 | + // call rescanblockchain (RPC) so any missing
I can't see where rescanblockchain() is being called. Am I missing something?
It happens outside of this call (somewhere in wallet.cpp). But the comment doesn't say "It" calls rescanblockchain, it either says "you" can call rescanblockchain().
I don't understand. The comment is:
// Recovery procedure:
// move wallet file to wallet.timestamp.bak
// Call Salvage with fAggressive=true to
// get as much data as possible.
// Rewrite salvaged data to fresh wallet file
// call rescanblockchain (RPC) so any missing
// transactions will be found.
This function does:
but it doesn't call rescanblockchain, and I can't see rescanblockchain being called by the callers.
Yes. You right. The rescan is not part of the call. But the part that calls CDB::Recover does always call a rescan. But now I got your point. We should refer to ScanForWalletTransactions() instead to rescanblockchain (RPC). Right?
sorry, I'm still not seeing it. You say "the part that calls CDB::Recover does always call a rescan". Where?
It's confusing. But CDB::Recover only gets calls by setting -salvagewallet which does set -rescan. Maybe I should remove that comment part... I don't know what's best because -rescan is currently mentioned there.
ah, makes sense now. Thank you. I wasn't looking in init.cpp, but now I think I see how this fits together. CWallet::Verify() is called first, which calls CWalletDB::Recover() if -salvagewallet is set. Later in AppInitMain(), we call CWallet::AppInitMain(), which is where the rescan happens if -salvagewallet is set.
Perhaps change the comment to something like:
// Try to recover the wallet:
// - move wallet file to wallet.timestamp.bak
// - call Salvage with fAggressive=true to get as much data as possible.
// - rewrite salvaged data to fresh wallet file
//
// CWallet::AppInitMain() will later run a full blockchain rescan to find
// any missing transactions.
This needs rebase (probably just wait till after 15 at this point). I think outstanding objections to the idea have all largely been removed, Concept ACK from me, at least.
Rebased.
1183 | + 1184 | + LOCK2(cs_main, pwallet->cs_wallet); 1185 | + 1186 | + CBlockIndex *pIndexStart = NULL; 1187 | + CBlockIndex *pIndexStop = NULL; 1188 | + if (request.params.size() > 0 && request.params[0].isNum()) {
Size checks can be dropped here and below. params[0] will return a null value if the param is missing.
1198 | + } 1199 | + 1200 | + //We can't rescan beyond non-pruned blocks, stop and throw an error 1201 | + if (fPruneMode) { 1202 | + CBlockIndex *block = chainActive.Tip(); 1203 | + while (block && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
This should check against pIndexStop too, so error won't trigger unnecessarily in cases where missing blocks are noncontinuous (which can happen with manual pruning).
1171 | + if (request.fHelp || request.params.size() > 2) { 1172 | + throw std::runtime_error( 1173 | + "rescanblockchain (\"startheight\") (\"stopheight\")\n" 1174 | + "\nRescan the local blockchain for wallet related transactions.\n" 1175 | + "\nArguments:\n" 1176 | + "1. \"startHeight\" (number, optional) blockheight where the rescan should start\n"
Capitalization of height doesn't match actual param name.
1172 | + throw std::runtime_error( 1173 | + "rescanblockchain (\"startheight\") (\"stopheight\")\n" 1174 | + "\nRescan the local blockchain for wallet related transactions.\n" 1175 | + "\nArguments:\n" 1176 | + "1. \"startHeight\" (number, optional) blockheight where the rescan should start\n" 1177 | + "2. \"stopHeight\" (number, optional) blockheight where the rescan should stop\n"
Could do what pruneblockchain does and allow height to be specified either as a physical height or a time: https://github.com/bitcoin/bitcoin/blob/1caafa6cde3b88d926611771f9b4c06fcc6e0007/src/rpc/blockchain.cpp#L855
utACK 7858b9fc739bdbf8dc5916fb17a7d44811bbe919, but I would prefer if this just deprecated the -rescan option instead of removing it. Removing it with no warning seems needless and kind of jerky, since it barely saves any code.
Rebased and addressed @ryanofsky 's comments on my rpc_rescan branch.
Cherry picked @luke-jr rebased / overhauled version. Still needs an RPC test.
1168 | + return NullUniValue; 1169 | + } 1170 | + 1171 | + if (request.fHelp || request.params.size() > 2) { 1172 | + throw std::runtime_error( 1173 | + "rescanblockchain (\"startheight\") (\"stopheight\")\n"
Remove inner ) (.
No... they're both independently optional.
But without named args that's not possible.
Yes it is...
How?
JSON null is equivalent to omitting a parameter.
What I mean is that with bitcoin-cli rescanblockchain 10 10 will always be startheight and to specify only stopheight you have to call bitcoin-cli rescanblockchain null 10, hence stopheight depends on startheight.
Not sure what you're getting at. Null is omitted.
1171 | + if (request.fHelp || request.params.size() > 2) { 1172 | + throw std::runtime_error( 1173 | + "rescanblockchain (\"startheight\") (\"stopheight\")\n" 1174 | + "\nRescan the local blockchain for wallet related transactions.\n" 1175 | + "\nArguments:\n" 1176 | + "1. \"startheight\" (number, optional) blockheight where the rescan should start\n"
Just start?
Also, if positive then height, if negative then depth relative to tip. Same for stop.
if positive then height, if negative then depth relative to tip
In general we don't like overloading arguments with multiple meanings in this way (I know - I've tried to do it myself before and got NACKed.
The argument names should be changed to start_height and stop_height to follow style guidelines. I know that this PR was opened before the style changes so I won't suggest that you change variable names, but this is a public interface so it should adhere to the new guidelines.
1183 | + 1184 | + LOCK2(cs_main, pwallet->cs_wallet); 1185 | + 1186 | + CBlockIndex *pindexStart = NULL; 1187 | + CBlockIndex *pindexStop = NULL; 1188 | + if (request.params[0].isNum()) {
IMO if (!request.params[0].isNull()) so that get_int throws if invalid value is supplied.
1190 | + } 1191 | + if (!pindexStart) { 1192 | + pindexStart = chainActive.Genesis(); 1193 | + } 1194 | + 1195 | + if (request.params[1].isNum()) {
Same as above.
1196 | + pindexStop = chainActive[request.params[1].get_int()]; 1197 | + } 1198 | + 1199 | + if (pindexStop && pindexStop->nHeight < pindexStart->nHeight) { 1200 | + // Flip the parameters to the expected order 1201 | + CBlockIndex * const tmp = pindexStart;
Use std::swap?
1195 | + if (request.params[1].isNum()) { 1196 | + pindexStop = chainActive[request.params[1].get_int()]; 1197 | + } 1198 | + 1199 | + if (pindexStop && pindexStop->nHeight < pindexStart->nHeight) { 1200 | + // Flip the parameters to the expected order
Should be an error instead?
Why?
Why would start be greater than stop? Seems to be a bad call?
It doesn't make much less sense to scan backward than to scan forward. We only actually support scanning forward, but the outcome is the same either way.
I see your point :+1:.
1553 | +CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, bool fUpdate) 1554 | { 1555 | int64_t nNow = GetTime(); 1556 | const CChainParams& chainParams = Params(); 1557 | 1558 | + if (pindexStop && pindexStop->nHeight < pindexStart->nHeight) {
Should not happen, assert instead?
Can happen from the perspective of the function. But maybe not with the current consumption of the function.
1204 | + } 1205 | + 1206 | + // We can't rescan beyond non-pruned blocks, stop and throw an error 1207 | + if (fPruneMode) { 1208 | + int nHeight = pindexStart->nHeight; 1209 | + while ((!pindexStop) || nHeight <= pindexStop->nHeight) {
Correct me if I'm wrong, but if start has BLOCK_HAVE_DATA then the remaining blocks also have, therefore no need to loop?
I think this may be currently true, but perhaps not safe to assume for the future. Unsure.
Maybe factor out to a CChain function so in the future it's more contained and for now remove the loop?
We loop backwards here which the check is necessary. Or am I wrong?
Something is wrong now because nothing changes the the loop exit conditions.
1181 | + ); 1182 | + } 1183 | + 1184 | + LOCK2(cs_main, pwallet->cs_wallet); 1185 | + 1186 | + CBlockIndex *pindexStart = NULL;
pindex_start?
1192 | + pindexStart = chainActive.Genesis(); 1193 | + } 1194 | + 1195 | + if (request.params[1].isNum()) { 1196 | + pindexStop = chainActive[request.params[1].get_int()]; 1197 | + }
Maybe we should } else { pindex_stop = chainActive.Tip(); }?
The stop height can just be a nullptr (== scan up to the tip).
1172 | + throw std::runtime_error( 1173 | + "rescanblockchain (\"startheight\") (\"stopheight\")\n" 1174 | + "\nRescan the local blockchain for wallet related transactions.\n" 1175 | + "\nArguments:\n" 1176 | + "1. \"startheight\" (number, optional) blockheight where the rescan should start\n" 1177 | + "2. \"stopheight\" (number, optional) blockheight where the rescan should stop\n"
Note that it's inclusive?
Need to change the progress calculation:
double dProgressTip = GuessVerificationProgress(chainParams.TxData(), chainActive.Tip());
Should rename to dProgressStop.
1219 | + 1220 | + if (pwallet) { 1221 | + pwallet->ScanForWalletTransactions(pindexStart, pindexStop, true); 1222 | + } 1223 | + 1224 | + return NullUniValue;
Could return the scanned range? Then the client can keep the returned stopheight to be the next startheight.
1209 | + while ((!pindexStop) || nHeight <= pindexStop->nHeight) { 1210 | + CBlockIndex * const block = chainActive[nHeight]; 1211 | + if (!block) { 1212 | + break; 1213 | + } 1214 | + if (!(block->pprev->nStatus & BLOCK_HAVE_DATA)) {
block->pprev can be nullptr:
bitcoind -regtest -prune=1
bitcoin-cli -regtest rescanblockchain <- segfault
Rebased and overhauled.
This no longer evicts the startup -rescan option.
Also added a RPC test.
1190 | + pindexStart = chainActive[request.params[0].get_int()]; 1191 | + if (!pindexStart) { 1192 | + throw JSONRPCError(RPC_WALLET_ERROR, "Invalid startheight"); 1193 | + } 1194 | + } 1195 | + if (!pindexStart) {
Why not just assign chainActive.Genesis() to pindexStart when declaring the variable (and then overwrite if !request.params[0].isNull())
Makes sense. Will update.
1183 | + } 1184 | + 1185 | + LOCK2(cs_main, pwallet->cs_wallet); 1186 | + 1187 | + CBlockIndex *pindexStart = nullptr; 1188 | + CBlockIndex *pindexStop = nullptr;
nit: slight preference to declare this variable immediately above the if (!request.params[1].isNull()) code block
IMO they should be declared / initialised together (for better code readability).
1187 | + CBlockIndex *pindexStart = nullptr; 1188 | + CBlockIndex *pindexStop = nullptr; 1189 | + if (!request.params[0].isNull()) { 1190 | + pindexStart = chainActive[request.params[0].get_int()]; 1191 | + if (!pindexStart) { 1192 | + throw JSONRPCError(RPC_WALLET_ERROR, "Invalid startheight");
This should be RPC_INVALID_PARAMETER, not RPC_WALLET_ERROR
1197 | + } 1198 | + 1199 | + if (!request.params[1].isNull()) { 1200 | + pindexStop = chainActive[request.params[1].get_int()]; 1201 | + if (!pindexStop || pindexStop->nHeight < pindexStart->nHeight) { 1202 | + throw JSONRPCError(RPC_WALLET_ERROR, "Invalid stopheight");
As above, this should be RPC_INVALID_PARAMETER, not RPC_WALLET_ERROR
If the heights are inverted, they should be flipped. 100..90 is no less rational than 90..100. This was in 79a11731f3, but seems to have disappeared silently in a rebase?
'The right thing' is a value judgement. In my opinion, the right thing is to keep APIs as tightly defined as possible in order to keep code simple and remove corner cases.
Of course, if there's a legitimate use case, we should allow both, but I don't see it.
The legitimate use case is to rescan the blockchain... There's no reason why one direction is to be preferred over the other for these arguments.
1209 | + while (block) { 1210 | + if (block->nHeight <= pindexStart->nHeight) { 1211 | + break; 1212 | + } 1213 | + if (!(block->nStatus & BLOCK_HAVE_DATA)) { 1214 | + throw JSONRPCError(RPC_WALLET_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.");
This should be RPC_MISC_ERROR. Trying to scan a pruned block is not an error in the wallet.
1216 | + block = block->pprev; 1217 | + } 1218 | + } 1219 | + 1220 | + CBlockIndex *stopBlock = nullptr; 1221 | + if (pwallet) {
why is this required? The call to EnsureWalletIsAvailable() above should ensure that the wallet is available.
Right... Will remove it. I think this is a rebase issue since the PR is originally from 2015.
1225 | + stopBlock = pindexStop ? pindexStop : chainActive.Tip(); 1226 | + } 1227 | + } 1228 | + 1229 | + UniValue response(UniValue::VOBJ); 1230 | + response.pushKV("startheight", pindexStart->nHeight);
These return fields should be documented in the help text.
918 | @@ -919,7 +919,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface 919 | void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock) override; 920 | bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate); 921 | int64_t RescanFromTime(int64_t startTime, bool update); 922 | - CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate = false); 923 | + CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop = nullptr, bool fUpdate = false);
It looks like you've updated all the calls to ScanForWalletTransactions() to explicitly set pindexStop (except in wallet_tests.cpp). Why not update wallet_tests.cpp and remove the default argument. I think in general it's best to avoid default arguments if possible.
90 | @@ -91,6 +91,21 @@ def run_test (self): 91 | self.start_node(1, extra_args=self.extra_args[1] + ['-rescan']) 92 | assert_equal(self.nodes[1].getbalance(), num_hd_adds + 1) 93 | 94 | + # Try a RPC based rescan 95 | + self.stop_node(1) 96 | + shutil.rmtree(tmpdir + "/node1/regtest/blocks")
Really, this should use os.path.join to construct the path, but I see this pattern is used in other tests, so I don't think you need to change it here.
Even better would be to move these functions to be methods in the TestNode class - remove_block_files(), remove_chainstate_files(), backup_wallet_file(), restore_wallet_file(), etc.
97 | + shutil.rmtree(tmpdir + "/node1/regtest/chainstate") 98 | + shutil.copyfile(tmpdir + "/hd.bak", tmpdir + "/node1/regtest/wallet.dat") 99 | + self.start_node(1, extra_args=self.extra_args[1]) 100 | + connect_nodes_bi(self.nodes, 0, 1) 101 | + self.sync_all() 102 | + out = self.nodes[1].rescanblockchain(0,1)
nit: add a space: (0, 1)
102 | + out = self.nodes[1].rescanblockchain(0,1) 103 | + assert_equal(out['startheight'], 0) 104 | + assert_equal(out['stopheight'], 1) 105 | + out = self.nodes[1].rescanblockchain() 106 | + assert_equal(out['startheight'], 0) 107 | + assert_equal(self.nodes[1].getbalance(), num_hd_adds + 1)
Perhaps assert that the rescan scanned up to the tip:
assert_equal(out['stopheight'], self.nodes[1].getblockcount())
Looks generally good. A few nits inline.
3235 | @@ -3235,6 +3236,7 @@ static const CRPCCommand commands[] = 3236 | { "wallet", "removeprunedfunds", &removeprunedfunds, {"txid"} }, 3237 | 3238 | { "generating", "generate", &generate, {"nblocks","maxtries"} }, 3239 | + { "wallet", "rescanblockchain", &rescanblockchain, {"startheight", "stopheight"} },
I think this was more appropriately positioned after "removeprunedfunds" as before. Probably after "move" makes the most sense, since it seems to be alphabetised.
Also not sure rpcdump.cpp makes sense for this one.
3199 | + CBlockIndex *pindexStart = chainActive.Genesis(); 3200 | + CBlockIndex *pindexStop = nullptr; 3201 | + if (!request.params[0].isNull()) { 3202 | + pindexStart = chainActive[request.params[0].get_int()]; 3203 | + if (!pindexStart) { 3204 | + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid startheight");
μNit: start_height since you're calling it that in the help.
3206 | + } 3207 | + 3208 | + if (!request.params[1].isNull()) { 3209 | + pindexStop = chainActive[request.params[1].get_int()]; 3210 | + if (!pindexStop || pindexStop->nHeight < pindexStart->nHeight) { 3211 | + throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid stopheight");
μNit: stop_height
3212 | + } 3213 | + } 3214 | + 3215 | + // We can't rescan beyond non-pruned blocks, stop and throw an error 3216 | + if (fPruneMode) { 3217 | + CBlockIndex *block = pindexStop ? pindexStop : chainActive.Tip();
Can be abbreviated to CBlockIndex *block = pindexStop ?: chainActive.Tip();
Fixed.
3216 | + if (fPruneMode) { 3217 | + CBlockIndex *block = pindexStop ? pindexStop : chainActive.Tip(); 3218 | + while (block) { 3219 | + if (block->nHeight <= pindexStart->nHeight) { 3220 | + break; 3221 | + }
Why not just while (block && block->nHeight > pindexStart->nHeight) {?
Fixed.
3228 | + 3229 | + CBlockIndex *stopBlock = nullptr; 3230 | + stopBlock = pwallet->ScanForWalletTransactions(pindexStart, pindexStop, true); 3231 | + if (!stopBlock) { 3232 | + // if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex 3233 | + stopBlock = pindexStop ? pindexStop : chainActive.Tip();
Same as above here; stopBlock = pindexStop ?: chainActive.Tip();
Fixed.
918 | @@ -919,7 +919,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface 919 | void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock) override; 920 | bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate); 921 | int64_t RescanFromTime(int64_t startTime, bool update); 922 | - CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate = false); 923 | + CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, bool fUpdate = false);
CBlockIndex* pindexStop = nullptr will let existing code default to current behavior (mostly in the test cases in wallet/test/wallet_tests.cpp).
Heh. I had it before but then @jnewbery convinced me to remove it: #7061 (review)
utACK fe471a9d5a0ac7b10765f99dae0741410f85804d
Did not look at test code yet.
Adressed @kallewoof's points.
3176 | + return NullUniValue; 3177 | + } 3178 | + 3179 | + if (request.fHelp || request.params.size() > 2) { 3180 | + throw std::runtime_error( 3181 | + "rescanblockchain (\"startheight\") (\"stopheight\")\n"
I missed this last time: start_height and stop_height here to match the rest.
Fixed.
utACK 6a51097090cda330a9d83be61be28baed8b0f11d
re-utACK e2d376b376c4861d90ee1a9c52933abb3cbbd03b
3183 | + "\nArguments:\n" 3184 | + "1. \"start_height\" (number, optional) block height where the rescan should start\n" 3185 | + "2. \"stop_height\" (number, optional) block height where the rescan should stop\n" 3186 | + "\nResult:\n" 3187 | + "{\n" 3188 | + " \"start_height\" : (number) The block height where the rescan has started\n"
Nit, remove space before :.
utACK e2d376b.
Will test tomorrow. While there's few ACK's why not update variables to snake case?
3179 | + if (request.fHelp || request.params.size() > 2) { 3180 | + throw std::runtime_error( 3181 | + "rescanblockchain (\"start_height\") (\"stop_height\")\n" 3182 | + "\nRescan the local blockchain for wallet related transactions.\n" 3183 | + "\nArguments:\n" 3184 | + "1. \"start_height\" (number, optional) block height where the rescan should start\n"
nit: add "If omitted, rescan starts from the genesis block."
Or just default=0
3180 | + throw std::runtime_error( 3181 | + "rescanblockchain (\"start_height\") (\"stop_height\")\n" 3182 | + "\nRescan the local blockchain for wallet related transactions.\n" 3183 | + "\nArguments:\n" 3184 | + "1. \"start_height\" (number, optional) block height where the rescan should start\n" 3185 | + "2. \"stop_height\" (number, optional) block height where the rescan should stop\n"
nit: add "If omitted, rescan stops at the chain tip."
Please specify if the scan stops before or after this block.
I think semantically it makes more sense to provide an inclusive range (if I say "scan blocks 10-20", not scanning 20 seems confusing).
If you really want an exclusive range, providing the number of blocks to scan (e.g., "scan 10 blocks starting at 10") has better ux, but still worse than inclusive.
3224 | + } 3225 | + 3226 | + CBlockIndex *stopBlock = nullptr; 3227 | + stopBlock = pwallet->ScanForWalletTransactions(pindexStart, pindexStop, true); 3228 | + if (!stopBlock) { 3229 | + // if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex
minor bug: sadly, the comment about the return value for ScanForWalletTransactions() added in #10208 is incomplete. If abortrescan is called during a rescan, then ScanForWalletTransactions() will return nullptr even if the though scan didn't complete successfully to the tip (or requested pindexStop after this PR)
I think the only way to correctly return the status of the rescan would be to have more information passed back by ScanForWalletTransactions().
(Test this by running rescanblockchain in one window and calling abortrescan in a different window. rescanblockchain returns the requested stop_height even though the rescan didn't extend that far).
@ryanofsky since he changed the semantics of ScanForWalletTransactions() in #10208.
Nice catch... added a protection that detects aborted rescans via a check of CWallet::IsAbortingRescan() after ScanForWalletTransactions().
Seems like a good pragmatic change. There's a (pathological) race condition where another thread starts a rescan and resets fAbortRescan before this RPC returns, but that's extremely unlikely.
I still think it would be nice to have ScanForWalletTransactions() give a more meaningful return value that indicates whether the rescan was aborted. The same bug exists in importmulti where RescanFromTime() will return the wrong time value if the rescan is aborted. However, that fix can be for another PR.
2 nits and a minor bug
9 | @@ -10,6 +10,7 @@ 10 | connect_nodes_bi, 11 | ) 12 | import shutil 13 | +import os
nit: please sort imports :)
and ideally place them in PEP8 ordering (ie standard library before project imports)
Looks good. One style nit, but ACK bdae58ebcd274629bc57f790ef3e1c9e13e1c91f with or without.
3221 | + } 3222 | + block = block->pprev; 3223 | + } 3224 | + } 3225 | + 3226 | + CBlockIndex *stopBlock = nullptr;
Join this with next line.
3228 | + if (!stopBlock) { 3229 | + if (pwallet->IsAbortingRescan()) { 3230 | + throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted."); 3231 | + } 3232 | + // if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex 3233 | + stopBlock = pindexStop ?: chainActive.Tip();
Avoid this GNU extension (empty operand)?
BTW, first time in this source code.
I did not know a ?: b was a GNU extension. Sorry for bad nit earlier. :(
Fixed.
3227 | + stopBlock = pwallet->ScanForWalletTransactions(pindexStart, pindexStop, true); 3228 | + if (!stopBlock) { 3229 | + if (pwallet->IsAbortingRescan()) { 3230 | + throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted."); 3231 | + } 3232 | + // if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex
This is wrong, from ScanForWalletTransactions doc:
Returns null if scan was successful. Otherwise, if a complete rescan was not possible (due to pruning or corruption), returns pointer to the most recent block that could not be scanned.
So it can return a non nullptr but catch transactions further in the chain. It only stops rescanning when abortrescan RPC is called.
Maybe we should add the most recent block that could not be scanned to the response.
It's not correct, right. I think we leave that fix for another PR (not directly related to this PR).
3205 | + } 3206 | + } 3207 | + 3208 | + if (!request.params[1].isNull()) { 3209 | + pindexStop = chainActive[request.params[1].get_int()]; 3210 | + if (!pindexStop || pindexStop->nHeight < pindexStart->nHeight) {
I think with this (and with the above pindexStart check), I'd like to see these errors differentiate between:
At the very least, 5. should be represented because it could be the start_height which is invalid.
Added case 5.
3215 | + // We can't rescan beyond non-pruned blocks, stop and throw an error 3216 | + if (fPruneMode) { 3217 | + CBlockIndex *block = pindexStop ?: chainActive.Tip(); 3218 | + while (block && block->nHeight > pindexStart->nHeight) { 3219 | + if (!(block->nStatus & BLOCK_HAVE_DATA)) { 3220 | + throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.");
There is a race condition for using getblockchaininfo RPC call if a new block comes in, unless you can pause pruning.
Maybe worth just documenting to retry until there is a pruning pause feature.
3183 | + "\nArguments:\n" 3184 | + "1. \"start_height\" (number, optional) block height where the rescan should start\n" 3185 | + "2. \"stop_height\" (number, optional) block height where the rescan should stop\n" 3186 | + "\nResult:\n" 3187 | + "{\n" 3188 | + " \"start_height\" (number) The block height where the rescan has started. If omitted, rescan starts from the genesis block.\n"
tense error s/start/started
3184 | + "1. \"start_height\" (number, optional) block height where the rescan should start\n" 3185 | + "2. \"stop_height\" (number, optional) block height where the rescan should stop\n" 3186 | + "\nResult:\n" 3187 | + "{\n" 3188 | + " \"start_height\" (number) The block height where the rescan has started. If omitted, rescan starts from the genesis block.\n" 3189 | + " \"stop_height\" (number) The height of the last rescanned block. If omitted, rescan stops at the chain tip.\n"
tense error s/stops/stopped
This is not accurate AFAICT. The block at stop_height is not scanned?
AFAIK the block defined with stop_height will be scanned.
3232 | + // if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex 3233 | + stopBlock = pindexStop ?: chainActive.Tip(); 3234 | + } 3235 | + 3236 | + UniValue response(UniValue::VOBJ); 3237 | + response.pushKV("start_height", pindexStart->nHeight);
If no blocks are re scanned, I don't think this is correct/is confusing.
3233 | + stopBlock = pindexStop ?: chainActive.Tip(); 3234 | + } 3235 | + 3236 | + UniValue response(UniValue::VOBJ); 3237 | + response.pushKV("start_height", pindexStart->nHeight); 3238 | + response.pushKV("stop_height", stopBlock->nHeight);
According to the ScanForWalletTransactions docs...
* Returns null if scan was successful. Otherwise, if a complete rescan was not
* possible (due to pruning or corruption), returns pointer to the most recent
* block that could not be scanned.
So with the current returned value it should be nHeight-1.
Concept Ack.
However, the handling of the ranges is incorrect afaict and needs to be corrected (and better documented).
@JeremyRubin the current implementation does scan the block defined with the stop_height parameter. I'll update the parameter documentation to make this more clear.
I think the range handling is correct but change the RPC help to \"stop_height\" (number, optional) the last block height that should be scanned.
Reverted the conditional ?: op abbreviation due to the fact that this requires a GNU extension.
The fix for the misleading return value of ScanForWalletTransaction and comment (#11450) should be handled outside of this PR.
1554 | @@ -1555,12 +1555,19 @@ int64_t CWallet::RescanFromTime(int64_t startTime, bool update) 1555 | * Returns null if scan was successful. Otherwise, if a complete rescan was not 1556 | * possible (due to pruning or corruption), returns pointer to the most recent 1557 | * block that could not be scanned. 1558 | + * 1559 | + * If pindexStop is not a nullptr, the scan will stop one block after the block-index
I think this is clearest to say:
If pindexStop is not a nullptr, we attempt to scan up to and including pIndexStop.
Ok. Let's say that we say run
rescanblockchain { start_height: 10, stop_height: 20 }
then in ScanForWalletTransactions, we fail at 20.
We will return
{ start_height: 10, stop_height: 20 }
which isn't true, because we failed to scan 20.
This is also true if we fail, at say, 15.
We will return
{ start_height: 10, stop_height: 15 }
Which is also not true because we didn't scan 15.
Also, more generally, it seems we could fail to scan many blocks in the range and still return that we scanned the range.
@JeremyRubin: Why do you think rescanblockchain { start_height: 10, stop_height: 20 } would not scan block at height 20? I just re-checked, re-tested and it looks like it does scan block 20.
I understand @JeremyRubin concern now. It's about corrupted blocks (when ReadBlockFromDisk fails). The current startup -rescans do sort of tolerate this.
Added a fix that now leads to an error thrown when detecting a corrupted block (https://github.com/bitcoin/bitcoin/pull/7061/files#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR3237)
Concept ACK. This seem like moving in the right direction, even if in the long term we want to avoid the need for rescans completely.
This now does replace the -rescan startup argument with a new RPC call rescanblockchain.
I don't see this in the code. Shouldn't we at least deprecate the startup argument at the same time? (I would not oppose to directly remove it as an exception to the general policy instead of waiting for 0.17). Perhaps note in the release notes that this is also supposed to be temporary.
This now does replace the -rescan startup argument with a new RPC call rescanblockchain.
I don't see this in the code. Shouldn't we at least deprecate the startup argument at the same time? (I would not oppose to directly remove it as an exception to the general policy instead of waiting for 0.17). Perhaps note in the release notes that this is also supposed to be temporary.
This was removed from the PRs description (but not from a later comment, now added a strike-through attr.)
IMO this is ready to merge even though there are some concerns that need to be addressed in follow ups:
rescanblockchain can be refactored a little to avoid the cs_main and cs_wallet locks;I also would like to discuss the option to make this RPC asynchronous so the caller doesn't wait for the rescan to complete, it only asks for a rescan. There is a big chance the caller interrupts the call, but I believe in server side the rescan continues.
utACK 559542a.
3233 | + } 3234 | + // if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex 3235 | + stopBlock = pindexStop ? pindexStop : chainActive.Tip(); 3236 | + } 3237 | + else { 3238 | + throw JSONRPCError(RPC_MISC_ERROR, "Rescan failed. Potentially corruputed data files.");
Typo corruputed -> corrupted
3187 | + "{\n" 3188 | + " \"start_height\" (number) The block height where the rescan has started. If omitted, rescan started from the genesis block.\n" 3189 | + " \"stop_height\" (number) The height of the last rescanned block. If omitted, rescan stopped at the chain tip.\n" 3190 | + "}\n" 3191 | + "\nExamples:\n" 3192 | + + HelpExampleCli("rescanblockchain", "\"100000 120000\"")
These quotes look wrong, should both start and stop height be surrounded in the same string? As numbers should they even have quotes around them?
HelpExampleRpc should also have commas between the arguments I believe
Small nit, number -> numeric in the lines above to be consistent with other calls
re-utACK https://github.com/bitcoin/bitcoin/pull/7061/commits/559542a57bfd4bd37c2f9d8397ec62aef32b67e5 modulo comments above
Fixed @MeshCollider nits.
I still think it's worth it to handle
and
differently. Specifically, the latter calls could still be handled and processed.
Tested ACK 35e7fd1147c64a286f778c7740fa735a16d448c5
I still think it's worth it to handle ... differently.
These already fail with Invalid start_height and Invalid stop_height. Yes, we can always provide more detailed error messages or logging, but lets not hold this PR up on that. It's already been very heavily reviewed.
I'll happily review follow-up PRs if you want to change error logging.
3188 | + " \"start_height\" (numeric) The block height where the rescan has started. If omitted, rescan started from the genesis block.\n" 3189 | + " \"stop_height\" (numeric) The height of the last rescanned block. If omitted, rescan stopped at the chain tip.\n" 3190 | + "}\n" 3191 | + "\nExamples:\n" 3192 | + + HelpExampleCli("rescanblockchain", "100000 120000") 3193 | + + HelpExampleRpc("rescanblockchain", "100000 120000")
I think the HelpExampleRpc should have a comma between the arguments, i.e. a comma after 100000
Yeah would be good to get this merged, sorry for yet another nit :)
3308 | @@ -3233,6 +3309,7 @@ static const CRPCCommand commands[] = 3309 | { "wallet", "walletpassphrasechange", &walletpassphrasechange, {"oldpassphrase","newpassphrase"} }, 3310 | { "wallet", "walletpassphrase", &walletpassphrase, {"passphrase","timeout"} }, 3311 | { "wallet", "removeprunedfunds", &removeprunedfunds, {"txid"} }, 3312 | + { "wallet", "rescanblockchain", &rescanblockchain, {"start_height", "stop_height"} },
Very tiny nit, but every other place skips the space between words in argument list. I.e. {"start_height","stop_height"}.
re-utACK 35e7fd1147c64a286f778c7740fa735a16d448c5
@jnewbery to be clear
I don't think it's fair to say it was heavily reviewed and I'm needlessly holding it up, I found a major bug in the implementation which required a (imo) pretty significant change to the semantics of the return value.
I'm not suggesting a change in error reporting, I am suggesting a functional change to the ranges which are handled by this call. Specifically, I would like for the cases where
to not throw an error and return a successful scan range (if possible).
@JeremyRubin - sorry if that came off as a personal criticism. That's not what I meant. This PR was re-opened in December last year and has been reviewed by 9 people so far. It's very useful functionality and it'd be great to see it merged. And yes - you did catch a subtle bug in your review which the rest of us missed. Thank you!
re: your suggested change to the interface - if start_height is beyond what's been sync'ed, then there's nothing to rescan and we should return an error to make it clear to the user that the call was a no-op. If stop_height is beyond the sync height, then it's safer to return an error and let the user call the method again with a valid stop_height. If the call succeeded then a user who's not paying close attention to the return value may incorrectly assume that the wallet is rescanned up to the requested stop_height.
3216 | + } 3217 | + 3218 | + // We can't rescan beyond non-pruned blocks, stop and throw an error 3219 | + if (fPruneMode) { 3220 | + CBlockIndex *block = pindexStop ? pindexStop : chainActive.Tip(); 3221 | + while (block && block->nHeight > pindexStart->nHeight) {
Should this be >=? Otherwise it won't check the start block. Maybe add comment if there's a special reason for skipping the start block.
I haven't followed most of the review conversation, but would give light conditional utACK for 35e7fd1147c64a286f778c7740fa735a16d448c5 if >= comment below is addressed.
I like @JeremyRubin's suggestion of avoiding errors when rescans are requested beyond the synced range, but it seems like that could easily be added in a followup.
Fixed @ryanofsky points with the >= check. Lets merge this now,... I think the ranges cleanup (if we want to do this) could be PRed by @JeremyRubin after this PR.
utACK 7a91ceb5e0c9d29dddf7b6ae4cbba802b790924c. Only change since last review was >= fix
🎉