Merge #17938: Disallow automatic conversion between disparate hash types

4d73691 Disallow automatic conversion between hash types (Ben Woosley)
fa9ef2c Remove an apparently unnecessary conversion (Ben Woosley)
966a22d Explicitly support conversion between equivalent hash types (Ben Woosley)
f32c1e0 Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley)
2c54217 Use explicit conversion from PKHash -> CKeyID (Ben Woosley)
a9e451f Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley)
3fcc468 Prefer explicit CScriptID construction (Ben Woosley)
0a5ea32 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 4d73691
  meshcollider:
    re-utACK 4d73691

Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54

Author: meshcollider

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);

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);
             }

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.

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;
         }

src/script/signingprovider.cpp

@@ -180,18 +180,18 @@ 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;
         CScriptID script_id(*script_hash);
         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);
             }
         }
     }

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));
 }

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 @@ class CNoDestination {
     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

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 */);

src/wallet/rpcwallet.cpp

@@ -3517,7 +3517,7 @@ class DescribeWalletAddressVisitor : public boost::static_visitor<UniValue>
 
     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 @@ class DescribeWalletAddressVisitor : public boost::static_visitor<UniValue>
     {
         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;

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;
     }