This PR adds optional parameters to the importprivkey, importaddress, and importpubkey RPCs to allow starting the rescan from a certain height instead of the genesis block. It also adds the optional paramter to the -rescan command line option.
This allows rescans to be shortened if the user knows the block in which the first transaction for his addresses was included in.
114@@ -114,6 +115,11 @@ UniValue importprivkey(const UniValue& params, bool fHelp)
115 if (params.size() > 2)
116 fRescan = params[2].get_bool();
117
118+ // Get the start height
if (fRescan) block where it’s used?
228@@ -222,6 +229,11 @@ UniValue importaddress(const UniValue& params, bool fHelp)
229 if (params.size() > 3)
230 fP2SH = params[3].get_bool();
231
232+ // Get the start height
233+ CBlockIndex* start = chainActive.Genesis();
234+ if (params.size() > 4 && fRescan)
235+ start = chainActive[params[4].get_int()];
if (fRescan), cs_main should be locked, right?
I tend to concept NACK. What if you want to import 10 addresses? Self-calculate the oldest heigh and pass it in when you import the last address?
IMO the importmulti #7551 call makes more sense maybe there could also be a reason to support the rescanblockchain #7061. But later should not be required if we do #7551 right.
3139@@ -3140,7 +3140,9 @@ bool CWallet::InitLoadWallet()
3140 RegisterValidationInterface(walletInstance);
3141
3142 CBlockIndex *pindexRescan = chainActive.Tip();
3143- if (GetBoolArg("-rescan", false))
3144+ if (GetBoolArg("-rescan", false) && GetArg("-rescan", 0) > 0)
3145+ pindexRescan = chainActive[GetArg("-rescan", 0)];
3146+ else if (GetBoolArg("-rescan", false))
-rescan is 0, and chainActive.Genesis() == chainActive[0]
What if you want to import 10 addresses? Self-calculate the oldest heigh and pass it in when you import the last address?
Sure, why not? The current behavior would have it rescan from genesis for the last address, and that behavior will still happen here.
As for importmulti, I agree that would also be good. However, I think that the individual import RPCs should also have the same rescanning behavior that importmulti does.
When importing an address, privkey, or pubkey, there is now an option which allows users to set the height from which to start rescanning to save time.
Add the optional parameter to the -rescan command line option to allow rescanning from a block height.
Add tests for the optional parameters to rescan from height for importaddress, importprivkey, and importpubkey.
@achow101: Right. This is a point.
Either we support no rescan for a single input (maybe together with a manual rescan like #7061) or each address/script/pubkey input call should support the same rescan flexibility.
The question now is if we should support heights or timestamps (key birthday). IIRC discussions in #7061 showed that most devs prefer timestamps.
100% agree with @jonasschnelli, importmulti with explicit key birthdays is the answer here, not adding yet another positional argument to RPC calls (yes, you can also use importmulti to import just one key). Let’s focus on that pull and get it merged.
In time we can deprecate import* in favor of the importmulti interface which can (but doesn’t need to) handle multiple keys. There are some other good reasons for that too: see discussion here #7551 (comment) on the importscript confusion.
(and no one generally really wants to import keys one by one and rescan in between)
Agree with @laanwj and @jonasschnelli. For me batch calls are preferred. Still not sure about the name importmulti thou.
But let’s not put aside the concept of rescanblockchain. It makes perfect sense to trigger notifications from a specific block/timestamp.
I agree, but that’s inferior in every way to just having one self-contained operation. @laanwj well this discussion is like
*rawtransactioncalls vscreatetransactionon steroids. In terms of API it is more desirable to have simple, flexible, self explanatory calls instead of one super configurable call.
However I see this like OpenGL glVertex vs glDrawArrays. There are too much locks and alike in import one by one vs impormulti.
Regarding rescan option, I think it is very reasonable to be in importmulti, but I hope it can be possible to just issue a rescan (for instance #7061).
@promag I think this is a bit different.
If we would have had something like importmulti as the only mechanism for importing, I think nobody would even know the concept of a rescan, which is just an artifact of how wallets used to work (without knowing up to where they were synced, and without birth time).
-rescan as startup option (not as RPC call). IMO we could drop the -rescan startup argument in favor or “importmulti” or/and RPC “rescanblockchain” https://github.com/bitcoin/bitcoin/pull/7061/files
We’ve talked about changing -rescan to a dummy before, because people would keep on suggesting it as a magic fix for all sorts of problems for years on the forums, while it had absolutely no reason (only when you manually changed your wallet.dat file it would be needed).
Since the introduction of importprivkey without rescan, it became useful again, since it was the only way to import multiple keys without a rescan after each.
I really think we should stop encouraging these crazy schemes that require people to know there is such a thing as a rescan and rely on having blockchain data available. RPC calls should just work, not require hacks to bring the wallet in a consistent state after the fact, with possible atomicity issues.