[Wallet] Add RPC call "rescanblockchain <startheight> <stopheight>"

[jonasschnelli]

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

[laanwj]

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?

[jonasschnelli]

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.

[laanwj]

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.

[gmaxwell]

@lannwj I thought thats part of what the height parameter here was for-- addressing pruning comparability?

[petertodd]

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?

[petertodd]

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.

[gmaxwell]

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.

[petertodd]

@gmaxwell A possible way around that would be to make the rescan check if you have any addresses that haven't yet been scanned in that range and error out if not. (basically make it say "no rescan needed")

[gmaxwell]

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.

[promag]

Remove?

[jonasschnelli]

Oops. A rebase issue. Thanks for point out. Fixed.

[pstratem]

agree with gmaxwell that this should scan a range of blocks

[jonasschnelli]

Agree with the stop parameter. Working on a implementation....

[sipa]

I would still prefer an approach that imports with birthdate instead of explicit rescanning.

[jonasschnelli]

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)?

[promag]

@jonasschnelli see #6570 (comment) regarding your last comment.

[GIJensen]

ACK

[mrbandrews]

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.

[promag]

Move to while condition?

[promag]

Because it's inclusive?

[jonasschnelli]

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.

[sipa]

This seems better #7984, but I still prefer not furthering the usage of various rescans. We need APIs that don't require users to keep track of the concept of rescanning IMHO.

[jonasschnelli]

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.

[sipa]

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.

[jonasschnelli]

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.

[jonasschnelli]

Another thing where this could be useful is restoring hd wallets

[MarcoFalke]

Since importmulti #7551 is merged, some use cases are covered by that.

[laanwj]

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.

[jonasschnelli]

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).

[jonasschnelli]

Reopened and overhauled.
This now does replace the -rescan startup argument with a new RPC call rescanblockchain. The reasons for that are:

• You can define the start and end-height
• It can be called during runtime
• It can work in multiwallet environment

Using -rescan will cancel the startup with an error referring to the new RPC call.

[ryanofsky]

Should add some test coverage for this. One easy way might be to add a new Rescan.manual enum value here:

bitcoin/test/functional/import-rescan.py

Line 32 in 4677151

Rescan = enum.Enum("Rescan", "no yes late_timestamp")

, then put

if self.rescan == Rescan.manual:
    self.node.rescanblockchain()

at the end of the do_import method.

[ryanofsky]

I think these files are automatically generated from -help output and could be reverted in the PR.

[jonasschnelli]

Fixed.

[ryanofsky]

Maybe still worth mentioning that pruning can get in the way of successfully importing wallet keys & rescanning.

[ryanofsky]

Maybe this should take either times or heights like the pruneblockchain RPC:

bitcoin/src/rpc/blockchain.cpp

Line 838 in 4677151

"1. \"height\" (numeric, required) The block height to prune up to. May be set to a discrete height, or a unix timestamp\n"

[jnewbery]

suggest you change the argument names to startheight and stopheight. No other rpcs use - as word delimiter.

[jonasschnelli]

Indeed. Fixed.

[ryanofsky]

Style might need to be updated for this code (moving opening brace to same line here, adding missing braces other lines)

[jonasschnelli]

Fixed.

[ryanofsky]

Might be helpful if error message could mention getblockchaininfo RPC for getting pruned height.

[jonasschnelli]

Fixed.

[ryanofsky]

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.

[jonasschnelli]

See the discussion about the stop argument: #7061 (comment)

[ryanofsky]

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?

[jonasschnelli]

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.

[ryanofsky]

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?

[jonasschnelli]

IMO Yes. It's pure db utility. Either we move it to RPC or to a new wallet/db tool.

[jnewbery]

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.

[jonasschnelli]

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.

[jnewbery]

Yes, you're right.

[jnewbery]

I can't see where rescanblockchain() is being called. Am I missing something?

[jonasschnelli]

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().

[jnewbery]

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:

• 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

but it doesn't call rescanblockchain, and I can't see rescanblockchain being called by the callers.

