diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -407,7 +407,6 @@ throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed"); } uint256 txid = tx.GetId(); - CWalletTx wtx(pwallet, MakeTransactionRef(std::move(tx))); CDataStream ssMB(ParseHexV(request.params[1], "proof"), SER_NETWORK, PROTOCOL_VERSION); @@ -443,10 +442,10 @@ CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height, merkleBlock.header.GetHash(), txnIndex); - wtx.m_confirm = confirm; - if (pwallet->IsMine(*wtx.tx)) { - pwallet->AddToWallet(wtx, false); + CTransactionRef tx_ref = MakeTransactionRef(tx); + if (pwallet->IsMine(*tx_ref)) { + pwallet->AddToWallet(std::move(tx_ref), confirm); return NullUniValue; } diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -30,8 +30,6 @@ // the test fail #define RANDOM_REPEATS 5 -std::vector> wtxn; - typedef std::set CoinSet; static std::vector vCoins; @@ -81,22 +79,19 @@ // fake out IsFromMe() tx.vin.resize(1); } - auto wtx = - std::make_unique(&wallet, MakeTransactionRef(std::move(tx))); + CWalletTx *wtx = wallet.AddToWallet(MakeTransactionRef(std::move(tx)), + /* confirm= */ {}); if (fIsFromMe) { wtx->m_amounts[CWalletTx::DEBIT].Set(ISMINE_SPENDABLE, SATOSHI); wtx->m_is_cache_empty = false; } - COutput output(wtx.get(), nInput, nAge, true /* spendable */, - true /* solvable */, true /* safe */); + COutput output(wtx, nInput, nAge, true /* spendable */, true /* solvable */, + true /* safe */); vCoins.push_back(output); - wallet.AddToWallet(*wtx.get()); - wtxn.emplace_back(std::move(wtx)); } static void empty_wallet() { vCoins.clear(); - wtxn.clear(); balance = Amount::zero(); } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -342,6 +342,7 @@ static int64_t AddTx(CWallet &wallet, uint32_t lockTime, int64_t mockTime, int64_t blockTime) { CMutableTransaction tx; + CWalletTx::Confirmation confirm; tx.nLockTime = lockTime; SetMockTime(mockTime); CBlockIndex *block = nullptr; @@ -353,25 +354,18 @@ block = inserted.first->second; block->nTime = blockTime; block->phashBlock = &hash; + confirm = {CWalletTx::Status::CONFIRMED, block->nHeight, hash, 0}; } - CWalletTx wtx(&wallet, MakeTransactionRef(tx)); - LOCK(wallet.cs_wallet); // If transaction is already in map, to avoid inconsistencies, // unconfirmation is needed before confirm again with different block. - std::map::iterator it = wallet.mapWallet.find(wtx.GetId()); - if (it != wallet.mapWallet.end()) { - wtx.setUnconfirmed(); - wallet.AddToWallet(wtx); - } - if (block) { - CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, - block->nHeight, block->GetBlockHash(), - 0); - wtx.m_confirm = confirm; - } - wallet.AddToWallet(wtx); - return wallet.mapWallet.at(wtx.GetId()).nTimeSmart; + return wallet + .AddToWallet(MakeTransactionRef(tx), confirm, + [&](CWalletTx &wtx, bool /* new_tx */) { + wtx.setUnconfirmed(); + return true; + }) + ->nTimeSmart; } // Simple test to verify assignment of CWalletTx::nSmartTime value. Could be diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1045,7 +1045,21 @@ DBErrors ReorderTransactions(); void MarkDirty(); - bool AddToWallet(const CWalletTx &wtxIn, bool fFlushOnClose = true); + + //! Callback for updating transaction metadata in mapWallet. + //! + //! @param wtx - reference to mapWallet transaction to update + //! @param new_tx - true if wtx is newly inserted, false if it previously + //! existed + //! + //! @return true if wtx is changed and needs to be saved to disk, otherwise + //! false + using UpdateWalletTxFn = std::function; + + CWalletTx *AddToWallet(CTransactionRef tx, + const CWalletTx::Confirmation &confirm, + const UpdateWalletTxFn &update_wtx = nullptr, + bool fFlushOnClose = true); void LoadToWallet(CWalletTx &wtxIn) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void transactionAddedToMempool(const CTransactionRef &tx) override; void blockConnected(const CBlock &block, int height) override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -854,18 +854,21 @@ return false; } -bool CWallet::AddToWallet(const CWalletTx &wtxIn, bool fFlushOnClose) { +CWalletTx *CWallet::AddToWallet(CTransactionRef tx, + const CWalletTx::Confirmation &confirm, + const UpdateWalletTxFn &update_wtx, + bool fFlushOnClose) { LOCK(cs_wallet); WalletBatch batch(*database, "r+", fFlushOnClose); - const TxId &txid = wtxIn.GetId(); + const TxId &txid = tx->GetId(); if (IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)) { // Mark used destinations std::set tx_destinations; - for (const CTxIn &txin : wtxIn.tx->vin) { + for (const CTxIn &txin : tx->vin) { const COutPoint &op = txin.prevout; SetSpentKeyState(batch, op.GetTxId(), op.GetN(), true, tx_destinations); @@ -875,12 +878,15 @@ } // Inserts only if not already there, returns tx inserted or tx found. - std::pair::iterator, bool> ret = - mapWallet.insert(std::make_pair(txid, wtxIn)); + auto ret = + mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), + std::forward_as_tuple(this, tx)); CWalletTx &wtx = (*ret.first).second; wtx.BindWallet(this); bool fInsertedNew = ret.second; + bool fUpdated = update_wtx && update_wtx(wtx, fInsertedNew); if (fInsertedNew) { + wtx.m_confirm = confirm; wtx.nTimeReceived = chain().getAdjustedTime(); wtx.nOrderPos = IncOrderPosNext(&batch); wtx.m_it_wtxOrdered = @@ -889,33 +895,27 @@ AddToSpends(txid); } - bool fUpdated = false; if (!fInsertedNew) { - if (wtxIn.m_confirm.status != wtx.m_confirm.status) { - wtx.m_confirm.status = wtxIn.m_confirm.status; - wtx.m_confirm.nIndex = wtxIn.m_confirm.nIndex; - wtx.m_confirm.hashBlock = wtxIn.m_confirm.hashBlock; - wtx.m_confirm.block_height = wtxIn.m_confirm.block_height; + if (confirm.status != wtx.m_confirm.status) { + wtx.m_confirm.status = confirm.status; + wtx.m_confirm.nIndex = confirm.nIndex; + wtx.m_confirm.hashBlock = confirm.hashBlock; + wtx.m_confirm.block_height = confirm.block_height; fUpdated = true; } else { - assert(wtx.m_confirm.nIndex == wtxIn.m_confirm.nIndex); - assert(wtx.m_confirm.hashBlock == wtxIn.m_confirm.hashBlock); - assert(wtx.m_confirm.block_height == wtxIn.m_confirm.block_height); - } - - if (wtxIn.fFromMe && wtxIn.fFromMe != wtx.fFromMe) { - wtx.fFromMe = wtxIn.fFromMe; - fUpdated = true; + assert(wtx.m_confirm.nIndex == confirm.nIndex); + assert(wtx.m_confirm.hashBlock == confirm.hashBlock); + assert(wtx.m_confirm.block_height == confirm.block_height); } } //// debug print - WalletLogPrintf("AddToWallet %s %s%s\n", wtxIn.GetId().ToString(), + WalletLogPrintf("AddToWallet %s %s%s\n", txid.ToString(), (fInsertedNew ? "new" : ""), (fUpdated ? "update" : "")); // Write to disk if ((fInsertedNew || fUpdated) && !batch.WriteTx(wtx)) { - return false; + return nullptr; } // Break debit/credit balance caches: @@ -930,7 +930,7 @@ std::string strCmd = gArgs.GetArg("-walletnotify", ""); if (!strCmd.empty()) { - boost::replace_all(strCmd, "%s", wtxIn.GetId().GetHex()); + boost::replace_all(strCmd, "%s", txid.GetHex()); #ifndef WIN32 // Substituting the wallet name isn't currently supported on windows // because windows shell escaping has not been implemented yet: @@ -946,7 +946,7 @@ } #endif - return true; + return &wtx; } void CWallet::LoadToWallet(CWalletTx &wtxIn) { @@ -1040,15 +1040,12 @@ } } - CWalletTx wtx(this, ptx); - // Block disconnection override an abandoned tx as unconfirmed // which means user may have to call abandontransaction again - wtx.m_confirm = confirm; - - return AddToWallet(wtx, false); + return AddToWallet(MakeTransactionRef(tx), confirm, + /* update_wtx= */ nullptr, + /* fFlushOnClose= */ false); } - return false; } @@ -3411,21 +3408,22 @@ std::vector> orderForm) { LOCK(cs_wallet); - CWalletTx wtxNew(this, std::move(tx)); - wtxNew.mapValue = std::move(mapValue); - wtxNew.vOrderForm = std::move(orderForm); - wtxNew.fTimeReceivedIsTxTime = true; - wtxNew.fFromMe = true; - - WalletLogPrintfToBeContinued("CommitTransaction:\n%s", - wtxNew.tx->ToString()); + WalletLogPrintfToBeContinued("CommitTransaction:\n%s", tx->ToString()); // Add tx to wallet, because if it has change it's also ours, otherwise just // for transaction history. - AddToWallet(wtxNew); + AddToWallet(tx, {}, [&](CWalletTx &wtx, bool new_tx) { + CHECK_NONFATAL(wtx.mapValue.empty()); + CHECK_NONFATAL(wtx.vOrderForm.empty()); + wtx.mapValue = std::move(mapValue); + wtx.vOrderForm = std::move(orderForm); + wtx.fTimeReceivedIsTxTime = true; + wtx.fFromMe = true; + return true; + }); // Notify that old coins are spent. - for (const CTxIn &txin : wtxNew.tx->vin) { + for (const CTxIn &txin : tx->vin) { CWalletTx &coin = mapWallet.at(txin.prevout.GetTxId()); coin.BindWallet(this); NotifyTransactionChanged(this, coin.GetId(), CT_UPDATED); @@ -3433,7 +3431,7 @@ // Get the inserted-CWalletTx from mapWallet so that the // fInMempool flag is cached properly - CWalletTx &wtx = mapWallet.at(wtxNew.GetId()); + CWalletTx &wtx = mapWallet.at(tx->GetId()); if (!fBroadcastTransactions) { // Don't submit tx to the mempool