Don't default rescan on private/public key imports.
Turn the default rescan for importprivkey, importaddress, importpubkey, importmulti to false to prevent accidental rescanning that is not cancellable.
Manual rescan should be triggered either through -rescan when restarting th Bitcoin Core program or manually by setting one of the parameters to true at the end of batch import.
Accidental rescans should be avoided.
Don't default rescan on private/public key imports.
Turn the default rescan for importprivkey, importaddress, importpubkey,
importmulti to false to prevent accidental rescanning that is not
cancellable.
Manual rescan should be triggered either through -rescan when restarting
th Bitcoin Core program or manually by setting one of the parameters to
true at the end of batch import.
Accidental rescans should be avoided.
Existing software depending on the default value may stop working correctly with this change. Not sure I agree that an accidental rescan is worse than a missed rescan. Maybe the rescan process should be cancelable somehow?
An alternative would be to get rid of the default and require rescan to be set explicitly.
On Tue, Apr 11, 2017 at 11:54 AM, kallewoof notifications@github.com wrote:
Existing software depending on the default value may stop working correctly with this change. Not sure I agree that an accidental rescan is worse than a missed rescan. Maybe the rescan process should be cancelable somehow?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10183#issuecomment-293209255, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv0cIb0MYTiLdiB6Os4QXgXhqfpfKks5ru03XgaJpZM4M52wE .
An accidental scan will lock the wallet thread and the RPC thread until the process is completed, what currently can be a multi-hour long process. A non-cancelable process. The only way it can be canceled is to forcefully interrupt the program which may cause data failure.
I'd rather drop the ability to skip reindex entirely, and require a timestamp for all imports. That way there is no possibility for confusion, and in the common case (it's a recently created key you're importing) it doesn't take particularly long anyway.
I do agree we need to make reindexing abortable.
It is my perspective that an option to skip indexing for never-used newly imported keys should be available.
CWallet.ScanForWalletTransactions should support scanning within specified block height ranges. It should also have a circuit breaker for aborting after a step is finished.
Another functionality I think would be highly useful is having an on-demand RPC rescanwallet command that supports start/stop and block height ranges.
My first focus was to bring up the issue of defaults and making sure they are sane and they don’t force a user to kill the process.
(The build fails because the test requires either default true for importmulti (accidental rescan) or explicitly setting rescan to true in the test.)
This is a long-existing API so there will be software relying on this. The new call importmulti was designed with the better idea that everything should be explicit, but retrofitting this to the old API is a pain.
So I would prefer a way to abort reindexing. That would directly address the problem. Even if the default is changed or removed it's easy enough to make a mistake.
It is my perspective that an option to skip indexing for never-used newly imported keys should be available.
Yes, importmulti with timestamp "now" does this.
Change importmulti rescan from false to true.
I undid the change of the default for importmulti so that the pull will pass the test.