[jonasschnelli]

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?

[jnewbery]

sorry, I'm still not seeing it. You say "the part that calls CDB::Recover does always call a rescan". Where?

[jonasschnelli]

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.

[jnewbery]

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.

[jnewbery]

suggest you change the argument names to startheight and stopheight. No other rpcs use - as word delimiter.

[jnewbery]

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()) {

[jnewbery]

should this return an error if heightstop is lower than heightstart? If heightstop is higher than the tip height?

[jnewbery]

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.

[ryanofsky]

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).

[jnewbery]

nit: update function comment to say that the rescan is up to pindexStop if it is non-null

[jonasschnelli]

Fixed.

[jnewbery]

Need to remove more of this comment, so that it just reads:

// show rescan progress in GUI as dialog

[jonasschnelli]

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?

[jnewbery]

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.

[TheBlueMatt]

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.

[jonasschnelli]

Rebased.

[ryanofsky]

utACK 7858b9f, 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.

[ryanofsky]

Capitalization of height doesn't match actual param name.

[ryanofsky]

Could do what pruneblockchain does and allow height to be specified either as a physical height or a time:

bitcoin/src/rpc/blockchain.cpp

Line 855 in 1caafa6

"1. \"height\" (numeric, required) The block height to prune up to. May be set to a discrete height, or a unix timestamp\n"

[ryanofsky]

Size checks can be dropped here and below. params[0] will return a null value if the param is missing.

[ryanofsky]

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).

[ryanofsky]

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).

[promag]

If #10941 goes first then the test can be extended to do a rescanblockchain and assert the notified blocks.

[luke-jr]

Rebased and addressed @ryanofsky 's comments on my rpc_rescan branch.

[jonasschnelli]

Cherry picked @luke-jr rebased / overhauled version.
Still needs an RPC test.

[promag]

Need to change the progress calculation:

         double dProgressTip = GuessVerificationProgress(chainParams.TxData(), chainActive.Tip());

Should rename to dProgressStop.

[promag]

Remove inner ) (.

[luke-jr]

No... they're both independently optional.

[promag]

But without named args that's not possible.

[luke-jr]

Yes it is...

[promag]

How?

[luke-jr]

JSON null is equivalent to omitting a parameter.

[promag]

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.

[luke-jr]

Not sure what you're getting at. Null is omitted.

[promag]

Just start?

Also, if positive then height, if negative then depth relative to tip. Same for stop.

[jnewbery]

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.

[promag]

Note that it's inclusive?

[promag]

pindex_start?

[promag]

IMO if (!request.params[0].isNull()) so that get_int throws if invalid value is supplied.

[promag]

Same as above.

[promag]

Maybe we should } else { pindex_stop = chainActive.Tip(); }?

[jonasschnelli]

The stop height can just be a nullptr (== scan up to the tip).

[promag]

Should be an error instead?

[luke-jr]

Why?

[promag]

Why would start be greater than stop? Seems to be a bad call?

[luke-jr]

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.

[promag]

I see your point 👍.

[promag]

Use std::swap?

[promag]

Correct me if I'm wrong, but if start has BLOCK_HAVE_DATA then the remaining blocks also have, therefore no need to loop?

[luke-jr]

I think this may be currently true, but perhaps not safe to assume for the future. Unsure.

[promag]

Maybe factor out to a CChain function so in the future it's more contained and for now remove the loop?

[jonasschnelli]

We loop backwards here which the check is necessary. Or am I wrong?

[promag]

Something is wrong now because nothing changes the the loop exit conditions.

[promag]

Should not happen, assert instead?

[jonasschnelli]

Can happen from the perspective of the function. But maybe not with the current consumption of the function.

[promag]

Could return the scanned range? Then the client can keep the returned stopheight to be the next startheight.

[promag]

block->pprev can be nullptr:

bitcoind -regtest -prune=1
bitcoin-cli -regtest rescanblockchain   <- segfault

[jonasschnelli]

Rebased and overhauled.
This no longer evicts the startup -rescan option.
Also added a RPC test.

