Merge bitcoin#11117: Prepare for non-Base58 addresses (#3294)

* Merge bitcoin#11117: Prepare for non-Base58 addresses

864cd27 Move CBitcoinAddress to base58.cpp (Pieter Wuille)
5c8ff0d Introduce wrappers around CBitcoinAddress (Pieter Wuille)

Pull request description:

  This patch removes the need for the intermediary Base58 type `CBitcoinAddress`, by providing {`Encode`,`Decode`,`IsValid`}`Destination` functions that directly operate on the conversion between `std::string`s and `CTxDestination`.

  As a side, it also fixes a number of indentation issues, and removes probably several unnecessary implicit `CTxDestination`<->`CBitcoinAddress` conversions.

  This change is far from complete. In follow-ups I'd like to:
  * Split off the specific address and key encoding logic from base58.h, and move it to a address.h or so.
  * Replace `CTxDestination` with a non-`boost::variant` version (which can be more efficient as `boost::variant` allocates everything on the heap, and remove the need for `boost::get<...>` and `IsValidDestination` calls everywhere).
  * Do the same for `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey`.

  However, I've tried to keep this patch to be minimally invasive, but still enough to support non-Base58 addresses. Perhaps a smaller patch is possible to hack Bech32 support into `CBitcoinAddress`, but I would consider that a move in the wrong direction.

Tree-SHA512: c2c77ffb57caeadf2429b1c2562ce60e8c7be8aa9f8e51b591f354b6b441162625b2efe14c023a1ae485cf2ed417263afa35c892891dfaa7844e7fbabccab85e

* CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* more CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* more CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* more CBitcoinAddress -> EncodeDestination in providertx.h

Signed-off-by: Pasta <pasta@dashboost.org>

* fix CBitcoinAddress GetKeyID check

Signed-off-by: Pasta <pasta@dashboost.org>

* fix providertx.cpp

Signed-off-by: Pasta <pasta@dashboost.org>

* hopefully fix governance-classes.cpp

Signed-off-by: Pasta <pasta@dashboost.org>

* partially fix governance-validators.cpp, unable to resolve "address.IsScript()"

Signed-off-by: Pasta <pasta@dashboost.org>

* partially fix governance-classes.cpp, unable to resolve "address.IsScript()"

Signed-off-by: Pasta <pasta@dashboost.org>

* fix governance-classes.h

Signed-off-by: Pasta <pasta@dashboost.org>

* DecodeTransaction -> DecodeDestination, fix governance-validators.cpp

Signed-off-by: Pasta <pasta@dashboost.org>

* More fixes for 3294

* Move GetIndexKey into rpc/misc.cpp near getAddressesFromParams

No need to have it in base58.cpp anymore as this is only used in getAddressesFromParams

Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: Alexander Block <ablock84@gmail.com>

Author: 4 people

src/base58.cpp

