add key generation type #5916

Closed
wants to merge 3 commits into
from

Projects

None yet

7 participants

@jonasschnelli
Member

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.

@laanwj laanwj added the Wallet label Mar 18, 2015
@laanwj
Member
laanwj commented Mar 18, 2015

Remembering where a key comes from, yeah, why not.

@laanwj
Member
laanwj commented Mar 18, 2015

"generationtype" may not be the best name though; we also have "generated" for wallet transactions that shows whether the coins were mined.

@jonasschnelli
Member

What about using keyGenerationType?

@laanwj
Member
laanwj commented Mar 18, 2015

key_generation_type then, we don't tend to use lowerCamelCase in RPC

@jonasschnelli
Member

Right.
Changed and pushed.

@laanwj
Member
laanwj commented Mar 20, 2015

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.

@jonasschnelli
Member

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.

@jonasschnelli
Member

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".

bildschirmfoto-2015-03-20-um-10 50 30

@laanwj
Member
laanwj commented Mar 24, 2015

SGTM, utACK

@luke-jr luke-jr and 1 other commented on an outdated diff Jun 2, 2015
src/wallet/walletdb.h
@@ -42,9 +42,15 @@ enum DBErrors
class CKeyMetadata
{
public:
- static const int CURRENT_VERSION=1;
+ static const int CURRENT_VERSION=2;
+ static const uint8_t KEY_GENERATION_TYPE_UNKNOWN = 0x0001;
+ static const uint8_t KEY_GENERATION_TYPE_IMPORTED = 0x0002;
+ static const uint8_t KEY_GENERATION_TYPE_UNENC_WALLET = 0x0004;
+ static const uint8_t KEY_GENERATION_TYPE_ENC_WALLET = 0x0008;
@luke-jr
luke-jr Jun 2, 2015 Member

Why are these 16-bit hex numbers for an 8-bit type?

@jonasschnelli
jonasschnelli Jun 2, 2015 Member

Yes. This is wrong. Changed and pushed.

@luke-jr
Member
luke-jr commented Jun 2, 2015

I don't understand the term "generation" in this context.

@jonasschnelli
Member

@luke-jr: "generation" was the best i could find. What would you prefer (maybe something like "Key generation environment" or "key generation security context")?

@luke-jr
Member
luke-jr commented Jun 24, 2015

"origin" perhaps.

Also, this is lacking -upgradewallet logic. Even if old versions can handle it, third-party software might not.

@jonasschnelli
Member

@luke-jr: right, it missed the -upgradewallet approach. I aim for including the key origin/generation type in the new wallet.

@jonasschnelli
Member

Renamed to key origin and added wallet new wallet feature level 70000

@luke-jr luke-jr commented on the diff Jul 11, 2015
src/wallet/rpcwallet.cpp
@@ -1165,6 +1165,22 @@ UniValue ListReceived(const UniValue& params, bool fByAccounts)
fIsWatchonly = (*it).second.fIsWatchonly;
}
+ // convert keyflags into a string
+ CKeyID keyID;
+ uint8_t keyFlags = 0;
+ if (address.GetKeyID(keyID))
+ keyFlags = pwalletMain->mapKeyMetadata[keyID].keyFlags;
+
+ std::string keyOrigin;
+ if (keyFlags & CKeyMetadata::KEY_ORIGIN_UNKNOWN)
@luke-jr
luke-jr Jul 11, 2015 Member

Shouldn't this be used for actually unknown origins (eg, 0 or no-known-bits-set)?

@jonasschnelli
jonasschnelli Jul 11, 2015 Member

Yes. Agree with you. I will change this to not only holding KEY_ORIGIN within the flags... update is on the way.

@laanwj laanwj and 1 other commented on an outdated diff Jul 21, 2015
src/wallet/walletdb.h
@@ -43,8 +43,16 @@ class CKeyMetadata
{
public:
static const int CURRENT_VERSION=1;
+ static const int VERSION_SUPPORT_FLAGS=2;
+ static const uint8_t KEY_ORIGIN_UNSET = 0x0000;
+ static const uint8_t KEY_ORIGIN_UNKNOWN = 0x0001;
+ static const uint8_t KEY_ORIGIN_IMPORTED = 0x0002;
+ static const uint8_t KEY_ORIGIN_UNENC_WALLET = 0x0004;
+ static const uint8_t KEY_ORIGIN_ENC_WALLET = 0x0008;
@laanwj
laanwj Jul 21, 2015 Member

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)

@jonasschnelli
jonasschnelli Jul 21, 2015 Member

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.

@laanwj
laanwj Jul 21, 2015 Member

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)

@jonasschnelli
jonasschnelli Jul 21, 2015 Member

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.

@laanwj
laanwj Jul 21, 2015 Member

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.

@jonasschnelli
jonasschnelli Jul 21, 2015 Member

Yeah. Agreed. Will remove it.

@jgarzik
Member
jgarzik commented Sep 15, 2015

concept and code review ACK

@dcousens
Contributor

NACK on the bit flag, concept ACK on the general idea (tracking private key origin).
An enumeration sounds fine.

@jonasschnelli
Member

@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?

@dcousens
Contributor

@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.

@laanwj
Member
laanwj commented Sep 24, 2015

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.

jonasschnelli added some commits Mar 17, 2015
@jonasschnelli @jonasschnelli jonasschnelli add key generation type
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`
1d6d3da
@jonasschnelli @jonasschnelli jonasschnelli renamed keyGenerationType to keyOrigin, added new wallet feature level f1fef04
@jonasschnelli jonasschnelli Use a 32bit bitmask for key generation type, remove unset type d4142be
@jonasschnelli
Member

Rebased and replace the 8bit bitfield with a 32bit bitfield, also removed the "unset" keyflag.

@luke-jr
Member
luke-jr commented Oct 23, 2015

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?

@jonasschnelli
Member

@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.

@pstratem

This doesn't look like it would handle multiple flags being set very well.

Member

Yea, if a non-sensible combination of bits is set, it should probably report it as 'unknown'.

Edit: or better, simply return a list of strings here.

@laanwj
Member
laanwj commented Feb 10, 2016

Rebased and replace the 8bit bitfield with a 32bit bitfield, also removed the "unset" keyflag.

That's better.
Concept ACK.

@btcdrak
Member
btcdrak commented Apr 5, 2016

Concept ACK.

@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2016
@luke-jr luke-jr Merge #5916 jonas_keygentype-0.11.x 7115804
@jonasschnelli
Member

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.

@luke-jr
Member
luke-jr commented Jul 20, 2016

Well, we don't want to add data unless the user upgrades their wallet explicitly (or makes a new one) either.

@jonasschnelli
Member

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.

@luke-jr
Member
luke-jr commented Jul 20, 2016

Won't old versions currently strip off the key origin data, even if they load the wallet itself okay?

@jonasschnelli
Member

Closing in favor of #8471

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment