Skip to content

Commit 6b46288

Browse files
author
MarcoFalke
committed
Merge #12949: tests: Avoid copies of CTransaction
fae58ec tests: Avoid copies of CTransaction (MarcoFalke) Pull request description: Avoid the copy (or move) constructor of `CTransaction` in test code, whereever a simple reference can be used instead. Tree-SHA512: 8ef2077a277d6182996f4671722fdc01a90909ae7431c1e52604aab8ed028910615028caf9b4cb07a9b15fdc04939dea2209cc3189dde7d38271256d9fe1076c
2 parents a63b4e3 + fae58ec commit 6b46288

10 files changed

+39
-35
lines changed

‎src/bench/mempool_eviction.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
#include <list>
1010
#include <vector>
1111

12-
static void AddTx(const CTransaction& tx, const CAmount& nFee, CTxMemPool& pool)
12+
static void AddTx(const CMutableTransaction& tx, const CAmount& nFee, CTxMemPool& pool)
1313
{
1414
int64_t nTime = 0;
1515
unsigned int nHeight = 1;

‎src/bench/verify_script.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ static void VerifyScriptBench(benchmark::State& state)
7171
CScript scriptPubKey = CScript() << witnessversion << ToByteVector(pubkeyHash);
7272
CScript scriptSig;
7373
CScript witScriptPubkey = CScript() << OP_DUP << OP_HASH160 << ToByteVector(pubkeyHash) << OP_EQUALVERIFY << OP_CHECKSIG;
74-
CTransaction txCredit = BuildCreditingTransaction(scriptPubKey);
74+
const CMutableTransaction& txCredit = BuildCreditingTransaction(scriptPubKey);
7575
CMutableTransaction txSpend = BuildSpendingTransaction(scriptSig, txCredit);
7676
CScriptWitness& witness = txSpend.vin[0].scriptWitness;
7777
witness.stack.emplace_back();

‎src/test/blockencodings_tests.cpp

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -52,16 +52,16 @@ static CBlock BuildBlockTestCase() {
5252
}
5353

5454
// Number of shared use_counts we expect for a tx we haven't touched
55-
// == 2 (mempool + our copy from the GetSharedTx call)
56-
#define SHARED_TX_OFFSET 2
55+
// (block + mempool + our copy from the GetSharedTx call)
56+
constexpr long SHARED_TX_OFFSET{3};
5757

5858
BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
5959
{
6060
CTxMemPool pool;
6161
TestMemPoolEntryHelper entry;
6262
CBlock block(BuildBlockTestCase());
6363

64-
pool.addUnchecked(block.vtx[2]->GetHash(), entry.FromTx(*block.vtx[2]));
64+
pool.addUnchecked(block.vtx[2]->GetHash(), entry.FromTx(block.vtx[2]));
6565
LOCK(pool.cs);
6666
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
6767

@@ -161,7 +161,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
161161
TestMemPoolEntryHelper entry;
162162
CBlock block(BuildBlockTestCase());
163163

164-
pool.addUnchecked(block.vtx[2]->GetHash(), entry.FromTx(*block.vtx[2]));
164+
pool.addUnchecked(block.vtx[2]->GetHash(), entry.FromTx(block.vtx[2]));
165165
LOCK(pool.cs);
166166
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
167167

@@ -188,7 +188,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
188188
BOOST_CHECK( partialBlock.IsTxAvailable(1));
189189
BOOST_CHECK( partialBlock.IsTxAvailable(2));
190190

191-
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);
191+
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // +1 because of partialBlock
192192

193193
CBlock block2;
194194
{
@@ -203,6 +203,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
203203
partialBlock.FillBlock(block2, {block.vtx[1]}); // Current implementation doesn't check txn here, but don't require that
204204
partialBlock = tmp;
205205
}
206+
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 2); // +2 because of partialBlock and block2
206207
bool mutated;
207208
BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated));
208209

@@ -213,13 +214,15 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
213214
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString());
214215
BOOST_CHECK(!mutated);
215216

217+
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 3); // +2 because of partialBlock and block2 and block3
218+
216219
txhash = block.vtx[2]->GetHash();
217220
block.vtx.clear();
218221
block2.vtx.clear();
219222
block3.vtx.clear();
220-
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy.
223+
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1 - 1); // + 1 because of partialBlock; -1 because of block.
221224
}
222-
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
225+
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET - 1); // -1 because of block
223226
}
224227

225228
BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
@@ -228,7 +231,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
228231
TestMemPoolEntryHelper entry;
229232
CBlock block(BuildBlockTestCase());
230233

