Fix hdmaster-key / seed-key confusion (scripted diff)

jnewbery commented at 3:16 pm on April 9, 2018: member

Addresses #12084 and #8684

This renames a couple of functions and members (no functional changes, expect log prints):

Rename CKey::SetMaster to CKey::SetSeed
Rename CHDChain::masterKeyId to CHDChain::seedID
Rename CHDChain::hdMasterKeyID to CHDChain::hdSeedID
Rename CWallet::GenerateNewHDMasterKey to CWallet::GenerateNewHDSeed
Rename CWallet::SetHDMasterKey to CWallet::SetHDSeed

As well it introduces a tiny API change:

RPC API change: Rename “hdmasterkeyid” to “hdseedid”, rename “hdmaster” in wallet-dump output to “hdseed”

Fixes also a bug:

Bugfix: use “s” instead of the incorrect “m” for the seed-key hd-keypath key metadata

jnewbery commented at 3:16 pm on April 9, 2018: member

This is #12094 reimplemented as a scripted diff.

jonasschnelli commented at 3:21 pm on April 9, 2018: contributor

utACK d41c76281f4abe06927560789f4a9623e1414f4c

MarcoFalke added the label Needs release notes on Apr 9, 2018

MarcoFalke added the label Wallet on Apr 9, 2018

practicalswift commented at 8:55 pm on April 9, 2018: contributor

Concept ACK

laanwj commented at 4:06 pm on April 10, 2018: member

Concept ACK

jonasschnelli commented at 9:04 am on April 17, 2018: contributor

Needs rebase

jnewbery force-pushed on Apr 17, 2018

jnewbery commented at 4:12 pm on April 17, 2018: member

Rebased and added release notes.

MarcoFalke removed the label Needs release notes on Apr 17, 2018

MarcoFalke commented at 4:31 pm on April 17, 2018: member

Would it make sense to additionally push back the “hdmasterkeyid” key-value pair for one release? This might make it easier to quickly downgrade after a failed upgrade to the latest release.

jnewbery commented at 6:27 pm on April 17, 2018: member

@MarcoFalke - sorry I don’t fully understand. Can you point out in the code where you mean?

in src/wallet/rpcwallet.cpp:4017 in b5e55fbf28 outdated

