fix rescan at importprivkey. #5087

pull panlilu wants to merge 1 commits into bitcoin:master from panlilu:master changing 1 files +2 −3

panlilu commented at 9:25 AM on October 14, 2014: none

Rescan should only be triggered when param Rescan is true.
fix rescan at importprivkey. c0ba21eba5
laanwj commented at 9:26 AM on October 14, 2014: member

Makes sense, utACK
sipa commented at 7:05 PM on October 14, 2014: member

utACK
TheBlueMatt commented at 7:49 PM on October 14, 2014: contributor

NACK, this breaks the "I ran a importprivkey without understanding that I needed a rescan, but restarted bitcoin with -rescan case", I think.
panlilu commented at 6:09 AM on October 15, 2014: none

The param rescan is true by default at function importprivkey, so that's not a problem. @TheBlueMatt
TheBlueMatt commented at 6:15 AM on October 15, 2014: contributor

Thats not entirely my point...its more that, if you're importing a privkey in a new wallet, whether you specify rescan or not, you really shouldnt set the wallet to never be able to rescan to something before the wallet birthday.
panlilu commented at 6:38 AM on October 15, 2014: none

In my pull request I just move the code "pwalletMain->nTimeFirstKey = 1;" to the right place. If you think the code "pwalletMain->nTimeFirstKey = 1;" is not working that's not the problem in this case. Perhaps you can make another commit.

laanwj commented at 6:55 AM on October 15, 2014: member

@panlilu I think he means that you need test that the scenario that he mentions still works. Ie,

# <key> is an old key that was created before the birthday of the wallet, and has transaction in the blockchain before that
bitcoin-cli importkey <key> "" false
# oops, rescan had to be true!
bitcoin-cli stop
bitcoind -rescan
# phew, it rescanned the key now and found the transactions associated with it

panlilu commented at 7:51 AM on October 15, 2014: none

Yes, the case is exist because of the update in 0.9.0 "Optimize rescan to skip blocks prior to birthday", but the param rescan in function importprivkey is from #1085 and before my commit bitcoind will start a rescan automaticly and immediately (Tested) thus the param with false is not working. Setting the param rescan false means I know the privatekey is brandnew.

There is some way to solve the case @TheBlueMatt given. 1.Just import another key and set the param rescan true (Don't need any code change). 2.Improve the -rescan, for example "-rescan=2 means scan the the whole chain". 3.Some better solution...?

Sorry for my misunderstanding at previous reply.
TheBlueMatt commented at 6:03 AM on October 16, 2014: contributor

Sorry, there appears to be a language barrier here...are you saying the scenario @laanwj listed above will currently not work on master?
panlilu commented at 2:30 AM on October 18, 2014: none

@TheBlueMatt Not working.
cozz commented at 2:45 AM on October 18, 2014: contributor
@panlilu Are you sure you didnt forget the label?
```
importprivkey <key> false
```
instead of
```
importprivkey <key> "" false
```
panlilu commented at 2:47 AM on October 18, 2014: none

@cozz Pretty sure.
cozz commented at 3:31 AM on October 18, 2014: contributor

Just tested on current master, I can not reproduce a bug here, the parameter works as expected, and also -rescan.

nTimeFirstKey is memory-only, not written to disk, it does not have any effect on rescan after restart. So I'm fine with merging this pull request as it doesnt hurt anybody, but I cant imagine how this will fix a bug either.
panlilu commented at 3:57 AM on October 18, 2014: none

I start my bitcoind with the -txindex parameter. I'm running a project for vanity address (with lot of address). It works fine before update the bitcoin-core. But it hang and with a 100% cpu when I import a new private key in the new master version bitcoin-core.
laanwj commented at 9:58 AM on October 22, 2014: member
nTimeFirstKey is memory-only, not written to disk, it does not have any effect on rescan after restart.

That's a good point. I take back my utACK.

To have effect beyond a restart you'd also have to change this:
```
pwalletMain->mapKeyMetadata[vchAddress].nCreateTime = 1;
```
pwalletMain->nTimeFirstKey = 1; is just an update so that nTimeFirstKey = min(a.nCreateTime for a in keys) holds. It should not be moved inside.

Maybe a a better solution to have control over future rescans would be to add an optional birthdate argument to importprivkey?
laanwj commented at 2:40 PM on October 29, 2014: member

Closing this for now - let me know if you want to work on a better solution and reopen this, or open a new pull
laanwj closed this on Oct 29, 2014
bitcoin locked this on Sep 8, 2021

Contributors

Linked (view graph)

#1085 importprivkey is very slow