Skip to content

Commit 4e1f270

Browse files
furszyfurszy
committed
Make CTransaction actually immutable
Adaptation coming from btc@81e3228fcb33e8ed32d8b9fbe917444ba080073a
1 parent 896b5e2 commit 4e1f270

15 files changed

+101
-109
lines changed

‎src/core_io.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
class CBlock;
1313
class CScript;
1414
class CTransaction;
15-
class CMutableTransaction;
15+
struct CMutableTransaction;
1616
class uint256;
1717
class UniValue;
1818

‎src/net_processing.cpp‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1395,9 +1395,8 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
13951395
else if (strCommand == NetMsgType::TX) {
13961396
std::vector<uint256> vWorkQueue;
13971397
std::vector<uint256> vEraseQueue;
1398-
CTransactionRef ptx;
1399-
vRecv >> ptx;
1400-
const CTransaction& tx = *ptx;
1398+
CTransaction tx(deserialize, vRecv);
1399+
CTransactionRef ptx = MakeTransactionRef(tx);
14011400

14021401
CInv inv(MSG_TX, tx.GetHash());
14031402
pfrom->AddInventoryKnown(inv);

‎src/primitives/transaction.cpp‎

Lines changed: 6 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -121,36 +121,20 @@ uint256 CMutableTransaction::GetHash() const
121121
return SerializeHash(*this);
122122
}
123123

124-
void CTransaction::UpdateHash() const
124+
uint256 CTransaction::ComputeHash() const
125125
{
126-
*const_cast<uint256*>(&hash) = SerializeHash(*this);
126+
return SerializeHash(*this);
127127
}
128128

129129
size_t CTransaction::DynamicMemoryUsage() const
130130
{
131131
return memusage::RecursiveDynamicUsage(vin) + memusage::RecursiveDynamicUsage(vout);
132132
}
133133

134-
CTransaction::CTransaction() : nVersion(CTransaction::CURRENT_VERSION), nType(TxType::NORMAL), vin(), vout(), nLockTime(0) { }
135-
136-
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) {
137-
UpdateHash();
138-
}
139-
140-
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) {
141-
UpdateHash();
142-
}
143-
144-
CTransaction& CTransaction::operator=(const CTransaction &tx) {
145-
*const_cast<int16_t*>(&nVersion) = tx.nVersion;
146-
*const_cast<int16_t*>(&nType) = tx.nType;
147-
*const_cast<std::vector<CTxIn>*>(&vin) = tx.vin;
148-
*const_cast<std::vector<CTxOut>*>(&vout) = tx.vout;
149-
*const_cast<unsigned int*>(&nLockTime) = tx.nLockTime;
150-
*const_cast<uint256*>(&hash) = tx.hash;
151-
*const_cast<Optional<SaplingTxData>*>(&sapData) = tx.sapData;
152-
return *this;
153-
}
134+
/* For backward compatibility, the hash is initialized to 0. TODO: remove the need for this default constructor entirely. */
135+
CTransaction::CTransaction() : nVersion(CTransaction::CURRENT_VERSION), nType(TxType::NORMAL), vin(), vout(), nLockTime(0), hash() {}
136+
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()) {}
137+
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()) {}
154138

