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:
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?
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.
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)
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
)?
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)
importmulti
) it could see use cases for rescanblockchain
.
-rescan
as a startup argument.
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.
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.
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"
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"
startheight
and stopheight
. No other rpcs use -
as word delimiter.
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.");
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)
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
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))
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?
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:
0if (needsRescan || GetBoolArg("-salvagewallet", true) || GetBoolArg("-zapwallettxes", true))
re: zapwallet and salvagewallet. I agree that they should be changed to RPCs for multiwallet.
needsRescan
is false and -salvagewallet
and -
-zapwallettxes` have not been set.
1511@@ -1508,6 +1512,9 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f
1512 } else {
1513 ret = nullptr;
1514 }
1515+ if (pindex == pindexStop) {
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
0if self.rescan == Rescan.manual:
1 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
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
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:
0if (request.params.size() > 0 && !request.params[0].isNull()) {
1193+ }
1194+
1195+ if (pwallet)
1196+ pwallet->ScanForWalletTransactions(pIndexStart, pIndexStop, true);
1197+
1198+ return NullUniValue;
<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.
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())
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
rescanblockchain()
is being called. Am I missing something?
rescanblockchain
, it either says “you” can call rescanblockchain()
.
I don’t understand. The comment is:
0 // Recovery procedure:
1 // move wallet file to wallet.timestamp.bak
2 // Call Salvage with fAggressive=true to
3 // get as much data as possible.
4 // Rewrite salvaged data to fresh wallet file
5 // call rescanblockchain (RPC) so any missing
6 // transactions will be found.
This function does:
but it doesn’t call rescanblockchain, and I can’t see rescanblockchain being called by the callers.
ScanForWalletTransactions()
instead to rescanblockchain (RPC)
. Right?
-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:
0 // Try to recover the wallet:
1 // - move wallet file to wallet.timestamp.bak
2 // - call Salvage with fAggressive=true to get as much data as possible.
3 // - rewrite salvaged data to fresh wallet file
4 //
5 // CWallet::AppInitMain() will later run a full blockchain rescan to find
6 // any missing transactions.
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()) {
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)) {
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"
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"
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
rpc_rescan
branch.
1168+ return NullUniValue;
1169+ }
1170+
1171+ if (request.fHelp || request.params.size() > 2) {
1172+ throw std::runtime_error(
1173+ "rescanblockchain (\"startheight\") (\"stopheight\")\n"
) (
.
null
is equivalent to omitting a parameter.
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
.
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()) {
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()) {
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;
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
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) {
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) {
BLOCK_HAVE_DATA
then the remaining blocks also have, therefore no need to loop?
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+ }
} else { pindex_stop = chainActive.Tip(); }
?
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"
Need to change the progress calculation:
0 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;
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:
0bitcoind -regtest -prune=1
1bitcoin-cli -regtest rescanblockchain <- segfault
-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) {
chainActive.Genesis()
to pindexStart
when declaring the variable (and then overwrite if !request.params[0].isNull()
)
1183+ }
1184+
1185+ LOCK2(cs_main, pwallet->cs_wallet);
1186+
1187+ CBlockIndex *pindexStart = nullptr;
1188+ CBlockIndex *pindexStop = nullptr;
if (!request.params[1].isNull())
code block
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");
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");
RPC_INVALID_PARAMETER
, not RPC_WALLET_ERROR
‘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.
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.");
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) {
EnsureWalletIsAvailable()
above should ensure that the wallet is available.
1225+ stopBlock = pindexStop ? pindexStop : chainActive.Tip();
1226+ }
1227+ }
1228+
1229+ UniValue response(UniValue::VOBJ);
1230+ response.pushKV("startheight", pindexStart->nHeight);
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);
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)
(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())
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"} },
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");
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");
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();
CBlockIndex *block = pindexStop ?: chainActive.Tip();
3216+ if (fPruneMode) {
3217+ CBlockIndex *block = pindexStop ? pindexStop : chainActive.Tip();
3218+ while (block) {
3219+ if (block->nHeight <= pindexStart->nHeight) {
3220+ break;
3221+ }
while (block && block->nHeight > pindexStart->nHeight) {
?
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();
stopBlock = pindexStop ?: chainActive.Tip();
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
).
utACK fe471a9d5a0ac7b10765f99dae0741410f85804d
Did not look at test code yet.
3176+ return NullUniValue;
3177+ }
3178+
3179+ if (request.fHelp || request.params.size() > 2) {
3180+ throw std::runtime_error(
3181+ "rescanblockchain (\"startheight\") (\"stopheight\")\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"
3186+ "\nResult:\n"
3187+ "{\n"
3188+ " \"start_height\" : (number) The block height where the rescan has started\n"
:
.
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"
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"
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.
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.
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)
3221+ }
3222+ block = block->pprev;
3223+ }
3224+ }
3225+
3226+ CBlockIndex *stopBlock = nullptr;
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();
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.
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.
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"
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"
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);
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).
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.
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.");
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\"")
number
-> numeric
in the lines above to be consistent with other calls
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")
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"} },
{"start_height","stop_height"}
.
@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) {
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.