Skip to content

Commit d002f9d

Browse files
ryanofskyryanofsky
committed
Disable CWalletTx copy constructor
Disable copying of CWalletTx objects to prevent bugs where instances get copied in and out of the mapWallet map and fields are updated in the wrong copy.
1 parent 65b9d8f commit d002f9d

File tree

6 files changed

+19
-18
lines changed

6 files changed

+19
-18
lines changed

‎src/wallet/rpcwallet.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1582,7 +1582,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request)
15821582
UniValue transactions(UniValue::VARR);
15831583

15841584
for (const std::pair<const uint256, CWalletTx>& pairWtx : pwallet->mapWallet) {
1585-
CWalletTx tx = pairWtx.second;
1585+
const CWalletTx& tx = pairWtx.second;
15861586

15871587
if (depth == -1 || abs(tx.GetDepthInMainChain()) < depth) {
15881588
ListTransactions(pwallet, tx, 0, true, transactions, filter, nullptr /* filter_label */);

‎src/wallet/test/psbt_wallet_tests.cpp‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,12 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
2222
CDataStream s_prev_tx1(ParseHex("0200000000010158e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd7501000000171600145f275f436b09a8cc9a2eb2a2f528485c68a56323feffffff02d8231f1b0100000017a914aed962d6654f9a2b36608eb9d64d2b260db4f1118700c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88702483045022100a22edcc6e5bc511af4cc4ae0de0fcd75c7e04d8c1c3a8aa9d820ed4b967384ec02200642963597b9b1bc22c75e9f3e117284a962188bf5e8a74c895089046a20ad770121035509a48eb623e10aace8bfd0212fdb8a8e5af3c94b0b133b95e114cab89e4f7965000000"), SER_NETWORK, PROTOCOL_VERSION);
2323
CTransactionRef prev_tx1;
2424
s_prev_tx1 >> prev_tx1;
25-
CWalletTx prev_wtx1(&m_wallet, prev_tx1);
26-
m_wallet.mapWallet.emplace(prev_wtx1.GetHash(), std::move(prev_wtx1));
25+
m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx1->GetHash()), std::forward_as_tuple(&m_wallet, prev_tx1));
2726

2827
CDataStream s_prev_tx2(ParseHex("0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f618765000000"), SER_NETWORK, PROTOCOL_VERSION);
2928
CTransactionRef prev_tx2;
3029
s_prev_tx2 >> prev_tx2;
31-
CWalletTx prev_wtx2(&m_wallet, prev_tx2);
32-
m_wallet.mapWallet.emplace(prev_wtx2.GetHash(), std::move(prev_wtx2));
30+
m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx2->GetHash()), std::forward_as_tuple(&m_wallet, prev_tx2));
3331

3432
// Add scripts
3533
CScript rs1;

‎src/wallet/wallet.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3096,7 +3096,7 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
30963096
return DBErrors::LOAD_OK;
30973097
}
30983098

