The current usage seems to be an overloading of meanings. CScriptID is used in the wallet as a lookup key, as well as a destination, and CKeyID likewise. Instead, have all destinations be dedicated types.
New types:
CScriptID->ScriptHash
CKeyID->PKHash
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
79+ explicit PKHash(const uint160& hash) : uint160(hash) {}
80+ explicit PKHash(const CPubKey& pubkey);
81+ using uint160::uint160;
82+};
83+
84+struct SHash : public uint160
SHash though, it sounds too general. I’d prefer a longer name that makes it more clear what is being hashed, maybe ScriptHash?
83+
84+struct ScriptHash : public uint160
85+{
86+ ScriptHash() : uint160() {}
87+ explicit ScriptHash(const uint160& hash) : uint160(hash) {}
88+ explicit ScriptHash(const CScript& script);
Concept ACK. Maybe enumerate the new types in the PR description?
Shouldn’t (no actually that’s the point :-)CKeyID GetID() in pubkey.h also return a PKHash ? That breaks a few other things from the looks of it, but otherwise it seems inconsistent.
72@@ -73,6 +73,22 @@ class CNoDestination {
73 friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return true; }
74 };
75
76+struct PKHash : public uint160
77+{
78+ PKHash() : uint160() {}
79+ explicit PKHash(const uint160& hash) : uint160(hash) {}
80+ explicit PKHash(const CPubKey& pubkey);
3767@@ -3768,7 +3768,7 @@ void CWallet::GetKeyBirthTimes(interfaces::Chain::Lock& locked_chain, std::map<C
3768 // get birth times for keys with metadata
3769 for (const auto& entry : mapKeyMetadata) {
3770 if (entry.second.nCreateTime) {
3771- mapKeyBirth[entry.first] = entry.second.nCreateTime;
3772+ mapKeyBirth[PKHash(entry.first)] = entry.second.nCreateTime;
This change and the 2 changes below will work but don’t really make sense logically. Would suggest reverting these and just changing the map key type to:
0boost::variant<CKeyID, CScriptID>
or maybe
0CKeyID
if you wanted to drop the “TODO: include scripts in GetKeyBirthTimes() output instead of separate” comment.
Conditional utACK 51482131c645ad565ad032a57bd105ede65f9b86 if fixes are added for travis failures. I’m guessing this will just require rebasing and adding some more CKeyID/CScriptID replacements.
This change was also suggested a while ago in: #11403 (review) and I think it could be simplified a little by taking the other suggestion in that comment and making ScriptHash inherit from CScriptID and PKHash inherit from CKeyID. This would avoid the need to add explicit conversions several places in this PR. I think it’s important for conversions from CScriptID to ScriptHash and CKeyID to PKHash to be explicit, but it seems fine for conversions in the opposite direction to be implicit.
PKHash, and added a commit to change GetKeyBirthTimes to returning CKeyID instead of CTxDestination.
570@@ -571,7 +571,7 @@ static UniValue decodescript(const JSONRPCRequest& request)
571 if (type.isStr() && type.get_str() != "scripthash") {
572 // P2SH cannot be wrapped in a P2SH. If this script is already a P2SH,
573 // don't return the address for a P2SH of the P2SH.
574- r.pushKV("p2sh", EncodeDestination(CScriptID(script)));
575+ r.pushKV("p2sh", EncodeDestination(ScriptHash(CScriptID(script))));
ScriptHash(script)? Same on line 601
489@@ -490,8 +490,8 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine)
490 {
491 CBasicKeyStore keystore;
492
493- CScript redeemscript = GetScriptForDestination(pubkeys[0].GetID());
494- CScript witnessscript = GetScriptForDestination(CScriptID(redeemscript));
495+ CScript redeemscript = GetScriptForDestination(ScriptHash(PKHash(pubkeys[0])));
ScriptHash here?
597@@ -598,7 +598,7 @@ BOOST_AUTO_TEST_CASE(script_standard_IsMine)
598 BOOST_CHECK(keystore.AddKey(keys[1]));
599
600 CScript redeemScript = GetScriptForMultisig(2, {uncompressedPubkey, pubkeys[1]});
601- scriptPubKey = GetScriptForDestination(CScriptID(redeemScript));
602+ scriptPubKey = GetScriptForDestination(ScriptHash(CScriptID(redeemScript)));
ScriptHash(redeemScript) again?
utACK https://github.com/bitcoin/bitcoin/pull/15452/commits/78e407ad0c26190a22de1bc8ed900164a44a36c3
By removal of the // TODO: include scripts in GetKeyBirthTimes() output instead of separate comment, I assume you think that shouldn’t be done, rather than because you implemented it.