@@ -212,6 +212,29 @@ int CBase58Data::CompareTo(const CBase58Data& b58) const
 
 namespace
 {
+/** base58-encoded Bitcoin addresses.
+ * Public-key-hash-addresses have version 0 (or 111 testnet).
+ * The data vector contains RIPEMD160(SHA256(pubkey)), where pubkey is the serialized public key.
+ * Script-hash-addresses have version 5 (or 196 testnet).
+ * The data vector contains RIPEMD160(SHA256(cscript)), where cscript is the serialized redemption script.
+ */
+class CBitcoinAddress : public CBase58Data {
+public:
+    bool Set(const CKeyID &id);
+    bool Set(const CScriptID &id);
+    bool Set(const CTxDestination &dest);
+    bool IsValid() const;
+    bool IsValid(const CChainParams &params) const;
+
+    CBitcoinAddress() {}
+    CBitcoinAddress(const CTxDestination &dest) { Set(dest); }
+    CBitcoinAddress(const std::string& strAddress) { SetString(strAddress); }
+    CBitcoinAddress(const char* pszAddress) { SetString(pszAddress); }
+
+    CTxDestination Get() const;
+    bool GetKeyID(CKeyID &keyID) const;
+};
+
 class CBitcoinAddressVisitor : public boost::static_visitor<bool>
 {
 private:
@@ -271,23 +294,6 @@ CTxDestination CBitcoinAddress::Get() const
         return CNoDestination();
 }
 
-bool CBitcoinAddress::GetIndexKey(uint160& hashBytes, int& type) const
-{
-    if (!IsValid()) {
-        return false;
-    } else if (vchVersion == Params().Base58Prefix(CChainParams::PUBKEY_ADDRESS)) {
-        memcpy(&hashBytes, vchData.data(), 20);
-        type = 1;
-        return true;
-    } else if (vchVersion == Params().Base58Prefix(CChainParams::SCRIPT_ADDRESS)) {
-        memcpy(&hashBytes, vchData.data(), 20);
-        type = 2;
-        return true;
-    }
-
-    return false;
-}
-
 bool CBitcoinAddress::GetKeyID(CKeyID& keyID) const
 {
     if (!IsValid() || vchVersion != Params().Base58Prefix(CChainParams::PUBKEY_ADDRESS))
@@ -298,11 +304,6 @@ bool CBitcoinAddress::GetKeyID(CKeyID& keyID) const
     return true;
 }
 
-bool CBitcoinAddress::IsScript() const
-{
-    return IsValid() && vchVersion == Params().Base58Prefix(CChainParams::SCRIPT_ADDRESS);
-}
-
 void CBitcoinSecret::SetKey(const CKey& vchSecret)
 {
     assert(vchSecret.IsValid());
@@ -335,3 +336,26 @@ bool CBitcoinSecret::SetString(const std::string& strSecret)
 {
     return SetString(strSecret.c_str());
 }
+
+std::string EncodeDestination(const CTxDestination& dest)
+{
+    CBitcoinAddress addr(dest);
+    if (!addr.IsValid()) return "";
+    return addr.ToString();
+}
+
+CTxDestination DecodeDestination(const std::string& str)
+{
+    return CBitcoinAddress(str).Get();
+}
+
+bool IsValidDestinationString(const std::string& str, const CChainParams& params)
+{
+    return CBitcoinAddress(str).IsValid(params);
+}
+
+bool IsValidDestinationString(const std::string& str)
+{
+    return CBitcoinAddress(str).IsValid();
+}
+

src/base58.h

@@ -95,31 +95,6 @@ class CBase58Data
     bool operator> (const CBase58Data& b58) const { return CompareTo(b58) >  0; }
 };
 
-/** base58-encoded Dash addresses.
- * Public-key-hash-addresses have version 76 (or 140 testnet).
- * The data vector contains RIPEMD160(SHA256(pubkey)), where pubkey is the serialized public key.
- * Script-hash-addresses have version 16 (or 19 testnet).
- * The data vector contains RIPEMD160(SHA256(cscript)), where cscript is the serialized redemption script.
- */
-class CBitcoinAddress : public CBase58Data {
-public:
-    bool Set(const CKeyID &id);
-    bool Set(const CScriptID &id);
-    bool Set(const CTxDestination &dest);
-    bool IsValid() const;
-    bool IsValid(const CChainParams &params) const;
-
-    CBitcoinAddress() {}
-    CBitcoinAddress(const CTxDestination &dest) { Set(dest); }
-    CBitcoinAddress(const std::string& strAddress) { SetString(strAddress); }
-    CBitcoinAddress(const char* pszAddress) { SetString(pszAddress); }
-
-    CTxDestination Get() const;
-    bool GetKeyID(CKeyID &keyID) const;
-    bool GetIndexKey(uint160& hashBytes, int& type) const;
-    bool IsScript() const;
-};
-
 /**
  * A base58-encoded secret key
  */
@@ -168,4 +143,9 @@ template<typename K, int Size, CChainParams::Base58Type Type> class CBitcoinExtK
 typedef CBitcoinExtKeyBase<CExtKey, BIP32_EXTKEY_SIZE, CChainParams::EXT_SECRET_KEY> CBitcoinExtKey;
 typedef CBitcoinExtKeyBase<CExtPubKey, BIP32_EXTKEY_SIZE, CChainParams::EXT_PUBLIC_KEY> CBitcoinExtPubKey;
 
+std::string EncodeDestination(const CTxDestination& dest);
+CTxDestination DecodeDestination(const std::string& str);
+bool IsValidDestinationString(const std::string& str);
+bool IsValidDestinationString(const std::string& str, const CChainParams& params);
+
 #endif // BITCOIN_BASE58_H

src/core_write.cpp

@@ -154,8 +154,9 @@ void ScriptPubKeyToUniv(const CScript& scriptPubKey,
     out.pushKV("type", GetTxnOutputType(type));
 
     UniValue a(UniValue::VARR);
-    for (const CTxDestination& addr : addresses)
-        a.push_back(CBitcoinAddress(addr).ToString());
+    for (const CTxDestination& addr : addresses) {
+        a.push_back(EncodeDestination(addr));
+    }
     out.pushKV("addresses", a);
 }
 
@@ -190,9 +191,9 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry,
                     in.push_back(Pair("value", ValueFromAmount(spentInfo.satoshis)));
                     in.push_back(Pair("valueSat", spentInfo.satoshis));
                     if (spentInfo.addressType == 1) {
-                        in.push_back(Pair("address", CBitcoinAddress(CKeyID(spentInfo.addressHash)).ToString()));
+                        in.push_back(Pair("address", EncodeDestination(CKeyID(spentInfo.addressHash))));
                     } else if (spentInfo.addressType == 2) {
-                        in.push_back(Pair("address", CBitcoinAddress(CScriptID(spentInfo.addressHash)).ToString()));
+                        in.push_back(Pair("address", EncodeDestination(CScriptID(spentInfo.addressHash))));
                     }
                 }
             }

