This creates a new serialized structure CKeyMetadata, and stores it alongside each newly created key. This data is used to calculate a whole-wallet birthday.
Wallet rescan is optimized using this information, to skip reading and scanning blocks earlier than the wallet birthday.
I certainly think this is useful data to keep around.
But, as precise time is a rare thing, for the wallet scan optimization to be exact (the "7200 seconds leeway" makes me a bit uneasy), would it maybe make sense to store a block chain height of first use? Now that we have key metadata anyway...
7200 seconds is two hours, which matches the blockchain rules (new blocks must have timestamps no more than 2 hours in the future, if I recall correctly).
And I think keys are properly thought of as independent of any one particular blockchain, so absolute time of creation is the right notion.
Yes, but you are using the local time of the machine that creates the key, at the time the key is generated. I don't think this should be ultimately trusted information (ie, reliable enough to base important optimizations on).
<shrug> The rescan optimization can do "X - 24 hours" and still be quite effective.
One secondary goal along with storing key time is running in client-mode may be optimized to ignore all blocks older than the wallet birthday.
Sounds like a good idea. I prefer keeping a timestamp + lee way, rather than storing blockchain height, as the latter is context that can change during reorganisations.
For deterministic wallet, the meta data object could be extended.
I haven't tested this.
47 | @@ -48,6 +48,10 @@ Value importprivkey(const Array& params, bool fHelp) 48 | 49 | if (!fGood) throw JSONRPCError(-5,"Invalid private key"); 50 | 51 | + int64 nKeyCreateTime = GetTime();
nKeyCreateTime defaults to the current time? Doesn't that break compatibility? (importprivkey currently rescans the entire history).
I'd prefer the current time to be the default (as is implemented here), but I'm afraid it will cause issues.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/fefb9e12b1be676ffff49fccb8be52ae1c65e779 for binaries and test log.
This is a very important feature for multisig. While you can do everything with the raw TX API, just importing the keys and spending from the wallet as usual is much easier, and will remain so for probably quite a while. For me, this patch would have saved a couple of hours worth of rescan time while I was testing multisig.
One possible addition... When doing a rescan, can we detect the time or height of the first appearance of a key, and store that if the birthdate isn't already known? This will be useful when someone later dumps them or has to import a whole wallet.
Oh, and maybe also change WIF to support metadata embedding.
RE: "just importing the keys and spending from the wallet..." : I think we should strongly encourage people to keep private keys private, and not get into the habit of exporting/importing them. If you are sending private keys around for anything other than backup then you're probably Doing It Wrong(tm).
I suspect that for a while to come, the most common usage of multisig will be one time cold storage wallets. You generate some keys, send coins to them, and then later, you redeem whatever is in them. Importing the privkey is the simplest way to do that, and however wrong it may be, I'll bet you a dollar that lots of people are going to do it.
I think we need a standard way of encoding a private key + a txid for such purposes, as that is enough for instant spending without rescanning history at all (however long it is ago).
Bitcoin Wallet and MultiBit can already exchange private keys against each other by using a text based export format. It's basically one base58-encoded key per line, optionally with a key creation time in ISO8601 format. The whole file can be AES encrypted in a way that also openssl can be used to decrypt.
I think it's time that we document this format properly and maybe propose it as a BIP.
I am happy to write up the existing key exchange format Andreas and I are using as a BIP if someone gives me a number. It gives a concrete format to critique and improve upon as required.
Did this get derailed due to not having a serialization for it?
I'm planning to rebase my wallet JSON dump/import RPC patch from 2 years ago, given that there's apparently many people who feel the need to toy with wallets, but shoot themself in the foot by working with individual keys instead of whole wallets. Key birthdates could nice be serialized in that.
@sipa will put it on the todo list. @gmaxwell No real blocker other than it seemed like @gavinandresen grumped over this pull request in general. :)
Yeah, but I grump over everything, it is my Prime Directive.
It looks like a rebase is not trivial, due to some wallet changes in the interim. Not difficult, but not trivial either.
Rebased.
179 | @@ -180,6 +180,7 @@ void CWalletDB::ListAccountCreditDebit(const string& strAccount, list<CAccountin 180 | return DB_LOAD_OK; 181 | } 182 | 183 | +static unsigned int nKeys = 0, nCKeys = 0, nKeyMeta = 0;
This is very ugly...
Apart from those static counter variables, ACK on the code changes. A more efficient wallet serialization format (where metadata is stored together with the keys) would be nice, but I guess that can easily be done together with a wallet format overhaul.
Also note that this pull request introduces an extensible key-metadata object in the wallet, fully versioned. It is now easier to add other metadata to each key.
Merged with upstream
ACK code changes; didn't test yet. Can you rebase instead of doing merges?
ACK on code changes
I'd like to merge this, can you squash the merge commit?
This also encapsulate wallet-read state information into CWalletScanState.
Rebased