155139
bool CTransaction::HasZerocoinSpendInputs() const
156140
{

‎src/primitives/transaction.h‎

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -221,18 +221,35 @@ struct CMutableTransaction;
221221
* - Optional<SaplingTxData> sapData
222222
* - Optional<std::vector<uint8_t>> extraPayload
223223
*/
224-
template<typename Stream, typename Operation, typename TxType>
225-
inline void SerializeTransaction(TxType& tx, Stream& s, Operation ser_action) {
226-
READWRITE(*const_cast<int16_t*>(&tx.nVersion));
227-
READWRITE(*const_cast<int16_t*>(&tx.nType));
228-
READWRITE(*const_cast<std::vector<CTxIn>*>(&tx.vin));
229-
READWRITE(*const_cast<std::vector<CTxOut>*>(&tx.vout));
230-
READWRITE(*const_cast<uint32_t*>(&tx.nLockTime));
224+
template<typename Stream, typename TxType>
225+
inline void UnserializeTransaction(TxType& tx, Stream& s) {
226+
tx.vin.clear();
227+
tx.vout.clear();
228+
229+
s >> tx.nVersion;
230+
s >> tx.nType;
231+
s >> tx.vin;
232+
s >> tx.vout;
233+
s >> tx.nLockTime;
234+
if (tx.isSaplingVersion()) {
235+
s >> tx.sapData;
236+
if (!tx.IsNormalType()) {
237+
s >> tx.extraPayload;
238+
}
239+
}
240+
}
231241

242+
template<typename Stream, typename TxType>
243+
inline void SerializeTransaction(const TxType& tx, Stream& s) {
244+
s << tx.nVersion;
245+
s << tx.nType;
246+
s << tx.vin;
247+
s << tx.vout;
248+
s << tx.nLockTime;
232249
if (tx.isSaplingVersion()) {
233-
READWRITE(*const_cast<Optional<SaplingTxData>*>(&tx.sapData));
250+
s << tx.sapData;
234251
if (!tx.IsNormalType()) {
235-
READWRITE(*const_cast<Optional<std::vector<uint8_t>>*>(&tx.extraPayload));
252+
s << tx.extraPayload;
236253
}
237254
}
238255
}
@@ -242,11 +259,6 @@ inline void SerializeTransaction(TxType& tx, Stream& s, Operation ser_action) {
242259
*/
243260
class CTransaction
244261
{
245-
private:
246-
/** Memory only. */
247-
const uint256 hash;
248-
void UpdateHash() const;
249-
250262
public:
251263
/** Transaction Versions */
252264
enum TxVersion: int16_t {
@@ -283,18 +295,14 @@ class CTransaction
283295
CTransaction(CMutableTransaction &&tx);
284296

285297
CTransaction(const CTransaction& tx) = default;
286-
CTransaction& operator=(const CTransaction& tx);
287-
288-
ADD_SERIALIZE_METHODS;
289298

290-
template <typename Stream, typename Operation>
291-
inline void SerializationOp(Stream& s, Operation ser_action) {
292-
SerializeTransaction(*this, s, ser_action);
293-
if (ser_action.ForRead()) {
294-
UpdateHash();
295-
}
299+
template <typename Stream>
300+
inline void Serialize(Stream& s) const {
301+
SerializeTransaction(*this, s);
296302
}
297303

304+
/** This deserializing constructor is provided instead of an Unserialize method.
305+
* Unserialize is not possible, since it would require overwriting const fields. */
298306
template <typename Stream>
299307
CTransaction(deserialize_type, Stream& s) : CTransaction(CMutableTransaction(deserialize, s)) {}
300308

@@ -400,6 +408,11 @@ class CTransaction
400408
std::string ToString() const;
401409

402410
size_t DynamicMemoryUsage() const;
411+
412+
private:
413+
/** Memory only. */
414+
const uint256 hash;
415+
uint256 ComputeHash() const;
403416
};
404417

405418
/** A mutable version of CTransaction. */
@@ -416,11 +429,14 @@ struct CMutableTransaction
416429
CMutableTransaction();
417430
CMutableTransaction(const CTransaction& tx);
418431

419-
ADD_SERIALIZE_METHODS;
432+
template <typename Stream>
433+
inline void Serialize(Stream& s) const {
434+
SerializeTransaction(*this, s);
435+
}
420436

421-
template <typename Stream, typename Operation>
422-
inline void SerializationOp(Stream& s, Operation ser_action) {
423-
SerializeTransaction(*this, s, ser_action);
437+
template <typename Stream>
438+
inline void Unserialize(Stream& s) {
439+
UnserializeTransaction(*this, s);
424440
}
425441

426442
template <typename Stream>

‎src/sapling/sapling_operation.cpp‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -177,12 +177,13 @@ OperationResult SaplingOperation::build()
177177
if (!opTx) {
178178
return errorOut("Failed to build transaction: " + txResult.GetError());
179179
}
180-
finalTx = *opTx;
180+
finalTx.reset();
181+
finalTx = MakeTransactionRef(*opTx);
181182

182183
// Now check fee
183-
bool isShielded = finalTx.IsShieldedTx();
184-
const CAmount& nFeeNeeded = isShielded ? GetShieldedTxMinFee(finalTx) :
185-
GetMinRelayFee(finalTx.GetTotalSize(), false);
184+
bool isShielded = finalTx->IsShieldedTx();
185+
const CAmount& nFeeNeeded = isShielded ? GetShieldedTxMinFee(*finalTx) :
186+
GetMinRelayFee(finalTx->GetTotalSize(), false);
186187
if (nFeeNeeded <= nFeeRet) {
187188
// Check that the fee is not too high.
188189
CAmount nMaxFee = nFeeNeeded * (isShielded ? 100 : 10000);
@@ -222,19 +223,20 @@ OperationResult SaplingOperation::build()
222223
if (!opTx) {
223224
return errorOut("Failed to build transaction: " + txResult.GetError());
224225
}
225-
finalTx = *opTx;
226+
finalTx.reset();
227+
finalTx = MakeTransactionRef(*opTx);
226228
return OperationResult(true);
227229
}
228230

229231
OperationResult SaplingOperation::send(std::string& retTxHash)
230232
{
231-
CWalletTx wtx(pwalletMain, MakeTransactionRef(finalTx));
233+
CWalletTx wtx(pwalletMain, finalTx);
232234
const CWallet::CommitResult& res = pwalletMain->CommitTransaction(wtx, tkeyChange, g_connman.get());
233235
if (res.status != CWallet::CommitStatus::OK) {
234236
return errorOut(res.ToString());
235237
}
236238

237-
retTxHash = finalTx.GetHash().ToString();
239+
retTxHash = finalTx->GetHash().ToString();
238240
return OperationResult(true);
239241
}
240242

‎src/sapling/sapling_operation.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,8 @@ class SaplingOperation {
104104
SaplingOperation* setCoinControl(const CCoinControl* _coinControl) { coinControl = _coinControl; return this; }
105105

106106
CAmount getFee() { return fee; }
107-
CTransaction getFinalTx() { return finalTx; }
107+
CTransaction getFinalTx() { return *finalTx; }
108+
CTransactionRef getFinalTxRef() { return finalTx; }
108109

109110
private:
110111
FromAddress fromAddress;
@@ -124,7 +125,7 @@ class SaplingOperation {
124125

125126
// Builder
126127
TransactionBuilder txBuilder;
127-
CTransaction finalTx;
128+
CTransactionRef finalTx;
128129

129130
OperationResult loadUtxos(TxValues& values);
130131
OperationResult loadUtxos(TxValues& txValues, const std::vector<COutput>& selectedUTXO, const CAmount selectedUTXOAmount);

‎src/script/bitcoinconsensus.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,7 @@ int bitcoinconsensus_verify_script(const unsigned char *scriptPubKey, unsigned i
7878
{
7979
try {
8080
TxInputStream stream(SER_NETWORK, PROTOCOL_VERSION, txTo, txToLen);
81-
CTransaction tx;
82-
stream >> tx;
81+
CTransaction tx(deserialize, stream);
8382
if (nIn >= tx.vin.size())
8483
return set_error(err, bitcoinconsensus_ERR_TX_INDEX);
8584
if (GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION) != txToLen)

‎src/script/sign.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ class TransactionSignatureCreator : public BaseSignatureCreator {
4646
};
4747

4848
class MutableTransactionSignatureCreator : public TransactionSignatureCreator {
49-
CTransaction tx;
49+
const CTransaction tx;
5050

5151
public:
5252
MutableTransactionSignatureCreator(const CKeyStore* keystoreIn, const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amount, int nHashTypeIn) : TransactionSignatureCreator(keystoreIn, &tx, nInIn, amount, nHashTypeIn), tx(*txToIn) {}

‎src/test/bloom_tests.cpp‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -111,16 +111,14 @@ BOOST_AUTO_TEST_CASE(bloom_create_insert_key)
111111
BOOST_AUTO_TEST_CASE(bloom_match)
112112
{
113113
// Random real transaction (b4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b)
114-
CTransaction tx;
115114
CDataStream stream(ParseHex("01000000010b26e9b7735eb6aabdf358bab62f9816a21ba9ebdb719d5299e88607d722c190000000008b4830450220070aca44506c5cef3a16ed519d7c3c39f8aab192c4e1c90d065f37b8a4af6141022100a8e160b856c2d43d27d8fba71e5aef6405b8643ac4cb7cb3c462aced7f14711a0141046d11fee51b0e60666d5049a9101a72741df480b96ee26488a4d3466b95c9a40ac5eeef87e10a5cd336c19a84565f80fa6c547957b7700ff4dfbdefe76036c339ffffffff021bff3d11000000001976a91404943fdd508053c75000106d3bc6e2754dbcff1988ac2f15de00000000001976a914a266436d2965547608b9e15d9032a7b9d64fa43188ac00000000"), SER_DISK, CLIENT_VERSION);
116-
stream >> tx;
115+
CTransaction tx(deserialize, stream);
117116

118117
// and one which spends it (e2769b09e784f32f62ef849763d4f45b98e07ba658647343b915ff832b110436)
119118
unsigned char ch[] = {0x01, 0x00, 0x00, 0x00, 0x01, 0x6b, 0xff, 0x7f, 0xcd, 0x4f, 0x85, 0x65, 0xef, 0x40, 0x6d, 0xd5, 0xd6, 0x3d, 0x4f, 0xf9, 0x4f, 0x31, 0x8f, 0xe8, 0x20, 0x27, 0xfd, 0x4d, 0xc4, 0x51, 0xb0, 0x44, 0x74, 0x01, 0x9f, 0x74, 0xb4, 0x00, 0x00, 0x00, 0x00, 0x8c, 0x49, 0x30, 0x46, 0x02, 0x21, 0x00, 0xda, 0x0d, 0xc6, 0xae, 0xce, 0xfe, 0x1e, 0x06, 0xef, 0xdf, 0x05, 0x77, 0x37, 0x57, 0xde, 0xb1, 0x68, 0x82, 0x09, 0x30, 0xe3, 0xb0, 0xd0, 0x3f, 0x46, 0xf5, 0xfc, 0xf1, 0x50, 0xbf, 0x99, 0x0c, 0x02, 0x21, 0x00, 0xd2, 0x5b, 0x5c, 0x87, 0x04, 0x00, 0x76, 0xe4, 0xf2, 0x53, 0xf8, 0x26, 0x2e, 0x76, 0x3e, 0x2d, 0xd5, 0x1e, 0x7f, 0xf0, 0xbe, 0x15, 0x77, 0x27, 0xc4, 0xbc, 0x42, 0x80, 0x7f, 0x17, 0xbd, 0x39, 0x01, 0x41, 0x04, 0xe6, 0xc2, 0x6e, 0xf6, 0x7d, 0xc6, 0x10, 0xd2, 0xcd, 0x19, 0x24, 0x84, 0x78, 0x9a, 0x6c, 0xf9, 0xae, 0xa9, 0x93, 0x0b, 0x94, 0x4b, 0x7e, 0x2d, 0xb5, 0x34, 0x2b, 0x9d, 0x9e, 0x5b, 0x9f, 0xf7, 0x9a, 0xff, 0x9a, 0x2e, 0xe1, 0x97, 0x8d, 0xd7, 0xfd, 0x01, 0xdf, 0xc5, 0x22, 0xee, 0x02, 0x28, 0x3d, 0x3b, 0x06, 0xa9, 0xd0, 0x3a, 0xcf, 0x80, 0x96, 0x96, 0x8d, 0x7d, 0xbb, 0x0f, 0x91, 0x78, 0xff, 0xff, 0xff, 0xff, 0x02, 0x8b, 0xa7, 0x94, 0x0e, 0x00, 0x00, 0x00, 0x00, 0x19, 0x76, 0xa9, 0x14, 0xba, 0xde, 0xec, 0xfd, 0xef, 0x05, 0x07, 0x24, 0x7f, 0xc8, 0xf7, 0x42, 0x41, 0xd7, 0x3b, 0xc0, 0x39, 0x97, 0x2d, 0x7b, 0x88, 0xac, 0x40, 0x94, 0xa8, 0x02, 0x00, 0x00, 0x00, 0x00, 0x19, 0x76, 0xa9, 0x14, 0xc1, 0x09, 0x32, 0x48, 0x3f, 0xec, 0x93, 0xed, 0x51, 0xf5, 0xfe, 0x95, 0xe7, 0x25, 0x59, 0xf2, 0xcc, 0x70, 0x43, 0xf9, 0x88, 0xac, 0x00, 0x00, 0x00, 0x00, 0x00};
120119
std::vector<unsigned char> vch(ch, ch + sizeof(ch) -1);
121120
CDataStream spendStream(vch, SER_DISK, CLIENT_VERSION);
122-
CTransaction spendingTx;
123-
spendStream >> spendingTx;
121+
CTransaction spendingTx(deserialize, spendStream);
124122

125123
CBloomFilter filter(10, 0.000001, 0, BLOOM_UPDATE_ALL);
126124
filter.insert(uint256("0xb4749f017444b051c44dfd2720e88f314ff94f3dd6d56d40ef65854fcd7fff6b"));

‎src/test/coins_tests.cpp‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ class CCoinsViewTest : public CCoinsView
172172
class TxWithNullifiers
173173
{
174174
public:
175-
CTransaction tx;
175+
CTransactionRef tx;
176176
uint256 saplingNullifier;
177177

178178
TxWithNullifiers()
@@ -183,7 +183,7 @@ class TxWithNullifiers
183183
SpendDescription sd;
184184
sd.nullifier = saplingNullifier;
185185
mutableTx.sapData->vShieldedSpend.push_back(sd);
186-
tx = CTransaction(mutableTx);
186+
tx = MakeTransactionRef(CTransaction(mutableTx));
187187
}
188188
};
189189

@@ -235,12 +235,12 @@ BOOST_AUTO_TEST_CASE(nullifier_regression_test)
235235
TxWithNullifiers txWithNullifiers;
236236

237237
// Insert a nullifier into the base.
238-
cache1.SetNullifiers(txWithNullifiers.tx, true);
238+
cache1.SetNullifiers(*txWithNullifiers.tx, true);
239239
checkNullifierCache(cache1, txWithNullifiers, true);
240240
cache1.Flush(); // Flush to base.
241241

242242
// Remove the nullifier from cache
243-
cache1.SetNullifiers(txWithNullifiers.tx, false);
243+
cache1.SetNullifiers(*txWithNullifiers.tx, false);
244244

245245
// The nullifier now should be `false`.
246246
checkNullifierCache(cache1, txWithNullifiers, false);
@@ -254,12 +254,12 @@ BOOST_AUTO_TEST_CASE(nullifier_regression_test)
254254
TxWithNullifiers txWithNullifiers;
255255

256256
// Insert a nullifier into the base.
257-
cache1.SetNullifiers(txWithNullifiers.tx, true);
257+
cache1.SetNullifiers(*txWithNullifiers.tx, true);
258258
checkNullifierCache(cache1, txWithNullifiers, true);
259259
cache1.Flush(); // Flush to base.
260260

261261
// Remove the nullifier from cache
262-
cache1.SetNullifiers(txWithNullifiers.tx, false);
262+
cache1.SetNullifiers(*txWithNullifiers.tx, false);
263263
cache1.Flush(); // Flush to base.
264264

265265
// The nullifier now should be `false`.
@@ -273,7 +273,7 @@ BOOST_AUTO_TEST_CASE(nullifier_regression_test)
273273

274274
// Insert a nullifier into the base.
275275
TxWithNullifiers txWithNullifiers;
276-
cache1.SetNullifiers(txWithNullifiers.tx, true);
276+
cache1.SetNullifiers(*txWithNullifiers.tx, true);
277277
checkNullifierCache(cache1, txWithNullifiers, true);
278278
cache1.Flush(); // Empties cache.
279279

@@ -282,7 +282,7 @@ BOOST_AUTO_TEST_CASE(nullifier_regression_test)
282282
// Remove the nullifier.
283283
CCoinsViewCache cache2(&cache1);
284284
checkNullifierCache(cache2, txWithNullifiers, true);
285-
cache1.SetNullifiers(txWithNullifiers.tx, false);
285+
cache1.SetNullifiers(*txWithNullifiers.tx, false);
286286
cache2.Flush(); // Empties cache, flushes to cache1.
287287
}
288288

@@ -297,14 +297,14 @@ BOOST_AUTO_TEST_CASE(nullifier_regression_test)
297297

298298
// Insert a nullifier into the base.
299299
TxWithNullifiers txWithNullifiers;
300-
cache1.SetNullifiers(txWithNullifiers.tx, true);
300+
cache1.SetNullifiers(*txWithNullifiers.tx, true);
301301
cache1.Flush(); // Empties cache.
302302

303303
// Create cache on top.
304304
{
305305
// Remove the nullifier.
306306
CCoinsViewCache cache2(&cache1);
307-
cache2.SetNullifiers(txWithNullifiers.tx, false);
307+
cache2.SetNullifiers(*txWithNullifiers.tx, false);
308308
cache2.Flush(); // Empties cache, flushes to cache1.
309309
}
310310

@@ -485,14 +485,14 @@ BOOST_AUTO_TEST_CASE(nullifiers_test)
485485

486486
TxWithNullifiers txWithNullifiers;
487487
checkNullifierCache(cache, txWithNullifiers, false);
488-
cache.SetNullifiers(txWithNullifiers.tx, true);
488+
cache.SetNullifiers(*txWithNullifiers.tx, true);
489489
checkNullifierCache(cache, txWithNullifiers, true);
490490
cache.Flush();
491491

492492
CCoinsViewCache cache2(&base);
493493

494494
checkNullifierCache(cache2, txWithNullifiers, true);
495-
cache2.SetNullifiers(txWithNullifiers.tx, false);
495+
cache2.SetNullifiers(*txWithNullifiers.tx, false);
496496
checkNullifierCache(cache2, txWithNullifiers, false);
497497
cache2.Flush();
498498

0 commit comments

Comments
 (0)