src/dash-tx.cpp

@@ -253,11 +253,11 @@ static void MutateTxAddOutAddr(CMutableTransaction& tx, const std::string& strIn
 
     // extract and validate ADDRESS
     std::string strAddr = vStrInputParts[1];
-    CBitcoinAddress addr(strAddr);
-    if (!addr.IsValid())
+    CTxDestination destination = DecodeDestination(strAddr);
+    if (!IsValidDestination(destination)) {
         throw std::runtime_error("invalid TX output address");
-    // build standard output script via GetScriptForDestination()
-    CScript scriptPubKey = GetScriptForDestination(addr.Get());
+    }
+    CScript scriptPubKey = GetScriptForDestination(destination);
 
     // construct TxOut, append to transaction output list
     CTxOut txout(value, scriptPubKey);
@@ -290,10 +290,8 @@ static void MutateTxAddOutPubKey(CMutableTransaction& tx, const std::string& str
     }
 
     if (bScriptHash) {
-        // Get the address for the redeem script, then call
-        // GetScriptForDestination() to construct a P2SH scriptPubKey.
-        CBitcoinAddress redeemScriptAddr(scriptPubKey);
-        scriptPubKey = GetScriptForDestination(redeemScriptAddr.Get());
+        // Get the ID for the script, and then construct a P2SH destination for it.
+        scriptPubKey = GetScriptForDestination(CScriptID(scriptPubKey));
     }
 
     // construct TxOut, append to transaction output list
@@ -355,9 +353,8 @@ static void MutateTxAddOutMultiSig(CMutableTransaction& tx, const std::string& s
             throw std::runtime_error(strprintf(
                         "redeemScript exceeds size limit: %d > %d", scriptPubKey.size(), MAX_SCRIPT_ELEMENT_SIZE));
         }
-        // GetScriptForDestination() to construct a P2SH scriptPubKey.
-        CBitcoinAddress addr(scriptPubKey);
-        scriptPubKey = GetScriptForDestination(addr.Get());
+        // Get the ID for the script, and then construct a P2SH destination for it.
+        scriptPubKey = GetScriptForDestination(CScriptID(scriptPubKey));
     }
 
     // construct TxOut, append to transaction output list
@@ -424,8 +421,7 @@ static void MutateTxAddOutScript(CMutableTransaction& tx, const std::string& str
             throw std::runtime_error(strprintf(
                         "redeemScript exceeds size limit: %d > %d", scriptPubKey.size(), MAX_SCRIPT_ELEMENT_SIZE));
         }
-        CBitcoinAddress addr(scriptPubKey);
-        scriptPubKey = GetScriptForDestination(addr.Get());
+        scriptPubKey = GetScriptForDestination(CScriptID(scriptPubKey));
     }
 
     // construct TxOut, append to transaction output list

src/evo/deterministicmns.cpp

@@ -29,16 +29,16 @@ std::string CDeterministicMNState::ToString() const
     std::string payoutAddress = "unknown";
     std::string operatorPayoutAddress = "none";
     if (ExtractDestination(scriptPayout, dest)) {
-        payoutAddress = CBitcoinAddress(dest).ToString();
+        payoutAddress = EncodeDestination(dest);
     }
     if (ExtractDestination(scriptOperatorPayout, dest)) {
-        operatorPayoutAddress = CBitcoinAddress(dest).ToString();
+        operatorPayoutAddress = EncodeDestination(dest);
     }
 
     return strprintf("CDeterministicMNState(nRegisteredHeight=%d, nLastPaidHeight=%d, nPoSePenalty=%d, nPoSeRevivedHeight=%d, nPoSeBanHeight=%d, nRevocationReason=%d, "
         "ownerAddress=%s, pubKeyOperator=%s, votingAddress=%s, addr=%s, payoutAddress=%s, operatorPayoutAddress=%s)",
         nRegisteredHeight, nLastPaidHeight, nPoSePenalty, nPoSeRevivedHeight, nPoSeBanHeight, nRevocationReason,
-        CBitcoinAddress(keyIDOwner).ToString(), pubKeyOperator.Get().ToString(), CBitcoinAddress(keyIDVoting).ToString(), addr.ToStringIPPort(false), payoutAddress, operatorPayoutAddress);
+        EncodeDestination(keyIDOwner), pubKeyOperator.Get().ToString(), EncodeDestination(keyIDVoting), addr.ToStringIPPort(false), payoutAddress, operatorPayoutAddress);
 }
 
 void CDeterministicMNState::ToJson(UniValue& obj) const
@@ -52,18 +52,16 @@ void CDeterministicMNState::ToJson(UniValue& obj) const
     obj.push_back(Pair("PoSeRevivedHeight", nPoSeRevivedHeight));
     obj.push_back(Pair("PoSeBanHeight", nPoSeBanHeight));
     obj.push_back(Pair("revocationReason", nRevocationReason));
-    obj.push_back(Pair("ownerAddress", CBitcoinAddress(keyIDOwner).ToString()));
-    obj.push_back(Pair("votingAddress", CBitcoinAddress(keyIDVoting).ToString()));
+    obj.push_back(Pair("ownerAddress", EncodeDestination(keyIDOwner)));
+    obj.push_back(Pair("votingAddress", EncodeDestination(keyIDVoting)));
 
     CTxDestination dest;
     if (ExtractDestination(scriptPayout, dest)) {
-        CBitcoinAddress payoutAddress(dest);
-        obj.push_back(Pair("payoutAddress", payoutAddress.ToString()));
+        obj.push_back(Pair("payoutAddress", EncodeDestination(dest)));
     }
     obj.push_back(Pair("pubKeyOperator", pubKeyOperator.Get().ToString()));
     if (ExtractDestination(scriptOperatorPayout, dest)) {
-        CBitcoinAddress operatorPayoutAddress(dest);
-        obj.push_back(Pair("operatorPayoutAddress", operatorPayoutAddress.ToString()));
+        obj.push_back(Pair("operatorPayoutAddress", EncodeDestination(dest)));
     }
 }
 
@@ -88,7 +86,7 @@ void CDeterministicMN::ToJson(UniValue& obj) const
     if (GetUTXOCoin(collateralOutpoint, coin)) {
         CTxDestination dest;
         if (ExtractDestination(coin.out.scriptPubKey, dest)) {
-            obj.push_back(Pair("collateralAddress", CBitcoinAddress(dest).ToString()));
+            obj.push_back(Pair("collateralAddress", EncodeDestination(dest)));
         }
     }
 

src/evo/providertx.cpp

@@ -132,7 +132,7 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid
     }
 
     CTxDestination collateralTxDest;
-    CKeyID keyForPayloadSig;
+    const CKeyID *keyForPayloadSig = nullptr;
     COutPoint collateralOutpoint;
 
     if (!ptx.collateralOutpoint.hash.IsNull()) {
@@ -147,7 +147,8 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid
 
         // Extract key from collateral. This only works for P2PK and P2PKH collaterals and will fail for P2SH.
         // Issuer of this ProRegTx must prove ownership with this key by signing the ProRegTx
-        if (!CBitcoinAddress(collateralTxDest).GetKeyID(keyForPayloadSig)) {
+        keyForPayloadSig = boost::get<CKeyID>(&collateralTxDest);
+        if (!keyForPayloadSig) {
             return state.DoS(10, false, REJECT_INVALID, "bad-protx-collateral-pkh");
         }
 
@@ -197,9 +198,9 @@ bool CheckProRegTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CValid
         return false;
     }
 
-    if (!keyForPayloadSig.IsNull()) {
+    if (keyForPayloadSig) {
         // collateral is not part of this ProRegTx, so we must verify ownership of the collateral
-        if (!CheckStringSig(ptx, keyForPayloadSig, state)) {
+        if (!CheckStringSig(ptx, *keyForPayloadSig, state)) {
             return false;
         }
     } else {
@@ -390,18 +391,17 @@ std::string CProRegTx::MakeSignString() const
     // We only include the important stuff in the string form...
 
     CTxDestination destPayout;
-    CBitcoinAddress addrPayout;
     std::string strPayout;
-    if (ExtractDestination(scriptPayout, destPayout) && addrPayout.Set(destPayout)) {
-        strPayout = addrPayout.ToString();
+    if (ExtractDestination(scriptPayout, destPayout)) {
+        strPayout = EncodeDestination(destPayout);
     } else {
         strPayout = HexStr(scriptPayout.begin(), scriptPayout.end());
     }
 
     s += strPayout + "|";
     s += strprintf("%d", nOperatorReward) + "|";
-    s += CBitcoinAddress(keyIDOwner).ToString() + "|";
-    s += CBitcoinAddress(keyIDVoting).ToString() + "|";
+    s += EncodeDestination(keyIDOwner) + "|";
+    s += EncodeDestination(keyIDVoting) + "|";
 
     // ... and also the full hash of the payload as a protection agains malleability and replays
     s += ::SerializeHash(*this).ToString();
@@ -414,19 +414,19 @@ std::string CProRegTx::ToString() const
     CTxDestination dest;
     std::string payee = "unknown";
     if (ExtractDestination(scriptPayout, dest)) {
-        payee = CBitcoinAddress(dest).ToString();
+        payee = EncodeDestination(dest);
     }
 
     return strprintf("CProRegTx(nVersion=%d, collateralOutpoint=%s, addr=%s, nOperatorReward=%f, ownerAddress=%s, pubKeyOperator=%s, votingAddress=%s, scriptPayout=%s)",
-        nVersion, collateralOutpoint.ToStringShort(), addr.ToString(), (double)nOperatorReward / 100, CBitcoinAddress(keyIDOwner).ToString(), pubKeyOperator.ToString(), CBitcoinAddress(keyIDVoting).ToString(), payee);
+        nVersion, collateralOutpoint.ToStringShort(), addr.ToString(), (double)nOperatorReward / 100, EncodeDestination(keyIDOwner), pubKeyOperator.ToString(), EncodeDestination(keyIDVoting), payee);
 }
 
 std::string CProUpServTx::ToString() const
 {
     CTxDestination dest;
     std::string payee = "unknown";
     if (ExtractDestination(scriptOperatorPayout, dest)) {
-        payee = CBitcoinAddress(dest).ToString();
+        payee = EncodeDestination(dest);
     }
 
     return strprintf("CProUpServTx(nVersion=%d, proTxHash=%s, addr=%s, operatorPayoutAddress=%s)",
@@ -438,11 +438,11 @@ std::string CProUpRegTx::ToString() const
     CTxDestination dest;
     std::string payee = "unknown";
     if (ExtractDestination(scriptPayout, dest)) {
-        payee = CBitcoinAddress(dest).ToString();
+        payee = EncodeDestination(dest);
     }
 
     return strprintf("CProUpRegTx(nVersion=%d, proTxHash=%s, pubKeyOperator=%s, votingAddress=%s, payoutAddress=%s)",
-        nVersion, proTxHash.ToString(), pubKeyOperator.ToString(), CBitcoinAddress(keyIDVoting).ToString(), payee);
+        nVersion, proTxHash.ToString(), pubKeyOperator.ToString(), EncodeDestination(keyIDVoting), payee);
 }
 
 std::string CProUpRevTx::ToString() const

src/evo/providertx.h

@@ -71,13 +71,12 @@ class CProRegTx
         obj.push_back(Pair("collateralHash", collateralOutpoint.hash.ToString()));
         obj.push_back(Pair("collateralIndex", (int)collateralOutpoint.n));
         obj.push_back(Pair("service", addr.ToString(false)));
-        obj.push_back(Pair("ownerAddress", CBitcoinAddress(keyIDOwner).ToString()));
-        obj.push_back(Pair("votingAddress", CBitcoinAddress(keyIDVoting).ToString()));
+        obj.push_back(Pair("ownerAddress", EncodeDestination(keyIDOwner)));
+        obj.push_back(Pair("votingAddress", EncodeDestination(keyIDVoting)));
 
         CTxDestination dest;
         if (ExtractDestination(scriptPayout, dest)) {
-            CBitcoinAddress bitcoinAddress(dest);
-            obj.push_back(Pair("payoutAddress", bitcoinAddress.ToString()));
+            obj.push_back(Pair("payoutAddress", EncodeDestination(dest)));
         }
         obj.push_back(Pair("pubKeyOperator", pubKeyOperator.ToString()));
         obj.push_back(Pair("operatorReward", (double)nOperatorReward / 100));
@@ -127,8 +126,7 @@ class CProUpServTx
         obj.push_back(Pair("service", addr.ToString(false)));
         CTxDestination dest;
         if (ExtractDestination(scriptOperatorPayout, dest)) {
-            CBitcoinAddress bitcoinAddress(dest);
-            obj.push_back(Pair("operatorPayoutAddress", bitcoinAddress.ToString()));
+            obj.push_back(Pair("operatorPayoutAddress", EncodeDestination(dest)));
         }
         obj.push_back(Pair("inputsHash", inputsHash.ToString()));
     }
@@ -176,11 +174,10 @@ class CProUpRegTx
         obj.setObject();
         obj.push_back(Pair("version", nVersion));
         obj.push_back(Pair("proTxHash", proTxHash.ToString()));
-        obj.push_back(Pair("votingAddress", CBitcoinAddress(keyIDVoting).ToString()));
+        obj.push_back(Pair("votingAddress", EncodeDestination(keyIDVoting)));
         CTxDestination dest;
         if (ExtractDestination(scriptPayout, dest)) {
-            CBitcoinAddress bitcoinAddress(dest);
-            obj.push_back(Pair("payoutAddress", bitcoinAddress.ToString()));
+            obj.push_back(Pair("payoutAddress", EncodeDestination(dest)));
         }
         obj.push_back(Pair("pubKeyOperator", pubKeyOperator.ToString()));
         obj.push_back(Pair("inputsHash", inputsHash.ToString()));