[MeshCollider]

utACK bf6f253

[jnewbery]

Looks generally good. A few nits inline.

[jnewbery]

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.

[jnewbery]

nit: slight preference to declare this variable immediately above the if (!request.params[1].isNull()) code block

[jonasschnelli]

IMO they should be declared / initialised together (for better code readability).

[jnewbery]

This should be RPC_INVALID_PARAMETER, not RPC_WALLET_ERROR

[jnewbery]

Why not just assign chainActive.Genesis() to pindexStart when declaring the variable (and then overwrite if !request.params[0].isNull())

[jonasschnelli]

Makes sense. Will update.

[jnewbery]

As above, this should be RPC_INVALID_PARAMETER, not RPC_WALLET_ERROR

[jnewbery]

This should be RPC_MISC_ERROR. Trying to scan a pruned block is not an error in the wallet.

[jnewbery]

why is this required? The call to EnsureWalletIsAvailable() above should ensure that the wallet is available.

[jonasschnelli]

Right... Will remove it. I think this is a rebase issue since the PR is originally from 2015.

[jnewbery]

These return fields should be documented in the help text.

[jnewbery]

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.

[jnewbery]

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.

[jnewbery]

nit: add a space: (0, 1)

[jnewbery]

Perhaps assert that the rescan scanned up to the tip:

assert_equal(out['stopheight'], self.nodes[1].getblockcount())

[luke-jr]

If the heights are inverted, they should be flipped. 100..90 is no less rational than 90..100. This was in 79a1173, but seems to have disappeared silently in a rebase?

[jnewbery]

@luke-jr - do you have a particular use-case for inverting the block heights? If not, I think this is fine as is. We should err towards being more strict in the interface to keep the code simpler.

[luke-jr]

Already discussed here.

There's no reason to have an arbitrary restriction that doesn't make sense and is undocumented, especially when we can trivially just do the right thing.

[jnewbery]

'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.

[luke-jr]

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.

[luke-jr]

I think this was more appropriately positioned after "removeprunedfunds" as before. Probably after "move" makes the most sense, since it seems to be alphabetised.

[luke-jr]

Also not sure rpcdump.cpp makes sense for this one.

[jonasschnelli]

Addresses @jnewbery's points.
Agree with @luke-jr that it should be in rpcwallet and not in rpcdump (moved it there).

[kallewoof]

utACK fe471a9

Did not look at test code yet.

[kallewoof]

μNit: start_height since you're calling it that in the help.

[kallewoof]

μNit: stop_height

[kallewoof]

Can be abbreviated to CBlockIndex *block = pindexStop ?: chainActive.Tip();

[jonasschnelli]

Fixed.

[kallewoof]

Why not just while (block && block->nHeight > pindexStart->nHeight) {?

[jonasschnelli]

Fixed.

[kallewoof]

Same as above here; stopBlock = pindexStop ?: chainActive.Tip();

[jonasschnelli]

Fixed.

[kallewoof]

CBlockIndex* pindexStop = nullptr will let existing code default to current behavior (mostly in the test cases in wallet/test/wallet_tests.cpp).

[jonasschnelli]

Heh. I had it before but then @jnewbery convinced me to remove it: #7061 (comment)

[jonasschnelli]

Adressed @kallewoof's points.

[kallewoof]

utACK 6a51097

[kallewoof]

I missed this last time: start_height and stop_height here to match the rest.

[jonasschnelli]

Fixed.

[kallewoof]

re-utACK e2d376b

[promag]

utACK e2d376b.

Will test tomorrow. While there's few ACK's why not update variables to snake case?

[promag]

Nit, remove space before :.

[jnewbery]

2 nits and a minor bug

[jnewbery]

nit: add "If omitted, rescan starts from the genesis block."

[MeshCollider]

Or just default=0

[jnewbery]

nit: add "If omitted, rescan stops at the chain tip."

[JeremyRubin]

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.

[jnewbery]

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.

[jonasschnelli]

Nice catch... added a protection that detects aborted rescans via a check of CWallet::IsAbortingRescan() after ScanForWalletTransactions().

[jnewbery]

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.

[jonasschnelli]

Fixed @jnewbery and @promag's nits.

[jnewbery]

nit: please sort imports :)