3099-
DBErrors CWallet::ZapWalletTx(std::vector<CWalletTx>& vWtx)
3099+
DBErrors CWallet::ZapWalletTx(std::list<CWalletTx>& vWtx)
31003100
{
31013101
DBErrors nZapWalletTxRet = WalletBatch(*database,"cr+").ZapWalletTx(vWtx);
31023102
if (nZapWalletTxRet == DBErrors::NEED_REWRITE)
@@ -3708,7 +3708,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
37083708
const std::string walletFile = WalletDataFilePath(location.GetPath()).string();
37093709

37103710
// needed to restore wallet transaction meta data after -zapwallettxes
3711-
std::vector<CWalletTx> vWtx;
3711+
std::list<CWalletTx> vWtx;
37123712

37133713
if (gArgs.GetBoolArg("-zapwallettxes", false)) {
37143714
chain.initMessage(_("Zapping all transactions from wallet...").translated);

‎src/wallet/wallet.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -553,6 +553,12 @@ class CWalletTx
553553
const uint256& GetHash() const { return tx->GetHash(); }
554554
bool IsCoinBase() const { return tx->IsCoinBase(); }
555555
bool IsImmatureCoinBase() const;
556+
557+
// Disable copying of CWalletTx objects to prevent bugs where instances get
558+
// copied in and out of the mapWallet map, and fields are updated in the
559+
// wrong copy.
560+
CWalletTx(CWalletTx const &) = delete;
561+
void operator=(CWalletTx const &x) = delete;
556562
};
557563

558564
class COutput
@@ -1057,7 +1063,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
10571063
void chainStateFlushed(const CBlockLocator& loc) override;
10581064

10591065
DBErrors LoadWallet(bool& fFirstRunRet);
1060-
DBErrors ZapWalletTx(std::vector<CWalletTx>& vWtx);
1066+
DBErrors ZapWalletTx(std::list<CWalletTx>& vWtx);
10611067
DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
10621068

10631069
bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::string& purpose);

‎src/wallet/walletdb.cpp‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -738,7 +738,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
738738
return result;
739739
}
740740

741-
DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::vector<CWalletTx>& vWtx)
741+
DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWalletTx>& vWtx)
742742
{
743743
DBErrors result = DBErrors::LOAD_OK;
744744

@@ -776,12 +776,9 @@ DBErrors WalletBatch::FindWalletTx(std::vector<uint256>& vTxHash, std::vector<CW
776776
if (strType == DBKeys::TX) {
777777
uint256 hash;
778778
ssKey >> hash;
779-
780-
CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef());
781-
ssValue >> wtx;
782-
783779
vTxHash.push_back(hash);
784-
vWtx.push_back(wtx);
780+
vWtx.emplace_back(nullptr /* wallet */, nullptr /* tx */);
781+
ssValue >> vWtx.back();
785782
}
786783
}
787784
pcursor->close();
@@ -800,7 +797,7 @@ DBErrors WalletBatch::ZapSelectTx(std::vector<uint256>& vTxHashIn, std::vector<u
800797
{
801798
// build list of wallet TXs and hashes
802799
std::vector<uint256> vTxHash;
803-
std::vector<CWalletTx> vWtx;
800+
std::list<CWalletTx> vWtx;
804801
DBErrors err = FindWalletTx(vTxHash, vWtx);
805802
if (err != DBErrors::LOAD_OK) {
806803
return err;
@@ -834,7 +831,7 @@ DBErrors WalletBatch::ZapSelectTx(std::vector<uint256>& vTxHashIn, std::vector<u
834831
return DBErrors::LOAD_OK;
835832
}
836833

837-
DBErrors WalletBatch::ZapWalletTx(std::vector<CWalletTx>& vWtx)
834+
DBErrors WalletBatch::ZapWalletTx(std::list<CWalletTx>& vWtx)
838835
{
839836
// build list of wallet TXs
840837
std::vector<uint256> vTxHash;

‎src/wallet/walletdb.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -260,8 +260,8 @@ class WalletBatch
260260
bool WriteActiveScriptPubKeyMan(uint8_t type, const uint256& id, bool internal);
261261

262262
DBErrors LoadWallet(CWallet* pwallet);
263-
DBErrors FindWalletTx(std::vector<uint256>& vTxHash, std::vector<CWalletTx>& vWtx);
264-
DBErrors ZapWalletTx(std::vector<CWalletTx>& vWtx);
263+
DBErrors FindWalletTx(std::vector<uint256>& vTxHash, std::list<CWalletTx>& vWtx);
264+
DBErrors ZapWalletTx(std::list<CWalletTx>& vWtx);
265265
DBErrors ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut);
266266
/* Try to (very carefully!) recover wallet database (with a possible key type filter) */
267267
static bool Recover(const fs::path& wallet_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& out_backup_filename);

0 commit comments

Comments
 (0)