Skip to content

Fix a couple of issues with -rescan#3796

Closed
UdjinM6 wants to merge 6 commits intodashpay:developfrom
UdjinM6:fix_rescan
Closed

Fix a couple of issues with -rescan#3796
UdjinM6 wants to merge 6 commits intodashpay:developfrom
UdjinM6:fix_rescan

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Nov 6, 2020

When a wallet is created we set nTimeFirstKey = GetTime() which allows -rescan to save time by fast-forwarding to a block around this time and ignoring all the older blocks. This works fine for non-HD wallets with random keys but HD wallets are special - because of their deterministic nature they might have keys which were used in txes before the wallet file was even created. This PR fixes it by updating key metadata when a key which appears to be used earlier than we expected is found db8b129 and by letting users to rescan from genesis block via -rescan=2 fa06123. For qt users there is a new corresponding repair option on Repair tab c93b18d. I also tweaked CreateWalletFromFile logs and getwalletinfo rpc to make it easier to track nTimeFirstKey changes 2d2fb11.

Screenshot 2020-11-10 at 01 05 52

@UdjinM6 UdjinM6 added this to the 17 milestone Nov 6, 2020
@PastaPastaPasta
Copy link
Member

Can you add a screenshot for c93b18d?

@PastaPastaPasta PastaPastaPasta added the RPC Some notable changes to RPC params/behaviour/descriptions label Nov 9, 2020
@UdjinM6
Copy link
Author

UdjinM6 commented Nov 9, 2020

@PastaPastaPasta sure, done.

EDIT: this PR does not change any rpc, it's gui+cmd-line only (from user perspective). Dropped RPC label.

@UdjinM6 UdjinM6 removed the RPC Some notable changes to RPC params/behaviour/descriptions label Nov 9, 2020
@PastaPastaPasta
Copy link
Member

You wouldn't call fa06123 RPC related? I guess it technically isn't...

@UdjinM6
Copy link
Author

UdjinM6 commented Nov 9, 2020

Hmmm... It's true that loadwallet behaviour is going be different for nodes started with -rescan=2 but that's an expected/desired behaviour which is controlled by cmd-line params, not by rpc (options). Imo no RPC label is needed here but.. let's ping @thephez :)

xdustinface
xdustinface previously approved these changes Nov 11, 2020
Copy link

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly tested ACK

Comment on lines +1275 to +1276
bool res = GetPubKey(keyid, vchPubKey);
assert(res); // this should never fail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't use res really, would it be better to make this one line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to call GetPubKey to get vchPubKey even when asserts are disabled. Technically, it shouldn't be possible to compile it that way (with -DNDEBUG) because of https://github.com/dashpay/dash/blob/master/src/validation.cpp#L61 but imo it's better to avoid introducing potential issues anyway.

while (pindexRescan && walletInstance->nTimeFirstKey && (pindexRescan->GetBlockTime() < (walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW))) {
pindexRescan = chainActive.Next(pindexRescan);
// unless a full rescan was requested
if (gArgs.GetArg("-rescan", 0) != 2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say someone puts in -rescan=3 should this still be true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about 4a0adc2?

@thephez
Copy link
Collaborator

thephez commented Nov 12, 2020

Hmmm... It's true that loadwallet behaviour is going be different for nodes started with -rescan=2 but that's an expected/desired behaviour which is controlled by cmd-line params, not by rpc (options). Imo no RPC label is needed here but.. let's ping @thephez :)

It appears that getwalletinfo was updated also to add timefirstkey, so the RPC label would still be relevant.

@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Nov 12, 2020
@UdjinM6
Copy link
Author

UdjinM6 commented Nov 12, 2020

It appears that getwalletinfo was updated

oh, right 🙈 added RPC label back

Copy link

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about 4a0adc2?

Looks good to me, utACK

@UdjinM6 UdjinM6 closed this Nov 26, 2020
@UdjinM6 UdjinM6 deleted the fix_rescan branch November 26, 2020 13:26
@UdjinM6 UdjinM6 restored the fix_rescan branch November 26, 2020 13:27
@UdjinM6 UdjinM6 deleted the fix_rescan branch November 26, 2020 13:27
@UdjinM6 UdjinM6 restored the fix_rescan branch November 26, 2020 13:27
@UdjinM6 UdjinM6 deleted the fix_rescan branch November 26, 2020 13:28
@UdjinM6
Copy link
Author

UdjinM6 commented Nov 26, 2020

Ooops, deleted the wrong one 🙈 pls see #3828

@UdjinM6 UdjinM6 removed this from the 17 milestone Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants