[Backport] Make CTransaction actually immutable

src/budget/budgetmanager.cpp

@@ -1224,23 +1224,23 @@ bool CheckCollateralConfs(const uint256& nTxCollateralHash, int nCurrentHeight,
 
 bool CheckCollateral(const uint256& nTxCollateralHash, const uint256& nExpectedHash, std::string& strError, int64_t& nTime, int nCurrentHeight, bool fBudgetFinalization)
 {
-    CTransaction txCollateral;
+    CTransactionRef txCollateral;
     uint256 nBlockHash;
     if (!GetTransaction(nTxCollateralHash, txCollateral, nBlockHash, true)) {
         strError = strprintf("Can't find collateral tx %s", nTxCollateralHash.ToString());
         return false;
     }
 
-    if (txCollateral.vout.size() < 1) return false;
-    if (txCollateral.nLockTime != 0) return false;
+    if (txCollateral->vout.size() < 1) return false;
+    if (txCollateral->nLockTime != 0) return false;
 
     CScript findScript;
     findScript << OP_RETURN << ToByteVector(nExpectedHash);
 
     bool foundOpReturn = false;
-    for (const CTxOut &o : txCollateral.vout) {
+    for (const CTxOut &o : txCollateral->vout) {
         if (!o.scriptPubKey.IsNormalPaymentScript() && !o.scriptPubKey.IsUnspendable()) {
-            strError = strprintf("Invalid Script %s", txCollateral.ToString());
+            strError = strprintf("Invalid Script %s", txCollateral->ToString());
             return false;
         }
         if (fBudgetFinalization) {
@@ -1267,7 +1267,7 @@ bool CheckCollateral(const uint256& nTxCollateralHash, const uint256& nExpectedH
     }
 
     if (!foundOpReturn) {
-        strError = strprintf("Couldn't find opReturn %s in %s", nExpectedHash.ToString(), txCollateral.ToString());
+        strError = strprintf("Couldn't find opReturn %s in %s", nExpectedHash.ToString(), txCollateral->ToString());
         return false;
     }
 

src/core_io.h

@@ -12,13 +12,14 @@
 class CBlock;
 class CScript;
 class CTransaction;
+struct CMutableTransaction;
 class uint256;
 class UniValue;
 
 // core_read.cpp
 extern CScript ParseScript(std::string s);
 extern std::string ScriptToAsmStr(const CScript& script, const bool fAttemptSighashDecode = false);
-extern bool DecodeHexTx(CTransaction& tx, const std::string& strHexTx);
+extern bool DecodeHexTx(CMutableTransaction& tx, const std::string& strHexTx);
 extern bool DecodeHexBlk(CBlock&, const std::string& strHexBlk);
 extern uint256 ParseHashUV(const UniValue& v, const std::string& strName);
 extern uint256 ParseHashStr(const std::string&, const std::string& strName);

src/core_read.cpp

@@ -74,7 +74,7 @@ CScript ParseScript(std::string s)
     return result;
 }
 
-bool DecodeHexTx(CTransaction& tx, const std::string& strHexTx)
+bool DecodeHexTx(CMutableTransaction& tx, const std::string& strHexTx)
 {
     if (!IsHex(strHexTx))
         return false;

src/masternode.cpp

@@ -191,10 +191,10 @@ bool CMasternode::IsInputAssociatedWithPubkey() const
     CScript payee;
     payee = GetScriptForDestination(pubKeyCollateralAddress.GetID());
 
-    CTransaction txVin;
+    CTransactionRef txVin;
     uint256 hash;
     if(GetTransaction(vin.prevout.hash, txVin, hash, true)) {
-        for (CTxOut out : txVin.vout) {
+        for (CTxOut out : txVin->vout) {
             if (out.nValue == 10000 * COIN && out.scriptPubKey == payee) return true;
         }
     }
@@ -517,7 +517,7 @@ bool CMasternodeBroadcast::CheckInputsAndAdd(int& nDoS)
     // verify that sig time is legit in past
     // should be at least not earlier than block when 1000 PIV tx got MASTERNODE_MIN_CONFIRMATIONS
     uint256 hashBlock = UINT256_ZERO;
-    CTransaction tx2;
+    CTransactionRef tx2;
     GetTransaction(vin.prevout.hash, tx2, hashBlock, true);
     BlockMap::iterator mi = mapBlockIndex.find(hashBlock);
     if (mi != mapBlockIndex.end() && (*mi).second) {

src/net_processing.cpp

@@ -1395,9 +1395,8 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
     else if (strCommand == NetMsgType::TX) {
         std::vector<uint256> vWorkQueue;
         std::vector<uint256> vEraseQueue;
-        CTransactionRef ptx;
-        vRecv >> ptx;
-        const CTransaction& tx = *ptx;
+        CTransaction tx(deserialize, vRecv);
+        CTransactionRef ptx = MakeTransactionRef(tx);
 
         CInv inv(MSG_TX, tx.GetHash());
         pfrom->AddInventoryKnown(inv);

src/pivx-tx.cpp

@@ -592,7 +592,7 @@ static int CommandLineRawTx(int argc, char* argv[])
             argv++;
         }
 
-        CTransaction txDecodeTmp;
+        CMutableTransaction tx;
         int startArg;
 
         if (!fCreateBlank) {
@@ -605,15 +605,13 @@ static int CommandLineRawTx(int argc, char* argv[])
             if (strHexTx == "-") // "-" implies standard input
                 strHexTx = readStdin();
 
-            if (!DecodeHexTx(txDecodeTmp, strHexTx))
+            if (!DecodeHexTx(tx, strHexTx))
                 throw std::runtime_error("invalid transaction encoding");
 
             startArg = 2;
         } else
             startArg = 1;
 
-        CMutableTransaction tx(txDecodeTmp);
-
         for (int i = startArg; i < argc; i++) {
             std::string arg = argv[i];
             std::string key, value;

src/primitives/transaction.cpp

@@ -121,36 +121,20 @@ uint256 CMutableTransaction::GetHash() const
     return SerializeHash(*this);
 }
 
-void CTransaction::UpdateHash() const
+uint256 CTransaction::ComputeHash() const
 {
-    *const_cast<uint256*>(&hash) = SerializeHash(*this);
+    return SerializeHash(*this);
 }
 
 size_t CTransaction::DynamicMemoryUsage() const
 {
     return memusage::RecursiveDynamicUsage(vin) + memusage::RecursiveDynamicUsage(vout);
 }
 
-CTransaction::CTransaction() : nVersion(CTransaction::CURRENT_VERSION), nType(TxType::NORMAL), vin(), vout(), nLockTime(0) { }
-
-CTransaction::CTransaction(const CMutableTransaction &tx) : nVersion(tx.nVersion), nType(tx.nType), vin(tx.vin), vout(tx.vout), nLockTime(tx.nLockTime), sapData(tx.sapData), extraPayload(tx.extraPayload) {
-    UpdateHash();
-}
-
-CTransaction::CTransaction(CMutableTransaction &&tx) : nVersion(tx.nVersion), nType(tx.nType), vin(std::move(tx.vin)), vout(std::move(tx.vout)), nLockTime(tx.nLockTime), sapData(tx.sapData), extraPayload(tx.extraPayload) {
-    UpdateHash();
-}
-
-CTransaction& CTransaction::operator=(const CTransaction &tx) {
-    *const_cast<int16_t*>(&nVersion) = tx.nVersion;
-    *const_cast<int16_t*>(&nType) = tx.nType;
-    *const_cast<std::vector<CTxIn>*>(&vin) = tx.vin;
-    *const_cast<std::vector<CTxOut>*>(&vout) = tx.vout;
-    *const_cast<unsigned int*>(&nLockTime) = tx.nLockTime;
-    *const_cast<uint256*>(&hash) = tx.hash;
-    *const_cast<Optional<SaplingTxData>*>(&sapData) = tx.sapData;
-    return *this;
-}
+/* For backward compatibility, the hash is initialized to 0. TODO: remove the need for this default constructor entirely. */
+CTransaction::CTransaction() : nVersion(CTransaction::CURRENT_VERSION), nType(TxType::NORMAL), vin(), vout(), nLockTime(0), hash() {}
+CTransaction::CTransaction(const CMutableTransaction &tx) : nVersion(tx.nVersion), nType(tx.nType), vin(tx.vin), vout(tx.vout), nLockTime(tx.nLockTime), sapData(tx.sapData), extraPayload(tx.extraPayload), hash(ComputeHash()) {}
+CTransaction::CTransaction(CMutableTransaction &&tx) : nVersion(tx.nVersion), nType(tx.nType), vin(std::move(tx.vin)), vout(std::move(tx.vout)), nLockTime(tx.nLockTime), sapData(tx.sapData), extraPayload(tx.extraPayload), hash(ComputeHash()) {}
 
 bool CTransaction::HasZerocoinSpendInputs() const
 {

src/primitives/transaction.h

@@ -221,18 +221,35 @@ struct CMutableTransaction;
  * - Optional<SaplingTxData> sapData
  * - Optional<std::vector<uint8_t>> extraPayload
  */
-template<typename Stream, typename Operation, typename TxType>
-inline void SerializeTransaction(TxType& tx, Stream& s, Operation ser_action) {
-    READWRITE(*const_cast<int16_t*>(&tx.nVersion));
-    READWRITE(*const_cast<int16_t*>(&tx.nType));
-    READWRITE(*const_cast<std::vector<CTxIn>*>(&tx.vin));
-    READWRITE(*const_cast<std::vector<CTxOut>*>(&tx.vout));
-    READWRITE(*const_cast<uint32_t*>(&tx.nLockTime));
+template<typename Stream, typename TxType>
+inline void UnserializeTransaction(TxType& tx, Stream& s) {
+    tx.vin.clear();
+    tx.vout.clear();
+
+    s >> tx.nVersion;
+    s >> tx.nType;
+    s >> tx.vin;
+    s >> tx.vout;
+    s >> tx.nLockTime;
+    if (tx.isSaplingVersion()) {
+        s >> tx.sapData;
+        if (!tx.IsNormalType()) {
+            s >> tx.extraPayload;
+        }
+    }
+}
 
+template<typename Stream, typename TxType>
+inline void SerializeTransaction(const TxType& tx, Stream& s) {
+    s << tx.nVersion;
+    s << tx.nType;
+    s << tx.vin;
+    s << tx.vout;
+    s << tx.nLockTime;
     if (tx.isSaplingVersion()) {
-        READWRITE(*const_cast<Optional<SaplingTxData>*>(&tx.sapData));
+        s << tx.sapData;
         if (!tx.IsNormalType()) {
-            READWRITE(*const_cast<Optional<std::vector<uint8_t>>*>(&tx.extraPayload));
+            s << tx.extraPayload;
         }
     }
 }
@@ -242,11 +259,6 @@ inline void SerializeTransaction(TxType& tx, Stream& s, Operation ser_action) {
  */
 class CTransaction
 {
-private:
-    /** Memory only. */
-    const uint256 hash;
-    void UpdateHash() const;
-
 public:
     /** Transaction Versions */
     enum TxVersion: int16_t {
@@ -283,18 +295,14 @@ class CTransaction
     CTransaction(CMutableTransaction &&tx);
 
     CTransaction(const CTransaction& tx) = default;
-    CTransaction& operator=(const CTransaction& tx);
-
-    ADD_SERIALIZE_METHODS;
 
-    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action) {
-        SerializeTransaction(*this, s, ser_action);
-        if (ser_action.ForRead()) {
-            UpdateHash();
-        }
+    template <typename Stream>
+    inline void Serialize(Stream& s) const {
+        SerializeTransaction(*this, s);
     }
 
+    /** This deserializing constructor is provided instead of an Unserialize method.
+      *  Unserialize is not possible, since it would require overwriting const fields. */
     template <typename Stream>
     CTransaction(deserialize_type, Stream& s) : CTransaction(CMutableTransaction(deserialize, s)) {}
 
@@ -400,6 +408,11 @@ class CTransaction
     std::string ToString() const;
 
     size_t DynamicMemoryUsage() const;
+
+private:
+    /** Memory only. */
+    const uint256 hash;
+    uint256 ComputeHash() const;
 };
 
 /** A mutable version of CTransaction. */
@@ -416,11 +429,14 @@ struct CMutableTransaction
     CMutableTransaction();
     CMutableTransaction(const CTransaction& tx);
 
-    ADD_SERIALIZE_METHODS;
+    template <typename Stream>
+    inline void Serialize(Stream& s) const {
+        SerializeTransaction(*this, s);
+    }
 
-    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action) {
-        SerializeTransaction(*this, s, ser_action);
+    template <typename Stream>
+    inline void Unserialize(Stream& s) {
+        UnserializeTransaction(*this, s);
     }
 
     template <typename Stream>

src/rest.cpp

@@ -357,7 +357,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
     if (!ParseHashStr(hashStr, hash))
         return RESTERR(req, HTTP_BAD_REQUEST, "Invalid hash: " + hashStr);
 
-    CTransaction tx;
+    CTransactionRef tx;
     uint256 hashBlock = uint256();
     if (!GetTransaction(hash, tx, hashBlock, true))
         return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
@@ -382,7 +382,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
 
     case RF_JSON: {
         UniValue objTx(UniValue::VOBJ);
-        TxToJSON(tx, hashBlock, objTx);
+        TxToJSON(*tx, hashBlock, objTx);
         std::string strJSON = objTx.write() + "\n";
         req->WriteHeader("Content-Type", "application/json");
         req->WriteReply(HTTP_OK, strJSON);

src/rpc/blockchain.cpp

@@ -1351,11 +1351,11 @@ UniValue getblockindexstats(const JSONRPCRequest& request) {
             // Transparent inputs
             for (unsigned int j = 0; j < tx.vin.size(); j++) {
                 COutPoint prevout = tx.vin[j].prevout;
-                CTransaction txPrev;
+                CTransactionRef txPrev;
                 uint256 hashBlock;
                 if(!GetTransaction(prevout.hash, txPrev, hashBlock, true))
                     throw JSONRPCError(RPC_DATABASE_ERROR, "failed to read tx from disk");
-                nValueIn += txPrev.vout[prevout.n].nValue;
+                nValueIn += txPrev->vout[prevout.n].nValue;
             }
             // Shield inputs
             nValueIn += tx.GetShieldedValueIn();