and ideally place them in PEP8 ordering (ie standard library before project imports)

[jnewbery]

Looks good. One style nit, but ACK bdae58e with or without.

[promag]

Join this with next line.

[promag]

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.

[promag]

Maybe we should add the most recent block that could not be scanned to the response.

[jonasschnelli]

It's not correct, right. I think we leave that fix for another PR (not directly related to this PR).

[promag]

Avoid this GNU extension (empty operand)?

[promag]

BTW, first time in this source code.

[kallewoof]

I did not know a ?: b was a GNU extension. Sorry for bad nit earlier. :(

[jonasschnelli]

Fixed.

[JeremyRubin]

Concept Ack.

However, the handling of the ranges is incorrect afaict and needs to be corrected (and better documented).

[JeremyRubin]

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.

[JeremyRubin]

tense error s/start/started

[JeremyRubin]

tense error s/stops/stopped

[JeremyRubin]

This is not accurate AFAICT. The block at stop_height is not scanned?

[jonasschnelli]

AFAIK the block defined with stop_height will be scanned.

[JeremyRubin]

I think with this (and with the above pindexStart check), I'd like to see these errors differentiate between:

1. Invalid Start Height: no negative heights
2. Invalid Start Height: Beyond what's been synced
3. Invalid Stop Height: No negative heights
4. Invalid Stop Height: Beyond what's been synced
5. Invalid Range: stop must be greater than start.

At the very least, 5. should be represented because it could be the start_height which is invalid.

[jonasschnelli]

Added case 5.

[JeremyRubin]

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.

[JeremyRubin]

If no blocks are re scanned, I don't think this is correct/is confusing.

[JeremyRubin]

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.

[jonasschnelli]

@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.

[jonasschnelli]

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.

[JeremyRubin]

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.

[JeremyRubin]

I think this is clearest to say:

If pindexStop is not a nullptr, we attempt to scan up to and including pIndexStop.

[JeremyRubin]

Also, more generally, it seems we could fail to scan many blocks in the range and still return that we scanned the range.

[jonasschnelli]

@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.

[jonasschnelli]

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)

[jtimon]

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.

[jonasschnelli]

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.)

[promag]

IMO this is ready to merge even though there are some concerns that need to be addressed in follow ups:

• Rescan continues even if a corrupted block is detected but the RPC fails to the caller;
• 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.

[MeshCollider]

Typo corruputed -> corrupted

[MeshCollider]

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

[MeshCollider]

re-utACK 559542a modulo comments above

[jonasschnelli]

Fixed @MeshCollider nits.

[JeremyRubin]

I still think it's worth it to handle

1. Invalid Start Height: no negative heights
2. Invalid Stop Height: No negative heights

and

1. Invalid Start Height: Beyond what's been synced
2. Invalid Stop Height: Beyond what's been synced

differently. Specifically, the latter calls could still be handled and processed.

[jnewbery]

Tested ACK 35e7fd1

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.

[MeshCollider]

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 :)

[kallewoof]

Very tiny nit, but every other place skips the space between words in argument list. I.e. {"start_height","stop_height"}.

[kallewoof]

re-utACK 35e7fd1

[JeremyRubin]

@jnewbery to be clear

1. 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.

2. 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

   1. Invalid Start Height: Beyond what's been synced
   2. Invalid Stop Height: Beyond what's been synced

to not throw an error and return a successful scan range (if possible).

[jnewbery]

@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.

[ryanofsky]

I haven't followed most of the review conversation, but would give light conditional utACK for 35e7fd1 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.

[ryanofsky]

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.

[jonasschnelli]

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.

[ryanofsky]

utACK 7a91ceb. Only change since last review was >= fix

[jnewbery]

reACK 7a91ceb

[jnewbery]

🎉