Merge #17938: Disallow automatic conversion between disparate hash types

4d7369125a82214ea42b808a32b71b315a5c3c72 Disallow automatic conversion between hash types (Ben Woosley)
fa9ef2cdbed32438bdb32623af6e06f13ecd35e4 Remove an apparently unnecessary conversion (Ben Woosley)
966a22d859db37b1775e2180e5be032fc4fdf483 Explicitly support conversion between equivalent hash types (Ben Woosley)
f32c1e07fd6c174ff3f6406a619550d2f6c19360 Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley)
2c54217f913967703b404747133be67cf2f4feac Use explicit conversion from PKHash -> CKeyID (Ben Woosley)
a9e451f144480d7b170e49087df162989d31cd20 Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley)
3fcc46812334074d2c77a6233e8a961cd0785872 Prefer explicit CScriptID construction (Ben Woosley)
0a5ea32ce605984094c5552877cb99bc81654f2c Prefer explicit uint160 conversion (Ben Woosley)

Pull request description:

  This bases the script/standard hash types, TxDestination-related and CScriptID on a base template which does not silently convert the underlying `uintN` type.

  Inspired by and built on #17924. Commits are small and focused to ease review.

  Note some of these changes may be relative to existing bugs of the same sort as #17924. See particularly "Convert CPubKey to WitnessV0KeyHash directly" and "Remove an apparently unnecessary conversion".

ACKs for top commit:
  achow101:
    ACK 4d7369125a82214ea42b808a32b71b315a5c3c72
  meshcollider:
    re-utACK 4d7369125a82214ea42b808a32b71b315a5c3c72

Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54

11 files changed, 122 insertions, 42 deletions