231-
pool.addUnchecked(block.vtx[1]->GetHash(), entry.FromTx(*block.vtx[1]));
234+
pool.addUnchecked(block.vtx[1]->GetHash(), entry.FromTx(block.vtx[1]));
232235
LOCK(pool.cs);
233236
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[1]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
234237

@@ -268,9 +271,9 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
268271
txhash = block.vtx[1]->GetHash();
269272
block.vtx.clear();
270273
block2.vtx.clear();
271-
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy.
274+
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1 - 1); // + 1 because of partialBlock; -1 because of block.
272275
}
273-
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
276+
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET - 1); // -1 because of block
274277
}
275278

276279
BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest)

‎src/test/multisig_tests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
BOOST_FIXTURE_TEST_SUITE(multisig_tests, BasicTestingSetup)
2020

2121
CScript
22-
sign_multisig(CScript scriptPubKey, std::vector<CKey> keys, CTransaction transaction, int whichIn)
22+
sign_multisig(const CScript& scriptPubKey, const std::vector<CKey>& keys, const CTransaction& transaction, int whichIn)
2323
{
2424
uint256 hash = SignatureHash(scriptPubKey, transaction, whichIn, SIGHASH_ALL, 0, SigVersion::BASE);
2525

‎src/test/script_tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1029,7 +1029,7 @@ BOOST_AUTO_TEST_CASE(script_PushData)
10291029
}
10301030

10311031
CScript
1032-
sign_multisig(CScript scriptPubKey, std::vector<CKey> keys, CTransaction transaction)
1032+
sign_multisig(const CScript& scriptPubKey, const std::vector<CKey>& keys, const CTransaction& transaction)
10331033
{
10341034
uint256 hash = SignatureHash(scriptPubKey, transaction, 0, SIGHASH_ALL, 0, SigVersion::BASE);
10351035

@@ -1053,7 +1053,7 @@ sign_multisig(CScript scriptPubKey, std::vector<CKey> keys, CTransaction transac
10531053
return result;
10541054
}
10551055
CScript
1056-
sign_multisig(CScript scriptPubKey, const CKey &key, CTransaction transaction)
1056+
sign_multisig(const CScript& scriptPubKey, const CKey& key, const CTransaction& transaction)
10571057
{
10581058
std::vector<CKey> keys;
10591059
keys.push_back(key);

‎src/test/serialize_tests.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ class CSerializeMethodsTestSingle
2323
CTransactionRef txval;
2424
public:
2525
CSerializeMethodsTestSingle() = default;
26-
CSerializeMethodsTestSingle(int intvalin, bool boolvalin, std::string stringvalin, const char* charstrvalin, CTransaction txvalin) : intval(intvalin), boolval(boolvalin), stringval(std::move(stringvalin)), txval(MakeTransactionRef(txvalin))
26+
CSerializeMethodsTestSingle(int intvalin, bool boolvalin, std::string stringvalin, const char* charstrvalin, const CTransactionRef& txvalin) : intval(intvalin), boolval(boolvalin), stringval(std::move(stringvalin)), txval(txvalin)
2727
{
2828
memcpy(charstrval, charstrvalin, sizeof(charstrval));
2929
}
@@ -350,8 +350,9 @@ BOOST_AUTO_TEST_CASE(class_methods)
350350
std::string stringval("testing");
351351
const char charstrval[16] = "testing charstr";
352352
CMutableTransaction txval;
353-
CSerializeMethodsTestSingle methodtest1(intval, boolval, stringval, charstrval, txval);
354-
CSerializeMethodsTestMany methodtest2(intval, boolval, stringval, charstrval, txval);
353+
CTransactionRef tx_ref{MakeTransactionRef(txval)};
354+
CSerializeMethodsTestSingle methodtest1(intval, boolval, stringval, charstrval, tx_ref);
355+
CSerializeMethodsTestMany methodtest2(intval, boolval, stringval, charstrval, tx_ref);
355356
CSerializeMethodsTestSingle methodtest3;
356357
CSerializeMethodsTestMany methodtest4;
357358
CDataStream ss(SER_DISK, PROTOCOL_VERSION);

‎src/test/test_bitcoin.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ TestChain100Setup::TestChain100Setup() : TestingSetup(CBaseChainParams::REGTEST)
123123
{
124124
std::vector<CMutableTransaction> noTxns;
125125
CBlock b = CreateAndProcessBlock(noTxns, scriptPubKey);
126-
coinbaseTxns.push_back(*b.vtx[0]);
126+
m_coinbase_txns.push_back(b.vtx[0]);
127127
}
128128
}
129129

@@ -164,12 +164,12 @@ TestChain100Setup::~TestChain100Setup()
164164

165165

166166
CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction &tx) {
167-
CTransaction txn(tx);
168-
return FromTx(txn);
167+
return FromTx(MakeTransactionRef(tx));
169168
}
170169

