Merge bitcoin#10368: [wallet] Remove helper conversion operator from wallet

laanwj · PastaPastaPasta · commit 354f57a838ad · 2020-01-22T09:11:34.000-06:00
5a5e4e9 [wallet] Remove CTransaction&() helper conversion operator from wallet implementation. (Karl-Johan Alm) Pull request description: The `CTransaction&()` operator in `CMerkleTx` makes conversion into `CTransaction`s transparent, but was marked as to-be-removed in favor of explicitly getting the `tx` ivar, presumably as the operator can lead to ambiguous behavior and makes the code harder to follow. This PR removes the operator and adapts callers. This includes some cases of `static_cast<CTransaction>(wtx)` → `*wtx.tx`, which is definitely an improvement. Tree-SHA512: 95856fec7194d6a79615ea1c322abfcd6bcedf6ffd0cfa89bbdd332ce13035fa52dd4b828d20df673072dde1be64b79c513529a6f422dd5f0961ce722a32d56a
diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp
@@ -25,7 +25,7 @@
 QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx)
 {
     AssertLockHeld(cs_main);
-    if (!CheckFinalTx(wtx))
+    if (!CheckFinalTx(*wtx.tx))
     {
         if (wtx.tx->nLockTime < LOCKTIME_THRESHOLD)
             return tr("Open for %n more block(s)", "", wtx.tx->nLockTime - chainActive.Height());
diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp
@@ -285,7 +285,7 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx, int chainLockHeight)
         status.label = QString::fromStdString(addrBookIt->second.name);
     }
 
-    if (!CheckFinalTx(wtx))
+    if (!CheckFinalTx(*wtx.tx))
     {
         if (wtx.tx->nLockTime < LOCKTIME_THRESHOLD)
         {
diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
@@ -244,7 +244,7 @@ class TransactionTablePriv
         std::map<uint256, CWalletTx>::iterator mi = wallet->mapWallet.find(rec->hash);
         if(mi != wallet->mapWallet.end())
         {
-            std::string strHex = EncodeHexTx(static_cast<CTransaction>(mi->second));
+            std::string strHex = EncodeHexTx(*mi->second.tx);
             return QString::fromStdString(strHex);
         }
         return QString();
diff --git a/src/qt/walletmodeltransaction.cpp b/src/qt/walletmodeltransaction.cpp
@@ -33,7 +33,7 @@ CWalletTx *WalletModelTransaction::getTransaction() const
 
 unsigned int WalletModelTransaction::getTransactionSize()
 {
-    return (!walletTransaction ? 0 : (::GetSerializeSize(walletTransaction->tx, SER_NETWORK, PROTOCOL_VERSION)));
+    return (!walletTransaction ? 0 : (::GetSerializeSize(*walletTransaction->tx, SER_NETWORK, PROTOCOL_VERSION)));
 }
 
 CAmount WalletModelTransaction::getTransactionFee() const
diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp
@@ -341,7 +341,7 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
 
     LOCK2(cs_main, pwallet->cs_wallet);
 
-    if (pwallet->IsMine(wtx)) {
+    if (pwallet->IsMine(*wtx.tx)) {
         pwallet->AddToWallet(wtx, false);
         return NullUniValue;
     }
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
@@ -1997,7 +1997,7 @@ UniValue gettransaction(const JSONRPCRequest& request)
     ListTransactions(pwallet, wtx, "*", 0, false, details, filter);
     entry.push_back(Pair("details", details));
 
-    std::string strHex = EncodeHexTx(static_cast<CTransaction>(wtx));
+    std::string strHex = EncodeHexTx(*wtx.tx);
     entry.push_back(Pair("hex", strHex));
 
     return entry;
diff --git a/src/wallet/test/accounting_tests.cpp b/src/wallet/test/accounting_tests.cpp
@@ -83,7 +83,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)
 
     wtx.mapValue["comment"] = "y";
     {
-        CMutableTransaction tx(wtx);
+        CMutableTransaction tx(*wtx.tx);
         --tx.nLockTime;  // Just to change the hash :)
         wtx.SetTx(MakeTransactionRef(std::move(tx)));
     }
@@ -93,7 +93,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)
 
     wtx.mapValue["comment"] = "x";
     {
-        CMutableTransaction tx(wtx);
+        CMutableTransaction tx(*wtx.tx);
         --tx.nLockTime;  // Just to change the hash :)
         wtx.SetTx(MakeTransactionRef(std::move(tx)));
     }
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -1493,14 +1493,15 @@ int CWallet::GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRo
     uint256 hash = outpoint.hash;
     unsigned int nout = outpoint.n;
 
+    // TODO wtx should refer to a CWalletTx object, not a pointer, based on surrounding code
     const CWalletTx* wtx = GetWalletTx(hash);
     if(wtx != nullptr)
     {
         std::map<uint256, CMutableTransaction>::const_iterator mdwi = mDenomWtxes.find(hash);
         if (mdwi == mDenomWtxes.end()) {
             // not known yet, let's add it
             LogPrint(BCLog::PRIVATESEND, "GetRealOutpointPrivateSendRounds INSERTING %s\n", hash.ToString());
-            mDenomWtxes[hash] = CMutableTransaction(*wtx);
+            mDenomWtxes[hash] = CMutableTransaction(*wtx->tx);
         } else if(mDenomWtxes[hash].vout[nout].nRounds != -10) {
             // found and it's not an initial value, just return it
             return mDenomWtxes[hash].vout[nout].nRounds;
@@ -2043,7 +2044,7 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman)
             LogPrintf("Relaying wtx %s\n", hash.ToString());
 
             if (connman) {
-                connman->RelayTransaction((CTransaction)*this);
+                connman->RelayTransaction(*tx);
                 return true;
             }
         }
@@ -2075,7 +2076,7 @@ CAmount CWalletTx::GetDebit(const isminefilter& filter) const
             debit += nDebitCached;
         else
         {
-            nDebitCached = pwallet->GetDebit(*this, ISMINE_SPENDABLE);
+            nDebitCached = pwallet->GetDebit(*tx, ISMINE_SPENDABLE);
             fDebitCached = true;
             debit += nDebitCached;
         }
@@ -2086,7 +2087,7 @@ CAmount CWalletTx::GetDebit(const isminefilter& filter) const
             debit += nWatchDebitCached;
         else
         {
-            nWatchDebitCached = pwallet->GetDebit(*this, ISMINE_WATCH_ONLY);
+            nWatchDebitCached = pwallet->GetDebit(*tx, ISMINE_WATCH_ONLY);
             fWatchDebitCached = true;
             debit += nWatchDebitCached;
         }
@@ -2108,7 +2109,7 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter) const
             credit += nCreditCached;
         else
         {
-            nCreditCached = pwallet->GetCredit(*this, ISMINE_SPENDABLE);
+            nCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);
             fCreditCached = true;
             credit += nCreditCached;
         }
@@ -2119,7 +2120,7 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter) const
             credit += nWatchCreditCached;
         else
         {
-            nWatchCreditCached = pwallet->GetCredit(*this, ISMINE_WATCH_ONLY);
+            nWatchCreditCached = pwallet->GetCredit(*tx, ISMINE_WATCH_ONLY);
             fWatchCreditCached = true;
             credit += nWatchCreditCached;
         }
@@ -2133,7 +2134,7 @@ CAmount CWalletTx::GetImmatureCredit(bool fUseCache) const
     {
         if (fUseCache && fImmatureCreditCached)
             return nImmatureCreditCached;
-        nImmatureCreditCached = pwallet->GetCredit(*this, ISMINE_SPENDABLE);
+        nImmatureCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);
         fImmatureCreditCached = true;
         return nImmatureCreditCached;
     }
@@ -2177,7 +2178,7 @@ CAmount CWalletTx::GetImmatureWatchOnlyCredit(const bool& fUseCache) const
     {
         if (fUseCache && fImmatureWatchCreditCached)
             return nImmatureWatchCreditCached;
-        nImmatureWatchCreditCached = pwallet->GetCredit(*this, ISMINE_WATCH_ONLY);
+        nImmatureWatchCreditCached = pwallet->GetCredit(*tx, ISMINE_WATCH_ONLY);
         fImmatureWatchCreditCached = true;
         return nImmatureWatchCreditCached;
     }
@@ -2297,7 +2298,7 @@ CAmount CWalletTx::GetChange() const
 {
     if (fChangeCached)
         return nChangeCached;
-    nChangeCached = pwallet->GetChange(*this);
+    nChangeCached = pwallet->GetChange(*tx);
     fChangeCached = true;
     return nChangeCached;
 }
@@ -2311,7 +2312,7 @@ bool CWalletTx::InMempool() const
 bool CWalletTx::IsTrusted() const
 {
     // Quick answer in most cases
-    if (!CheckFinalTx(*this))
+    if (!CheckFinalTx(*tx))
         return false;
     int nDepth = GetDepthInMainChain();
     if (nDepth >= 1)
@@ -2677,7 +2678,7 @@ void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const
         for (auto pcoin : GetSpendableTXs()) {
             const uint256& wtxid = pcoin->GetHash();
 
-            if (!CheckFinalTx(*pcoin))
+            if (!CheckFinalTx(*pcoin->tx))
                 continue;
 
             if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -229,10 +229,6 @@ class CMerkleTx
         Init();
     }
 
-    /** Helper conversion operator to allow passing CMerkleTx where CTransaction is expected.
-     *  TODO: adapt callers and remove this operator. */
-    operator const CTransaction&() const { return *tx; }
-
     void Init()
     {
         hashBlock = uint256();
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
@@ -275,7 +275,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
             CWalletTx wtx;
             ssValue >> wtx;
             CValidationState state;
-            if (!(CheckTransaction(wtx, state) && (wtx.GetHash() == hash) && state.IsValid()))
+            if (!(CheckTransaction(*wtx.tx, state) && (wtx.GetHash() == hash) && state.IsValid()))
                 return false;
 
             // Undo serialize changes in 31600

Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,7 @@`
`25`	`25`	`QString TransactionDesc::FormatTxStatus(const CWalletTx& wtx)`
`26`	`26`	`{`
`27`	`27`	`AssertLockHeld(cs_main);`
`28`		`- if (!CheckFinalTx(wtx))`
	`28`	`+ if (!CheckFinalTx(*wtx.tx))`
`29`	`29`	`{`
`30`	`30`	`if (wtx.tx->nLockTime < LOCKTIME_THRESHOLD)`
`31`	`31`	`return tr("Open for %n more block(s)", "", wtx.tx->nLockTime - chainActive.Height());`
Original file line number	Diff line number	Diff line change
`@@ -285,7 +285,7 @@ void TransactionRecord::updateStatus(const CWalletTx &wtx, int chainLockHeight)`
`285`	`285`	`status.label = QString::fromStdString(addrBookIt->second.name);`
`286`	`286`	`}`
`287`	`287`
`288`		`- if (!CheckFinalTx(wtx))`
	`288`	`+ if (!CheckFinalTx(*wtx.tx))`
`289`	`289`	`{`
`290`	`290`	`if (wtx.tx->nLockTime < LOCKTIME_THRESHOLD)`
`291`	`291`	`{`
Original file line number	Diff line number	Diff line change
`@@ -244,7 +244,7 @@ class TransactionTablePriv`
`244`	`244`	`std::map<uint256, CWalletTx>::iterator mi = wallet->mapWallet.find(rec->hash);`
`245`	`245`	`if(mi != wallet->mapWallet.end())`
`246`	`246`	`{`
`247`		`- std::string strHex = EncodeHexTx(static_cast<CTransaction>(mi->second));`
	`247`	`+ std::string strHex = EncodeHexTx(*mi->second.tx);`
`248`	`248`	`return QString::fromStdString(strHex);`
`249`	`249`	`}`
`250`	`250`	`return QString();`
Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ CWalletTx *WalletModelTransaction::getTransaction() const`
`33`	`33`
`34`	`34`	`unsigned int WalletModelTransaction::getTransactionSize()`
`35`	`35`	`{`
`36`		`- return (!walletTransaction ? 0 : (::GetSerializeSize(walletTransaction->tx, SER_NETWORK, PROTOCOL_VERSION)));`
	`36`	`+ return (!walletTransaction ? 0 : (::GetSerializeSize(*walletTransaction->tx, SER_NETWORK, PROTOCOL_VERSION)));`
`37`	`37`	`}`
`38`	`38`
`39`	`39`	`CAmount WalletModelTransaction::getTransactionFee() const`
Original file line number	Diff line number	Diff line change
`@@ -341,7 +341,7 @@ UniValue importprunedfunds(const JSONRPCRequest& request)`
`341`	`341`
`342`	`342`	`LOCK2(cs_main, pwallet->cs_wallet);`
`343`	`343`
`344`		`- if (pwallet->IsMine(wtx)) {`
	`344`	`+ if (pwallet->IsMine(*wtx.tx)) {`
`345`	`345`	`pwallet->AddToWallet(wtx, false);`
`346`	`346`	`return NullUniValue;`
`347`	`347`	`}`
Original file line number	Diff line number	Diff line change
`@@ -83,7 +83,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)`
`83`	`83`
`84`	`84`	`wtx.mapValue["comment"] = "y";`
`85`	`85`	`{`
`86`		`- CMutableTransaction tx(wtx);`
	`86`	`+ CMutableTransaction tx(*wtx.tx);`
`87`	`87`	`--tx.nLockTime; // Just to change the hash :)`
`88`	`88`	`wtx.SetTx(MakeTransactionRef(std::move(tx)));`
`89`	`89`	`}`
`@@ -93,7 +93,7 @@ BOOST_AUTO_TEST_CASE(acc_orderupgrade)`
`93`	`93`
`94`	`94`	`wtx.mapValue["comment"] = "x";`
`95`	`95`	`{`
`96`		`- CMutableTransaction tx(wtx);`
	`96`	`+ CMutableTransaction tx(*wtx.tx);`
`97`	`97`	`--tx.nLockTime; // Just to change the hash :)`
`98`	`98`	`wtx.SetTx(MakeTransactionRef(std::move(tx)));`
`99`	`99`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1493,14 +1493,15 @@ int CWallet::GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRo`
`1493`	`1493`	`uint256 hash = outpoint.hash;`
`1494`	`1494`	`unsigned int nout = outpoint.n;`
`1495`	`1495`
	`1496`	`+ // TODO wtx should refer to a CWalletTx object, not a pointer, based on surrounding code`
`1496`	`1497`	`const CWalletTx* wtx = GetWalletTx(hash);`
`1497`	`1498`	`if(wtx != nullptr)`
`1498`	`1499`	`{`
`1499`	`1500`	`std::map<uint256, CMutableTransaction>::const_iterator mdwi = mDenomWtxes.find(hash);`
`1500`	`1501`	`if (mdwi == mDenomWtxes.end()) {`
`1501`	`1502`	`// not known yet, let's add it`
`1502`	`1503`	`LogPrint(BCLog::PRIVATESEND, "GetRealOutpointPrivateSendRounds INSERTING %s\n", hash.ToString());`
`1503`		`- mDenomWtxes[hash] = CMutableTransaction(*wtx);`
	`1504`	`+ mDenomWtxes[hash] = CMutableTransaction(*wtx->tx);`
`1504`	`1505`	`} else if(mDenomWtxes[hash].vout[nout].nRounds != -10) {`
`1505`	`1506`	`// found and it's not an initial value, just return it`
`1506`	`1507`	`return mDenomWtxes[hash].vout[nout].nRounds;`
`@@ -2043,7 +2044,7 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman)`
`2043`	`2044`	`LogPrintf("Relaying wtx %s\n", hash.ToString());`
`2044`	`2045`
`2045`	`2046`	`if (connman) {`
`2046`		`- connman->RelayTransaction((CTransaction)*this);`
	`2047`	`+ connman->RelayTransaction(*tx);`
`2047`	`2048`	`return true;`
`2048`	`2049`	`}`
`2049`	`2050`	`}`
`@@ -2075,7 +2076,7 @@ CAmount CWalletTx::GetDebit(const isminefilter& filter) const`
`2075`	`2076`	`debit += nDebitCached;`
`2076`	`2077`	`else`
`2077`	`2078`	`{`
`2078`		`- nDebitCached = pwallet->GetDebit(*this, ISMINE_SPENDABLE);`
	`2079`	`+ nDebitCached = pwallet->GetDebit(*tx, ISMINE_SPENDABLE);`
`2079`	`2080`	`fDebitCached = true;`
`2080`	`2081`	`debit += nDebitCached;`
`2081`	`2082`	`}`
`@@ -2086,7 +2087,7 @@ CAmount CWalletTx::GetDebit(const isminefilter& filter) const`
`2086`	`2087`	`debit += nWatchDebitCached;`
`2087`	`2088`	`else`
`2088`	`2089`	`{`
`2089`		`- nWatchDebitCached = pwallet->GetDebit(*this, ISMINE_WATCH_ONLY);`
	`2090`	`+ nWatchDebitCached = pwallet->GetDebit(*tx, ISMINE_WATCH_ONLY);`
`2090`	`2091`	`fWatchDebitCached = true;`
`2091`	`2092`	`debit += nWatchDebitCached;`
`2092`	`2093`	`}`
`@@ -2108,7 +2109,7 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter) const`
`2108`	`2109`	`credit += nCreditCached;`
`2109`	`2110`	`else`
`2110`	`2111`	`{`
`2111`		`- nCreditCached = pwallet->GetCredit(*this, ISMINE_SPENDABLE);`
	`2112`	`+ nCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);`
`2112`	`2113`	`fCreditCached = true;`
`2113`	`2114`	`credit += nCreditCached;`
`2114`	`2115`	`}`
`@@ -2119,7 +2120,7 @@ CAmount CWalletTx::GetCredit(const isminefilter& filter) const`
`2119`	`2120`	`credit += nWatchCreditCached;`
`2120`	`2121`	`else`
`2121`	`2122`	`{`
`2122`		`- nWatchCreditCached = pwallet->GetCredit(*this, ISMINE_WATCH_ONLY);`
	`2123`	`+ nWatchCreditCached = pwallet->GetCredit(*tx, ISMINE_WATCH_ONLY);`
`2123`	`2124`	`fWatchCreditCached = true;`
`2124`	`2125`	`credit += nWatchCreditCached;`
`2125`	`2126`	`}`
`@@ -2133,7 +2134,7 @@ CAmount CWalletTx::GetImmatureCredit(bool fUseCache) const`
`2133`	`2134`	`{`
`2134`	`2135`	`if (fUseCache && fImmatureCreditCached)`
`2135`	`2136`	`return nImmatureCreditCached;`
`2136`		`- nImmatureCreditCached = pwallet->GetCredit(*this, ISMINE_SPENDABLE);`
	`2137`	`+ nImmatureCreditCached = pwallet->GetCredit(*tx, ISMINE_SPENDABLE);`
`2137`	`2138`	`fImmatureCreditCached = true;`
`2138`	`2139`	`return nImmatureCreditCached;`
`2139`	`2140`	`}`
`@@ -2177,7 +2178,7 @@ CAmount CWalletTx::GetImmatureWatchOnlyCredit(const bool& fUseCache) const`
`2177`	`2178`	`{`
`2178`	`2179`	`if (fUseCache && fImmatureWatchCreditCached)`
`2179`	`2180`	`return nImmatureWatchCreditCached;`
`2180`		`- nImmatureWatchCreditCached = pwallet->GetCredit(*this, ISMINE_WATCH_ONLY);`
	`2181`	`+ nImmatureWatchCreditCached = pwallet->GetCredit(*tx, ISMINE_WATCH_ONLY);`
`2181`	`2182`	`fImmatureWatchCreditCached = true;`
`2182`	`2183`	`return nImmatureWatchCreditCached;`
`2183`	`2184`	`}`
`@@ -2297,7 +2298,7 @@ CAmount CWalletTx::GetChange() const`
`2297`	`2298`	`{`
`2298`	`2299`	`if (fChangeCached)`
`2299`	`2300`	`return nChangeCached;`
`2300`		`- nChangeCached = pwallet->GetChange(*this);`
	`2301`	`+ nChangeCached = pwallet->GetChange(*tx);`
`2301`	`2302`	`fChangeCached = true;`
`2302`	`2303`	`return nChangeCached;`
`2303`	`2304`	`}`
`@@ -2311,7 +2312,7 @@ bool CWalletTx::InMempool() const`
`2311`	`2312`	`bool CWalletTx::IsTrusted() const`
`2312`	`2313`	`{`
`2313`	`2314`	`// Quick answer in most cases`
`2314`		`- if (!CheckFinalTx(*this))`
	`2315`	`+ if (!CheckFinalTx(*tx))`
`2315`	`2316`	`return false;`
`2316`	`2317`	`int nDepth = GetDepthInMainChain();`
`2317`	`2318`	`if (nDepth >= 1)`
`@@ -2677,7 +2678,7 @@ void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const`
`2677`	`2678`	`for (auto pcoin : GetSpendableTXs()) {`
`2678`	`2679`	`const uint256& wtxid = pcoin->GetHash();`
`2679`	`2680`
`2680`		`- if (!CheckFinalTx(*pcoin))`
	`2681`	`+ if (!CheckFinalTx(*pcoin->tx))`
`2681`	`2682`	`continue;`
`2682`	`2683`
`2683`	`2684`	`if (pcoin->IsCoinBase() && pcoin->GetBlocksToMaturity() > 0)`
Original file line number	Diff line number	Diff line change
`@@ -229,10 +229,6 @@ class CMerkleTx`
`229`	`229`	`Init();`
`230`	`230`	`}`
`231`	`231`
`232`		`- /** Helper conversion operator to allow passing CMerkleTx where CTransaction is expected.`
`233`		`- * TODO: adapt callers and remove this operator. */`
`234`		`- operator const CTransaction&() const { return *tx; }`
`235`		`-`
`236`	`232`	`void Init()`
`237`	`233`	`{`
`238`	`234`	`hashBlock = uint256();`