From 41505e64aa358134536f095fed32aa7c3728088c Mon Sep 17 00:00:00 2001 From: fanquake Date: Sun, 13 Dec 2020 09:50:03 +0800 Subject: [PATCH] Merge #20588: Remove unused and confusing CTransaction constructor fac39c198324715565897f4240709340477af0bf wallet: document that tx in CreateTransaction is purely an out-param (MarcoFalke) faac31521bb7ecbf999541cf918d3750ff589de4 Remove unused and confusing CTransaction constructor (MarcoFalke) Pull request description: The constructor is confusing and dangerous (as explained in the TODO), fix that by removing it. ACKs for top commit: laanwj: Code review ACK fac39c198324715565897f4240709340477af0bf promag: Code review ACK fac39c198324715565897f4240709340477af0bf. theStack: Code review ACK fac39c198324715565897f4240709340477af0bf Tree-SHA512: e0c8cffce8d8ee0166b8e1cbfe85ed0657611e26e2af0d69fde70eceaa5d75cbde3eb489af0428fe4fc431360b4c791fb1cc21b8dee7d4c7a4f17df00836229d --- src/bench/duplicate_inputs.cpp | 2 +- src/blockencodings.cpp | 2 +- src/coinjoin/coinjoin.h | 5 ++--- src/primitives/transaction.cpp | 2 -- src/primitives/transaction.h | 8 ++------ src/test/fuzz/script_sigcache.cpp | 2 +- src/test/fuzz/transaction.cpp | 2 +- src/wallet/test/coinjoin_tests.cpp | 2 +- src/wallet/wallet.cpp | 3 ++- 9 files changed, 11 insertions(+), 17 deletions(-) diff --git a/src/bench/duplicate_inputs.cpp b/src/bench/duplicate_inputs.cpp index 19da79cb56..173a2faca9 100644 --- a/src/bench/duplicate_inputs.cpp +++ b/src/bench/duplicate_inputs.cpp @@ -44,7 +44,7 @@ static void DuplicateInputs(benchmark::Bench& bench) naughtyTx.vout[0].nValue = 0; naughtyTx.vout[0].scriptPubKey = SCRIPT_PUB; - uint64_t n_inputs = (((MaxBlockSize() / ::GetSerializeSize(CTransaction(), PROTOCOL_VERSION)) - (CTransaction(coinbaseTx).GetTotalSize() + CTransaction(naughtyTx).GetTotalSize())) / 41) - 100; + uint64_t n_inputs = (((MaxBlockSize() / ::GetSerializeSize(CMutableTransaction(), PROTOCOL_VERSION)) - (CTransaction(coinbaseTx).GetTotalSize() + CTransaction(naughtyTx).GetTotalSize())) / 41) - 100; for (uint64_t x = 0; x < (n_inputs - 1); ++x) { naughtyTx.vin.emplace_back(GetRandHash(), 0, CScript(), 0); } diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index ec169725d8..567d04d54c 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -16,7 +16,7 @@ #include -#define MIN_TRANSACTION_SIZE (::GetSerializeSize(CTransaction(), PROTOCOL_VERSION)) +#define MIN_TRANSACTION_SIZE (::GetSerializeSize(CMutableTransaction(), PROTOCOL_VERSION)) CBlockHeaderAndShortTxIDs::CBlockHeaderAndShortTxIDs(const CBlock& block) : nonce(GetRand(std::numeric_limits::max())), diff --git a/src/coinjoin/coinjoin.h b/src/coinjoin/coinjoin.h index f2214a329b..20bbbd808d 100644 --- a/src/coinjoin/coinjoin.h +++ b/src/coinjoin/coinjoin.h @@ -152,7 +152,7 @@ public: CService addr; CCoinJoinEntry() : - txCollateral(MakeTransactionRef()) + txCollateral(MakeTransactionRef(CMutableTransaction{})) { } @@ -246,9 +246,8 @@ public: uint256 m_protxHash; std::vector vchSig; int64_t sigTime{0}; - CCoinJoinBroadcastTx() : - tx(MakeTransactionRef()) + tx(MakeTransactionRef(CMutableTransaction{})) { } diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 61c488a5f1..feb97c0894 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -92,8 +92,6 @@ uint256 CTransaction::ComputeHash() const return SerializeHash(*this); } -/* For backward compatibility, the hash is initialized to 0. TODO: remove the need for this default constructor entirely. */ -CTransaction::CTransaction() : vin(), vout(), nVersion(CTransaction::CURRENT_VERSION), nType(TRANSACTION_NORMAL), nLockTime(0), hash{} {} CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nType(tx.nType), nLockTime(tx.nLockTime), vExtraPayload(tx.vExtraPayload), hash{ComputeHash()} {} CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), nVersion(tx.nVersion), nType(tx.nType), nLockTime(tx.nLockTime), vExtraPayload(tx.vExtraPayload), hash{ComputeHash()} {} diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 73f2e50e3b..cc0652a40b 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -212,12 +212,9 @@ private: uint256 ComputeHash() const; public: - /** Construct a CTransaction that qualifies as IsNull() */ - CTransaction(); - /** Convert a CMutableTransaction into a CTransaction. */ - explicit CTransaction(const CMutableTransaction &tx); - CTransaction(CMutableTransaction &&tx); + explicit CTransaction(const CMutableTransaction& tx); + CTransaction(CMutableTransaction&& tx); template inline void Serialize(Stream& s) const { @@ -319,7 +316,6 @@ struct CMutableTransaction }; typedef std::shared_ptr CTransactionRef; -static inline CTransactionRef MakeTransactionRef() { return std::make_shared(); } template static inline CTransactionRef MakeTransactionRef(Tx&& txIn) { return std::make_shared(std::forward(txIn)); } /** Implementation of BIP69 diff --git a/src/test/fuzz/script_sigcache.cpp b/src/test/fuzz/script_sigcache.cpp index c2b38be391..9fb76ae793 100644 --- a/src/test/fuzz/script_sigcache.cpp +++ b/src/test/fuzz/script_sigcache.cpp @@ -28,7 +28,7 @@ FUZZ_TARGET_INIT(script_sigcache, initialize_script_sigcache) FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size()); const std::optional mutable_transaction = ConsumeDeserializable(fuzzed_data_provider); - const CTransaction tx = mutable_transaction ? CTransaction{*mutable_transaction} : CTransaction{}; + const CTransaction tx{mutable_transaction ? *mutable_transaction : CMutableTransaction{}}; const unsigned int n_in = fuzzed_data_provider.ConsumeIntegral(); const CAmount amount = ConsumeMoney(fuzzed_data_provider); const bool store = fuzzed_data_provider.ConsumeBool(); diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp index ae9cc9b4b1..3665b465ac 100644 --- a/src/test/fuzz/transaction.cpp +++ b/src/test/fuzz/transaction.cpp @@ -41,7 +41,7 @@ FUZZ_TARGET_INIT(transaction, initialize_transaction) return CTransaction(deserialize, ds); } catch (const std::ios_base::failure&) { valid_tx = false; - return CTransaction(); + return CTransaction{CMutableTransaction{}}; } }(); bool valid_mutable_tx = true; diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index ed9fe3285d..e53dc78eb8 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/src/wallet/test/coinjoin_tests.cpp @@ -173,7 +173,6 @@ public: CompactTallyItem GetTallyItem(const std::vector& vecAmounts) { CompactTallyItem tallyItem; - CTransactionRef tx; ReserveDestination reserveDest(wallet.get()); CAmount nFeeRet; int nChangePosRet = -1; @@ -185,6 +184,7 @@ public: BOOST_CHECK(reserveDest.GetReservedDestination(tallyItem.txdest, false)); } for (CAmount nAmount : vecAmounts) { + CTransactionRef tx; BOOST_CHECK(wallet->CreateTransaction({{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, tx, nFeeRet, nChangePosRet, strError, coinControl)); { LOCK2(wallet->cs_wallet, cs_main); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7e57de0a5f..6be105ba06 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3760,7 +3760,7 @@ bool CWallet::CreateTransaction( int nExtraPayloadSize) { int nChangePosIn = nChangePosInOut; - CTransactionRef tx2 = tx; + Assert(!tx); // tx is an out-param. TODO change the return type from bool to tx (or nullptr) bool res = CreateTransactionInternal(vecSend, tx, nFeeRet, nChangePosInOut, error, coin_control, sign, nExtraPayloadSize); // try with avoidpartialspends unless it's enabled already if (res && nFeeRet > 0 /* 0 means non-functional fee rate estimation */ && m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) { @@ -3774,6 +3774,7 @@ bool CWallet::CreateTransaction( } CAmount nFeeRet2; + CTransactionRef tx2; int nChangePosInOut2 = nChangePosIn; bilingual_str error2; // fired and forgotten; if an error occurs, we discard the results if (CreateTransactionInternal(vecSend, tx2, nFeeRet2, nChangePosInOut2, error2, tmp_cc, sign, nExtraPayloadSize)) {