171-
CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransaction &txn) {
172-
return CTxMemPoolEntry(MakeTransactionRef(txn), nFee, nTime, nHeight,
170+
CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx)
171+
{
172+
return CTxMemPoolEntry(tx, nFee, nTime, nHeight,
173173
spendsCoinbase, sigOpCost, lp);
174174
}
175175

‎src/test/test_bitcoin.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ struct TestChain100Setup : public TestingSetup {
8787

8888
~TestChain100Setup();
8989

90-
std::vector<CTransaction> coinbaseTxns; // For convenience, coinbase transactions
90+
std::vector<CTransactionRef> m_coinbase_txns; // For convenience, coinbase transactions
9191
CKey coinbaseKey; // private/public key needed to spend coinbase transactions
9292
};
9393

@@ -107,8 +107,8 @@ struct TestMemPoolEntryHelper
107107
nFee(0), nTime(0), nHeight(1),
108108
spendsCoinbase(false), sigOpCost(4) { }
109109

110-
CTxMemPoolEntry FromTx(const CMutableTransaction &tx);
111-
CTxMemPoolEntry FromTx(const CTransaction &tx);
110+
CTxMemPoolEntry FromTx(const CMutableTransaction& tx);
111+
CTxMemPoolEntry FromTx(const CTransactionRef& tx);
112112

113113
// Change the default value
114114
TestMemPoolEntryHelper &Fee(CAmount _fee) { nFee = _fee; return *this; }

‎src/test/txvalidationcache_tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
4848
{
4949
spends[i].nVersion = 1;
5050
spends[i].vin.resize(1);
51-
spends[i].vin[0].prevout.hash = coinbaseTxns[0].GetHash();
51+
spends[i].vin[0].prevout.hash = m_coinbase_txns[0]->GetHash();
5252
spends[i].vin[0].prevout.n = 0;
5353
spends[i].vout.resize(1);
5454
spends[i].vout[0].nValue = 11*CENT;
@@ -167,7 +167,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
167167

168168
spend_tx.nVersion = 1;
169169
spend_tx.vin.resize(1);
170-
spend_tx.vin[0].prevout.hash = coinbaseTxns[0].GetHash();
170+
spend_tx.vin[0].prevout.hash = m_coinbase_txns[0]->GetHash();
171171
spend_tx.vin[0].prevout.n = 0;
172172
spend_tx.vout.resize(4);
173173
spend_tx.vout[0].nValue = 11*CENT;

‎src/wallet/test/wallet_tests.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -119,14 +119,14 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
119119
// will pick up both blocks, not just the first.
120120
const int64_t BLOCK_TIME = chainActive.Tip()->GetBlockTimeMax() + 5;
121121
SetMockTime(BLOCK_TIME);
122-
coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
123-
coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
122+
m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
123+
m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
124124

125125
// Set key birthday to block time increased by the timestamp window, so
126126
// rescan will start at the block time.
127127
const int64_t KEY_TIME = BLOCK_TIME + TIMESTAMP_WINDOW;
128128
SetMockTime(KEY_TIME);
129-
coinbaseTxns.emplace_back(*CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
129+
m_coinbase_txns.emplace_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
130130

131131
LOCK(cs_main);
132132

@@ -157,9 +157,9 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
157157

158158
LOCK(wallet.cs_wallet);
159159
BOOST_CHECK_EQUAL(wallet.mapWallet.size(), 3U);
160-
BOOST_CHECK_EQUAL(coinbaseTxns.size(), 103U);
161-
for (size_t i = 0; i < coinbaseTxns.size(); ++i) {
162-
bool found = wallet.GetWalletTx(coinbaseTxns[i].GetHash());
160+
BOOST_CHECK_EQUAL(m_coinbase_txns.size(), 103U);
161+
for (size_t i = 0; i < m_coinbase_txns.size(); ++i) {
162+
bool found = wallet.GetWalletTx(m_coinbase_txns[i]->GetHash());
163163
bool expected = i >= 100;
164164
BOOST_CHECK_EQUAL(found, expected);
165165
}
@@ -178,7 +178,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
178178
BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
179179
{
180180
CWallet wallet("dummy", WalletDatabase::CreateDummy());
181-
CWalletTx wtx(&wallet, MakeTransactionRef(coinbaseTxns.back()));
181+
CWalletTx wtx(&wallet, m_coinbase_txns.back());
182182
LOCK2(cs_main, wallet.cs_wallet);
183183
wtx.hashBlock = chainActive.Tip()->GetBlockHash();
184184
wtx.nIndex = 0;

0 commit comments

Comments
 (0)