Fix importprivkey / rescan #3502

1. 
   cozz commented at 7:39 pm on January 9, 2014: contributor

   importprivkey / rescan does not always work currently.

   Use case:

   • dumpprivkey with a balance
   • close bitcoin
   • delete wallet.dat
   • start bitcoin
   • importprivkey

   Result: rescan fails because wallet uses nTimeFirstKey of the other keys. Setting pwalletMain->nTimeFirstKey = 1; solves the problem in this case, scanning the chain from genesis. But I if import with rescan=false, then close bitcoin and do bitcoin-qt -rescan this fails again. Because nCreateTime of the metadata is zero, and now nTimeFirstKey is set to zero, but overwritten later again, because if(0) is false. So setting this to 1 solves the problem.

   Further ideas, not part of this pull: dumprivkey could export nCreationTime as well. importprivkey would then have distinguish between old and new format. rescan from the genesis block could reassign all nCreationTime.

2. 
   luke-jr commented at 7:45 pm on January 9, 2014: member

   Keys and addresses don’t have balances. You’re not supposed to use import/dump privkey like this. Just backupwallet for backups.
3. 
   sipa commented at 8:08 pm on January 9, 2014: member

   Though I agree with Luke’s opinion here, it’s not an excuse for broken functionality. If you’re using dumpprivkey/importprivkey, you’re already managing your wallet at the per key level, and as we have rpc methods for that, they should work.
4. 
   laanwj commented at 7:49 am on January 10, 2014: member

   Agree with @luke-jr here. What you are doing is very dangerous. You should never assume that the balance is only on one key. Many people lost their coins this way.

   This is exactly what dumpwallet/importwallet is for which is in master (and will be in 0.9). These methods export and import all the keys in your wallet.

   Code changes look ok to me though…

5. 
   gmaxwell commented at 7:55 am on January 10, 2014: contributor

   OH, this probably explains some misbehavior I saw with salvagewallet missing outputs! See, look at how much easier slacking off and not investigating the cause was…
6. 
   laanwj commented at 12:20 pm on January 17, 2014: member

   Can we get some ACKs/NACKs here? This is a very simple patch, and if it solves a problem (and in the correct way) it should be in 0.9 IMO.
7. 
   sipa commented at 3:53 pm on January 18, 2014: member

   This is not the correct way to solve the problem, IMHO.

   We should set pwallet->mapKeyMetadata[keyid].nCreationTime to 1 in importprivkey, before calling AddKeyPubKey, instead of initializing every key’s birthdate to 1.

8. 
   sipa commented at 3:56 pm on January 18, 2014: member

   I do agree that import/dump need a way to deal with birthdates, though. I suggest adding a bool to dumpprivkey [includebirth=false], which if set, outputs a string privkey@unixtimestamp. Importprivkey can then support keys in that format too, and set the imported key’s timestamp to the provided one, instead of 1.
9. Fix importprivkey / rescan 1f12844fc0
10. 
    cozz commented at 7:15 pm on January 18, 2014: contributor

    I have now updated to what @sipa suggested. Tested both, rescanning through importprivkey and through -rescan.

    About the import/dump of birthdate: I dont like so much bothering the user with the birthdates, I think this should be transparent to the user. So how about introducing a version parameter to dumpprivkey [version=2]. So default is version 2, dump with @unixtimestamp. Now if someone needs to export from 0.9 and import in 0.8, he can call dumpprivkey version=1, exporting without @unixtimestamp. Good/bad idea?

    Also maybe adding a little checksum at least to the timestamp would be good, I think, as importing a private key should be guaranteed to work. If checksum wrong, set timestamp to 1.

11. 
    sipa commented at 7:20 pm on January 18, 2014: member

    @cozz I consider this function micro-management. We provide it, so it must work. But in general, I’d very much prefer to encourage entire wallet dumping/importing (dumpwallet/importwallet) which already handles birthdates. I wouldn’t bother adding a checksum there too.

    Regarding a version number rather than a boolean… meh, I don’t think we’ll add many more versions later for the same typo of keys.

12. 
    sipa commented at 7:21 pm on January 18, 2014: member

    Haven’t tested, but ACK code changes.
13. 
    BitcoinPullTester commented at 7:33 pm on January 18, 2014: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1f12844fc03dfc1863ea9a5096f8650c64a0b2ac for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
14. 
    laanwj commented at 7:37 pm on January 18, 2014: member

    ACK (much easier to review with this localized change)
15. laanwj referenced this in commit 6586bc3b51 on Jan 22, 2014
16. laanwj merged this on Jan 22, 2014
17. laanwj closed this on Jan 22, 2014

    ---

18. DrahtBot locked this on Sep 8, 2021

Contributors
cozz luke-jr sipa laanwj gmaxwell BitcoinPullTester