A encrypted wallet can still hold keys which where created when the wallet was unencrypted. Could solve #3314.
This PR will add a 8bit-flags-int to the CKeyMetadata class.
listreceivedbyaddress will report whether the key was generated within a encrypted wallet or if it was imported throught importprivkey.
If this gets conceptual ACKs id like to extend this to the UI level.
Remembering where a key comes from, yeah, why not.
"generationtype" may not be the best name though; we also have "generated" for wallet transactions that shows whether the coins were mined.
What about using keyGenerationType?
key_generation_type then, we don't tend to use lowerCamelCase in RPC
Right. Changed and pushed.
Hm, one thought: Would the resulting wallet still be backwards compatible? We have a versioning/upgrade system for wallets, not sure if it should be used here.
Good point. I'll have to check/test the backward compatibility. I'm not sure what happens to the deserialization when there are one additional byte at the end.
It's backward and forward compatible. Tested.
The 1byte flag added at the end of the CKeyMetadata serialization stream will be ignored when running (old) master-branch. Using a "new" wallet on a "old" bitcoind will still keep the key generation data because Metadata is somehow immutable (will only be created/changes when a key gets generated). Also testes with a full keypool generated with "key generation type".
SGTM, utACK
41 | @@ -42,9 +42,15 @@ enum DBErrors 42 | class CKeyMetadata 43 | { 44 | public: 45 | - static const int CURRENT_VERSION=1; 46 | + static const int CURRENT_VERSION=2; 47 | + static const uint8_t KEY_GENERATION_TYPE_UNKNOWN = 0x0001; 48 | + static const uint8_t KEY_GENERATION_TYPE_IMPORTED = 0x0002; 49 | + static const uint8_t KEY_GENERATION_TYPE_UNENC_WALLET = 0x0004; 50 | + static const uint8_t KEY_GENERATION_TYPE_ENC_WALLET = 0x0008;
Why are these 16-bit hex numbers for an 8-bit type?
Yes. This is wrong. Changed and pushed.
I don't understand the term "generation" in this context.
@luke-jr: "generation" was the best i could find. What would you prefer (maybe something like "Key generation environment" or "key generation security context")?
"origin" perhaps.
Also, this is lacking -upgradewallet logic. Even if old versions can handle it, third-party software might not.
@luke-jr: right, it missed the -upgradewallet approach. I aim for including the key origin/generation type in the new wallet.
Renamed to key origin and added wallet new wallet feature level 70000
1170 | + uint8_t keyFlags = 0; 1171 | + if (address.GetKeyID(keyID)) 1172 | + keyFlags = pwalletMain->mapKeyMetadata[keyID].keyFlags; 1173 | + 1174 | + std::string keyOrigin; 1175 | + if (keyFlags & CKeyMetadata::KEY_ORIGIN_UNKNOWN)
Shouldn't this be used for actually unknown origins (eg, 0 or no-known-bits-set)?
Yes. Agree with you. I will change this to not only holding KEY_ORIGIN within the flags... update is on the way.
42 | @@ -43,8 +43,16 @@ class CKeyMetadata 43 | { 44 | public: 45 | static const int CURRENT_VERSION=1; 46 | + static const int VERSION_SUPPORT_FLAGS=2; 47 | + static const uint8_t KEY_ORIGIN_UNSET = 0x0000; 48 | + static const uint8_t KEY_ORIGIN_UNKNOWN = 0x0001; 49 | + static const uint8_t KEY_ORIGIN_IMPORTED = 0x0002; 50 | + static const uint8_t KEY_ORIGIN_UNENC_WALLET = 0x0004; 51 | + static const uint8_t KEY_ORIGIN_ENC_WALLET = 0x0008;
I don't understand why you're using a bitfield here instead of an enumeration. Do combinations of these ever happen? In rpcwallet.cpp you squash it to one "enum" anyway.
(or is eg IMPORTED ENC used to signify that the key was imported while the wallet was encrypted? If so, the rpc should probably return a list of flag strings instead of one string)
At this point, it is probably the right approach (to use bitfield). It would mean to be extendable because it is a serialized and database stored uint8. Combinations should be possible.
But i agree that the rpcwallet.cpp part should be better. Will take a look at it.
But an enumeration is extensible too, by adding entries. You could say that a bitfield limits extensibility as -at most- 8 flags are possible, whereas an enumeration allows for 256 discrete options in the same space. Unless combinations are meaningful, there is no need to use a bitfield. But I don't care deeply. It just seems weird, especially to have both an ENC and UNENC flag, they conflict)
The main idea was to allow multiple key flags. Like (imported) AND (encrypted OR unencrypted) or potential new key flags like (UI OR RPC together with AND's above).
I just thought when changing the database model, do it right and flexible.
That idea is good, but you're burning flags! You have only 8! At the least get rid of UNKNOWN as a flag, !x already implies that.
Yeah. Agreed. Will remove it.
concept and code review ACK
NACK on the bit flag, concept ACK on the general idea (tracking private key origin). An enumeration sounds fine.
@dcousens: i really like the idea of bit flags. IMO it is flexible, it could indicate different things like if a key was generated deterministic after bip32, etc. What are the downsides of the bit flags compared to a enum/int?
@jonasschnelli I'd go so far to say that this could be user defined, as even a string type might be suitable.
The bit flag idea seems very isolated and only useful for very specific applications.
I don't really have a strong opinion either way on this, did you have a use case in mind?
edit: in terms of #3314, a bit flag would make sense.
I'd disagree with using a string type. It uses more memory and disk space (this is per key!), and is also worst for anything machine-understandable.
A encrypted wallet can still hold keys which where created when the wallet was unencrypted.
This PR will add a 8bit-flags-int to the CKeyMetadata class.
`listreceivedbyaddress` will report whether the key was generated within a enctypted wallet or if it was imported throught `importprivkey`
Rebased and replace the 8bit bitfield with a 32bit bitfield, also removed the "unset" keyflag.
How will this top commit affect wallets with top-minus-one keyFlags? Bitcoin LJR includes the previous semantics, and it would be nice not to break loading such wallets. Maybe the wallet version can be bumped one to handle that case?
@luke-jr: hmm.. yes. This would be required unless we want to break LJR 0.11 wallets. I have plans to update this PR, but it will take some weeks (this PR has low prio). If anyone likes to take this over: go ahead.
Rebased and replace the 8bit bitfield with a 32bit bitfield, also removed the "unset" keyflag.
That's better. Concept ACK.
Concept ACK.
Do we really need a minVersionBump for this change? This PRs minVersionBump to 70000 is kinda senseless.
What about just detecting the change in the deserialization code? I guess every useable deserialization code (assume third party apps) can handle this.
Well, we don't want to add data unless the user upgrades their wallet explicitly (or makes a new one) either.
Well, we don't want to add data unless the user upgrades their wallet explicitly (or makes a new one) either.
But this would make the upgraded wallet no longer work with older versions of bitcoin-core (<0.13 if we would merge this for 0.13) even with the fact that older version are capable of handling the changed/new CKeyMetadata format. This sounds inefficient to me.
Won't old versions currently strip off the key origin data, even if they load the wallet itself okay?
Closing in favor of #8471