diff --git a/src/outputtype.cpp b/src/outputtype.cpp
index ea7a86d6d6..871474d56e 100644
--- a/src/outputtype.cpp
+++ b/src/outputtype.cpp
@@ -53,7 +53,7 @@ CTxDestination GetDestinationForKey(const CPubKey& key, OutputType type)
     case OutputType::P2SH_SEGWIT:
     case OutputType::BECH32: {
         if (!key.IsCompressed()) return PKHash(key);
-        CTxDestination witdest = WitnessV0KeyHash(PKHash(key));
+        CTxDestination witdest = WitnessV0KeyHash(key);
         CScript witprog = GetScriptForDestination(witdest);
         if (type == OutputType::P2SH_SEGWIT) {
             return ScriptHash(witprog);
diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp
index f44a9f285a..7c72858501 100644
--- a/src/qt/coincontroldialog.cpp
+++ b/src/qt/coincontroldialog.cpp
@@ -456,7 +456,7 @@ void CoinControlDialog::updateLabels(CCoinControl& m_coin_control, WalletModel *
         {
             CPubKey pubkey;
             PKHash *pkhash = boost::get<PKHash>(&address);
-            if (pkhash && model->wallet().getPubKey(out.txout.scriptPubKey, CKeyID(*pkhash), pubkey))
+            if (pkhash && model->wallet().getPubKey(out.txout.scriptPubKey, ToKeyID(*pkhash), pubkey))
             {
                 nBytesInputs += (pubkey.IsCompressed() ? 148 : 180);
             }
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index e14217c307..814f8bddfe 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -595,7 +595,7 @@ static UniValue decodescript(const JSONRPCRequest& request)
             if (which_type == TX_PUBKEY) {
                 segwitScr = GetScriptForDestination(WitnessV0KeyHash(Hash160(solutions_data[0].begin(), solutions_data[0].end())));
             } else if (which_type == TX_PUBKEYHASH) {
-                segwitScr = GetScriptForDestination(WitnessV0KeyHash(solutions_data[0]));
+                segwitScr = GetScriptForDestination(WitnessV0KeyHash(uint160{solutions_data[0]}));
             } else {
                 // Scripts that are not fit for P2WPKH are encoded as P2WSH.
                 // Newer segwit program versions should be considered when then become available.
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index 1e00afcf89..43988c4fd7 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -131,7 +131,7 @@ static bool SignStep(const SigningProvider& provider, const BaseSignatureCreator
     }
     case TX_SCRIPTHASH:
         h160 = uint160(vSolutions[0]);
-        if (GetCScript(provider, sigdata, h160, scriptRet)) {
+        if (GetCScript(provider, sigdata, CScriptID{h160}, scriptRet)) {
             ret.push_back(std::vector<unsigned char>(scriptRet.begin(), scriptRet.end()));
             return true;
         }
@@ -165,7 +165,7 @@ static bool SignStep(const SigningProvider& provider, const BaseSignatureCreator
 
     case TX_WITNESS_V0_SCRIPTHASH:
         CRIPEMD160().Write(&vSolutions[0][0], vSolutions[0].size()).Finalize(h160.begin());
-        if (GetCScript(provider, sigdata, h160, scriptRet)) {
+        if (GetCScript(provider, sigdata, CScriptID{h160}, scriptRet)) {
             ret.push_back(std::vector<unsigned char>(scriptRet.begin(), scriptRet.end()));
             return true;
         }
@@ -458,7 +458,7 @@ bool IsSegWitOutput(const SigningProvider& provider, const CScript& script)
     if (whichtype == TX_SCRIPTHASH) {
         auto h160 = uint160(solutions[0]);
         CScript subscript;
-        if (provider.GetCScript(h160, subscript)) {
+        if (provider.GetCScript(CScriptID{h160}, subscript)) {
             whichtype = Solver(subscript, solutions);
             if (whichtype == TX_WITNESS_V0_SCRIPTHASH || whichtype == TX_WITNESS_V0_KEYHASH || whichtype == TX_WITNESS_UNKNOWN) return true;
         }
diff --git a/src/script/signingprovider.cpp b/src/script/signingprovider.cpp
index 01757e2f65..2d8dc7d471 100644
--- a/src/script/signingprovider.cpp
+++ b/src/script/signingprovider.cpp
@@ -180,10 +180,10 @@ CKeyID GetKeyForDestination(const SigningProvider& store, const CTxDestination&
     // Only supports destinations which map to single public keys, i.e. P2PKH,
     // P2WPKH, and P2SH-P2WPKH.
     if (auto id = boost::get<PKHash>(&dest)) {
-        return CKeyID(*id);
+        return ToKeyID(*id);
     }
     if (auto witness_id = boost::get<WitnessV0KeyHash>(&dest)) {
-        return CKeyID(*witness_id);
+        return ToKeyID(*witness_id);
     }
     if (auto script_hash = boost::get<ScriptHash>(&dest)) {
         CScript script;
@@ -191,7 +191,7 @@ CKeyID GetKeyForDestination(const SigningProvider& store, const CTxDestination&
         CTxDestination inner_dest;
         if (store.GetCScript(script_id, script) && ExtractDestination(script, inner_dest)) {
             if (auto inner_witness_id = boost::get<WitnessV0KeyHash>(&inner_dest)) {
-                return CKeyID(*inner_witness_id);
+                return ToKeyID(*inner_witness_id);
             }
         }
     }
diff --git a/src/script/standard.cpp b/src/script/standard.cpp
index d199a84cee..2adf6ce56d 100644
--- a/src/script/standard.cpp
+++ b/src/script/standard.cpp
@@ -16,11 +16,27 @@ typedef std::vector<unsigned char> valtype;
 bool fAcceptDatacarrier = DEFAULT_ACCEPT_DATACARRIER;
 unsigned nMaxDatacarrierBytes = MAX_OP_RETURN_RELAY;
 
-CScriptID::CScriptID(const CScript& in) : uint160(Hash160(in.begin(), in.end())) {}
+CScriptID::CScriptID(const CScript& in) : BaseHash(Hash160(in.begin(), in.end())) {}
+CScriptID::CScriptID(const ScriptHash& in) : BaseHash(static_cast<uint160>(in)) {}
 
-ScriptHash::ScriptHash(const CScript& in) : uint160(Hash160(in.begin(), in.end())) {}
+ScriptHash::ScriptHash(const CScript& in) : BaseHash(Hash160(in.begin(), in.end())) {}
+ScriptHash::ScriptHash(const CScriptID& in) : BaseHash(static_cast<uint160>(in)) {}
 
-PKHash::PKHash(const CPubKey& pubkey) : uint160(pubkey.GetID()) {}
+PKHash::PKHash(const CPubKey& pubkey) : BaseHash(pubkey.GetID()) {}
+PKHash::PKHash(const CKeyID& pubkey_id) : BaseHash(pubkey_id) {}
+
+WitnessV0KeyHash::WitnessV0KeyHash(const CPubKey& pubkey) : BaseHash(pubkey.GetID()) {}
+WitnessV0KeyHash::WitnessV0KeyHash(const PKHash& pubkey_hash) : BaseHash(static_cast<uint160>(pubkey_hash)) {}
+
+CKeyID ToKeyID(const PKHash& key_hash)
+{
+    return CKeyID{static_cast<uint160>(key_hash)};
+}
+
+CKeyID ToKeyID(const WitnessV0KeyHash& key_hash)
+{
+    return CKeyID{static_cast<uint160>(key_hash)};
+}
 
 WitnessV0ScriptHash::WitnessV0ScriptHash(const CScript& in)
 {
@@ -307,7 +323,7 @@ CScript GetScriptForWitness(const CScript& redeemscript)
     if (typ == TX_PUBKEY) {
         return GetScriptForDestination(WitnessV0KeyHash(Hash160(vSolutions[0].begin(), vSolutions[0].end())));
     } else if (typ == TX_PUBKEYHASH) {
-        return GetScriptForDestination(WitnessV0KeyHash(vSolutions[0]));
+        return GetScriptForDestination(WitnessV0KeyHash(uint160{vSolutions[0]}));
     }
     return GetScriptForDestination(WitnessV0ScriptHash(redeemscript));
 }
diff --git a/src/script/standard.h b/src/script/standard.h
index 2929425670..4baed6da6e 100644
--- a/src/script/standard.h
+++ b/src/script/standard.h
@@ -18,14 +18,77 @@ static const bool DEFAULT_ACCEPT_DATACARRIER = true;
 
 class CKeyID;
 class CScript;
+struct ScriptHash;
+
+template<typename HashType>
+class BaseHash
+{
+protected:
+    HashType m_hash;
+
+public:
+    BaseHash() : m_hash() {}
+    BaseHash(const HashType& in) : m_hash(in) {}
+
+    unsigned char* begin()
+    {
+        return m_hash.begin();
+    }
+
+    const unsigned char* begin() const
+    {
+        return m_hash.begin();
+    }
+
+    unsigned char* end()
+    {
+        return m_hash.end();
+    }
+
+    const unsigned char* end() const
+    {
+        return m_hash.end();
+    }
+
+    operator std::vector<unsigned char>() const
+    {
+        return std::vector<unsigned char>{m_hash.begin(), m_hash.end()};
+    }
+
+    std::string ToString() const
+    {
+        return m_hash.ToString();
+    }
+
+    bool operator==(const BaseHash<HashType>& other) const noexcept
+    {
+        return m_hash == other.m_hash;
+    }
+
+    bool operator!=(const BaseHash<HashType>& other) const noexcept
+    {
+        return !(m_hash == other.m_hash);
+    }
+
+    bool operator<(const BaseHash<HashType>& other) const noexcept
+    {
+        return m_hash < other.m_hash;
+    }
+
+    size_t size() const
+    {
+        return m_hash.size();
+    }
+};
 
 /** A reference to a CScript: the Hash160 of its serialization (see script.h) */
-class CScriptID : public uint160
+class CScriptID : public BaseHash<uint160>
 {
 public:
-    CScriptID() : uint160() {}
+    CScriptID() : BaseHash() {}
     explicit CScriptID(const CScript& in);
-    CScriptID(const uint160& in) : uint160(in) {}
+    explicit CScriptID(const uint160& in) : BaseHash(in) {}
+    explicit CScriptID(const ScriptHash& in);
 };
 
 /**
@@ -73,41 +136,44 @@ public:
     friend bool operator<(const CNoDestination &a, const CNoDestination &b) { return true; }
 };
 
-struct PKHash : public uint160
+struct PKHash : public BaseHash<uint160>
 {
-    PKHash() : uint160() {}
-    explicit PKHash(const uint160& hash) : uint160(hash) {}
+    PKHash() : BaseHash() {}
+    explicit PKHash(const uint160& hash) : BaseHash(hash) {}
     explicit PKHash(const CPubKey& pubkey);
-    using uint160::uint160;
+    explicit PKHash(const CKeyID& pubkey_id);
 };
+CKeyID ToKeyID(const PKHash& key_hash);
 
 struct WitnessV0KeyHash;
-struct ScriptHash : public uint160
+struct ScriptHash : public BaseHash<uint160>
 {
-    ScriptHash() : uint160() {}
+    ScriptHash() : BaseHash() {}
     // These don't do what you'd expect.
     // Use ScriptHash(GetScriptForDestination(...)) instead.
     explicit ScriptHash(const WitnessV0KeyHash& hash) = delete;
     explicit ScriptHash(const PKHash& hash) = delete;
-    explicit ScriptHash(const uint160& hash) : uint160(hash) {}
+
+    explicit ScriptHash(const uint160& hash) : BaseHash(hash) {}
     explicit ScriptHash(const CScript& script);
-    using uint160::uint160;
+    explicit ScriptHash(const CScriptID& script);
 };
 
-struct WitnessV0ScriptHash : public uint256
+struct WitnessV0ScriptHash : public BaseHash<uint256>
 {
-    WitnessV0ScriptHash() : uint256() {}
-    explicit WitnessV0ScriptHash(const uint256& hash) : uint256(hash) {}
+    WitnessV0ScriptHash() : BaseHash() {}
+    explicit WitnessV0ScriptHash(const uint256& hash) : BaseHash(hash) {}
     explicit WitnessV0ScriptHash(const CScript& script);
-    using uint256::uint256;
 };
 
-struct WitnessV0KeyHash : public uint160
+struct WitnessV0KeyHash : public BaseHash<uint160>
 {
-    WitnessV0KeyHash() : uint160() {}
-    explicit WitnessV0KeyHash(const uint160& hash) : uint160(hash) {}
-    using uint160::uint160;
+    WitnessV0KeyHash() : BaseHash() {}
+    explicit WitnessV0KeyHash(const uint160& hash) : BaseHash(hash) {}
+    explicit WitnessV0KeyHash(const CPubKey& pubkey);
+    explicit WitnessV0KeyHash(const PKHash& pubkey_hash);
 };
+CKeyID ToKeyID(const WitnessV0KeyHash& key_hash);
 
 //! CTxDestination subtype to encode any future Witness version
 struct WitnessUnknown
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
index d9fe741a6e..541675bcef 100644
--- a/src/wallet/rpcdump.cpp
+++ b/src/wallet/rpcdump.cpp
@@ -297,7 +297,7 @@ UniValue importaddress(const JSONRPCRequest& request)
             pwallet->ImportScripts(scripts, 0 /* timestamp */);
 
             if (fP2SH) {
-                scripts.insert(GetScriptForDestination(ScriptHash(CScriptID(redeem_script))));
+                scripts.insert(GetScriptForDestination(ScriptHash(redeem_script)));
             }
 
             pwallet->ImportScriptPubKeys(strLabel, scripts, false /* have_solving_data */, true /* apply_label */, 1 /* timestamp */);
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 8cb53e0698..b751cdd88a 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3517,7 +3517,7 @@ public:
 
     UniValue operator()(const PKHash& pkhash) const
     {
-        CKeyID keyID(pkhash);
+        CKeyID keyID{ToKeyID(pkhash)};
         UniValue obj(UniValue::VOBJ);
         CPubKey vchPubKey;
         if (provider && provider->GetPubKey(keyID, vchPubKey)) {
@@ -3542,7 +3542,7 @@ public:
     {
         UniValue obj(UniValue::VOBJ);
         CPubKey pubkey;
-        if (provider && provider->GetPubKey(CKeyID(id), pubkey)) {
+        if (provider && provider->GetPubKey(ToKeyID(id), pubkey)) {
             obj.pushKV("pubkey", HexStr(pubkey));
         }
         return obj;
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
index 8a2a798644..6ec34951ec 100644
--- a/src/wallet/scriptpubkeyman.cpp
+++ b/src/wallet/scriptpubkeyman.cpp
@@ -573,9 +573,8 @@ bool LegacyScriptPubKeyMan::SignTransaction(CMutableTransaction& tx, const std::
 
 SigningResult LegacyScriptPubKeyMan::SignMessage(const std::string& message, const PKHash& pkhash, std::string& str_sig) const
 {
-    CKeyID key_id(pkhash);
     CKey key;
-    if (!GetKey(key_id, key)) {
+    if (!GetKey(ToKeyID(pkhash), key)) {
         return SigningResult::PRIVATE_KEY_NOT_AVAILABLE;
     }
 
@@ -2052,9 +2051,8 @@ SigningResult DescriptorScriptPubKeyMan::SignMessage(const std::string& message,
         return SigningResult::PRIVATE_KEY_NOT_AVAILABLE;
     }
 
-    CKeyID key_id(pkhash);
     CKey key;
-    if (!keys->GetKey(key_id, key)) {
+    if (!keys->GetKey(ToKeyID(pkhash), key)) {
         return SigningResult::PRIVATE_KEY_NOT_AVAILABLE;
     }
 
diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp
index cdb0522920..e416f16044 100644
--- a/src/wallet/test/ismine_tests.cpp
+++ b/src/wallet/test/ismine_tests.cpp
@@ -167,7 +167,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard)
         keystore.SetupLegacyScriptPubKeyMan();
         LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore);
 
-        CScript witnessscript = GetScriptForDestination(WitnessV0KeyHash(PKHash(pubkeys[0])));
+        CScript witnessscript = GetScriptForDestination(WitnessV0KeyHash(pubkeys[0]));
         scriptPubKey = GetScriptForDestination(WitnessV0ScriptHash(witnessscript));
 
         BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddCScript(witnessscript));
@@ -202,7 +202,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard)
         LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore);
         BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0]));
 
-        scriptPubKey = GetScriptForDestination(WitnessV0KeyHash(PKHash(pubkeys[0])));
+        scriptPubKey = GetScriptForDestination(WitnessV0KeyHash(pubkeys[0]));
 
         // Keystore implicitly has key and P2SH redeemScript
         BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddCScript(scriptPubKey));
@@ -217,7 +217,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard)
         LOCK(keystore.GetLegacyScriptPubKeyMan()->cs_KeyStore);
         BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(uncompressedKey));
 
-        scriptPubKey = GetScriptForDestination(WitnessV0KeyHash(PKHash(uncompressedPubkey)));
+        scriptPubKey = GetScriptForDestination(WitnessV0KeyHash(uncompressedPubkey));
 
         // Keystore has key, but no P2SH redeemScript
         result = keystore.GetLegacyScriptPubKeyMan()->IsMine(scriptPubKey);