3840@@ -3841,7 +3841,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3841         ret.pushKV("timestamp", meta->nCreateTime);
3842         if (!meta->hdKeypath.empty()) {
3843             ret.pushKV("hdkeypath", meta->hdKeypath);
3844-            ret.pushKV("hdmasterkeyid", meta->hdMasterKeyID.GetHex());

MarcoFalke commented at 8:02 pm on April 17, 2018:

E.g. here it seems that you are removing hdmasterkeyid from the dict. To make it easier for people that rely on this key to upgrade/downgrade without having to change their code (or configuration) every time they switch, it could make sense to leave the ret.pushKV("hdmasterkeyid", meta->hdMasterKeyID.GetHex()); in for now. (Can be removed in the next release, for example)

jnewbery commented at 9:01 pm on April 17, 2018:

Ah, I see. And also have hdseedid return the same value? Yes, I can do that.

I can also hide hdmasterkeyid behind a deprecation switch if you think that’s useful.

MarcoFalke commented at 9:11 pm on April 17, 2018:

Not sure if we need the overkill of a deprecation switch for every tiny api change. Just returning both and mentioning that one of the is deprecated in the docs seems pragmatic.

jnewbery commented at 9:35 pm on April 17, 2018:

Sounds good. I’ll just return both and mention in the release notes

The deprecation switch isn’t too heavy, so if anyone thinks it’s necessary I can add it.

jnewbery force-pushed on Apr 17, 2018

jnewbery commented at 9:49 pm on April 17, 2018: member

@MarcoFalke - addressed your review comment.

MarcoFalke commented at 6:34 pm on May 18, 2018: member

Needs rebase due to #12560?

MarcoFalke commented at 8:04 pm on May 18, 2018: member

In the scripted diff you might want to remove the line ren MasterKeyID SeedID, since it is unused. (Also, switching everything to camel case seems appropriate.)

utACK dead17b

jnewbery force-pushed on May 18, 2018

jnewbery commented at 10:25 pm on May 18, 2018: member

In the scripted diff you might want to remove the line ren MasterKeyID SeedID, since it is unused. (Also, switching everything to camel case seems appropriate.)

Thanks. Both done!

in test/functional/wallet_dump.py:39 in b824f56480 outdated

38@@ -39,7 +39,7 @@ def read_dump(file_name, addrs, script_addrs, hd_master_addr_old):
39                     if keytype == "inactivehdmaster=1":

MarcoFalke commented at 11:23 pm on May 18, 2018:

This one should be fixed up in commit 05dfe84ec2 [refactor] manually change remaining instances of master key to seed.

jnewbery commented at 3:26 pm on May 19, 2018:

done

MarcoFalke commented at 11:28 pm on May 18, 2018: member

nit: The DeriveNewMasterHDKey can be done in the scripted-diff I believe.

scripted-diff: Rename master key to seed

-BEGIN VERIFY SCRIPT-

ren() { git grep -l "\<$1\>" 'src/*.cpp' 'src/*.h' test | xargs sed -i "s:\<$1\>:$2:g"; }
ren GenerateNewHDMasterKey  GenerateNewSeed
ren DeriveNewMasterHDKey    DeriveNewSeed
ren SetHDMasterKey          SetHDSeed
ren hdMasterKeyID           hd_seed_id
ren masterKeyID             seed_id
ren SetMaster               SetSeed
ren hdmasterkeyid           hdseedid
ren hdmaster                hdseed

-END VERIFY SCRIPT-

131d4450b9

[refactor] manually change remaining instances of master key to seed. c75c351419

[rpc] [wallet] Add 'hdmasterkeyid' alias return values.

Restores the  return value in getwalletinfo() and getaddressinfo()
RPC methods for backwards compatibility

79053a5f2b

[docs] Add release notes for HD master key -> HD seed rename 6249021d15

jnewbery force-pushed on May 19, 2018

jnewbery commented at 3:26 pm on May 19, 2018: member

The DeriveNewMasterHDKey can be done in the scripted-diff

Done

MarcoFalke commented at 9:28 pm on May 19, 2018: member

utACK 6249021d152dec348eb4325c0dfccb3ba59f46d1

MarcoFalke commented at 9:49 pm on May 19, 2018: member

@achow101 and @jonasschnelli Mind to look at the fix, since both of you opened an issue report about this?

achow101 commented at 8:25 pm on May 20, 2018: member

utACK 79053a5f2b26ee3dfd9a0bb3fd01ac4733fc92b5

jonasschnelli commented at 7:41 am on May 21, 2018: contributor

(re) utACK 79053a5f2b26ee3dfd9a0bb3fd01ac4733fc92b5

jonasschnelli merged this on May 21, 2018

jonasschnelli closed this on May 21, 2018

jonasschnelli referenced this in commit 6738813bcb on May 21, 2018

jnewbery deleted the branch on May 21, 2018

in src/wallet/wallet.h:1145 in 6249021d15

1138@@ -1139,17 +1139,17 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
1139     /* Returns true if HD is enabled */
1140     bool IsHDEnabled() const;
1141 
1142-    /* Generates a new HD master key (will not be activated) */
1143-    CPubKey GenerateNewHDMasterKey();
1144+    /* Generates a new HD seed (will not be activated) */
1145+    CPubKey GenerateNewSeed();
1146 
1147     /* Derives a new HD master key (will not be activated) */

MarcoFalke commented at 4:07 pm on May 21, 2018:

Post merge nit: Should say seed here as well instead of “master key”?

jnewbery commented at 4:59 pm on May 21, 2018:

Fixed in #13297

MarcoFalke referenced this in commit d82c5d15c5 on May 21, 2018

underdarkskies referenced this in commit d54eb3eaa9 on Jul 30, 2018

sipa added the label Needs release notes on Aug 14, 2018

sipa removed the label Needs release notes on Aug 14, 2018

blondfrogs referenced this in commit 697eba709e on Sep 20, 2018

blondfrogs referenced this in commit 8bd572697c on Sep 20, 2018

blondfrogs referenced this in commit 93e29026f2 on Sep 20, 2018

Mengerian referenced this in commit 174f07f368 on Oct 31, 2019

UdjinM6 referenced this in commit a39799d0e9 on Jun 18, 2021

TheArbitrator referenced this in commit 674ca11edf on Jun 21, 2021

UdjinM6 referenced this in commit e1bebeb731 on Jun 24, 2021

UdjinM6 referenced this in commit 984a37825d on Jun 26, 2021

UdjinM6 referenced this in commit 7de8332d9e on Jun 26, 2021

UdjinM6 referenced this in commit 21bb444ad3 on Jun 28, 2021

MarcoFalke locked this on Sep 8, 2021

Fix hdmaster-key / seed-key confusion (scripted diff) #12924