Skip to content

Commit

Permalink
Remove CWalletTx merging logic from AddToWallet
Browse files Browse the repository at this point in the history 
Instead of AddToWallet taking a temporary CWalletTx object and then potentially
merging it with a pre-existing CWalletTx, have it take a callback so callers
can update the pre-existing CWalletTx directly.

This makes AddToWallet simpler because now it is only has to be concerned with
saving CWalletTx objects and not merging them.

This makes AddToWallet calls clearer because they can now make direct updates to
CWalletTx entries without having to make temporary objects and then worry about
how they will be merged.

This is a pure refactoring, no behavior is changing.
  • Loading branch information
@ryanofsky
ryanofsky committed May 1, 2020
1 parent 608359b commit 2b9cba2
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 64 deletions.
7 changes: 3 additions & 4 deletions src/wallet/rpcdump.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,6 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
if (!DecodeHexTx(tx, request.params[0].get_str()))
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");
uint256 hashTx = tx.GetHash();
CWalletTx wtx(pwallet, MakeTransactionRef(std::move(tx)));

CDataStream ssMB(ParseHexV(request.params[1], "proof"), SER_NETWORK, PROTOCOL_VERSION);
CMerkleBlock merkleBlock;
Expand All @@ -372,10 +371,10 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
unsigned int txnIndex = vIndex[it - vMatch.begin()];

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;
}

Expand Down
9 changes: 2 additions & 7 deletions src/wallet/test/coinselector_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ BOOST_FIXTURE_TEST_SUITE(coinselector_tests, WalletTestingSetup)
// we repeat those tests this many times and only complain if all iterations of the test fail
#define RANDOM_REPEATS 5

std::vector<std::unique_ptr<CWalletTx>> wtxn;

typedef std::set<CInputCoin> CoinSet;

static std::vector<COutput> vCoins;
Expand Down Expand Up @@ -74,16 +72,14 @@ static void add_coin(CWallet& wallet, const CAmount& nValue, int nAge = 6*24, bo
// so stop vin being empty, and cache a non-zero Debit to fake out IsFromMe()
tx.vin.resize(1);
}
std::unique_ptr<CWalletTx> wtx = MakeUnique<CWalletTx>(&wallet, MakeTransactionRef(std::move(tx)));
CWalletTx* wtx = wallet.AddToWallet(MakeTransactionRef(std::move(tx)), /* confirm= */ {});
if (fIsFromMe)
{
wtx->m_amounts[CWalletTx::DEBIT].Set(ISMINE_SPENDABLE, 1);
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 add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false)
{
Expand All @@ -93,7 +89,6 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa
static void empty_wallet(void)
{
vCoins.clear();
wtxn.clear();
balance = 0;
}

Expand Down
17 changes: 5 additions & 12 deletions src/wallet/test/wallet_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
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;
Expand All @@ -341,23 +342,15 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64
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<uint256, CWalletTx>::iterator it = wallet.mapWallet.find(wtx.GetHash());
if (it != wallet.mapWallet.end()) {
return wallet.AddToWallet(MakeTransactionRef(tx), confirm, [&](CWalletTx& wtx, bool /* new_tx */) {
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.GetHash()).nTimeSmart;
return true;
})->nTimeSmart;
}

// Simple test to verify assignment of CWalletTx::nSmartTime value. Could be
Expand Down
73 changes: 33 additions & 40 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -783,19 +783,19 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
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);

uint256 hash = wtxIn.GetHash();
uint256 hash = tx->GetHash();

if (IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE)) {
// Mark used destinations
std::set<CTxDestination> tx_destinations;

for (const CTxIn& txin : wtxIn.tx->vin) {
for (const CTxIn& txin : tx->vin) {
const COutPoint& op = txin.prevout;
SetSpentKeyState(batch, op.hash, op.n, true, tx_destinations);
}
Expand All @@ -804,55 +804,51 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
}

// Inserts only if not already there, returns tx inserted or tx found
std::pair<std::map<uint256, CWalletTx>::iterator, bool> ret = mapWallet.insert(std::make_pair(hash, wtxIn));
auto ret = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), 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 = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
wtx.nTimeSmart = ComputeTimeSmart(wtx);
AddToSpends(hash);
}

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);
}
// If we have a witness-stripped version of this transaction, and we
// see a new version with a witness, then we must be upgrading a pre-segwit
// wallet. Store the new version of the transaction with the witness,
// as the stripped-version must be invalid.
// TODO: Store all versions of the transaction, instead of just one.
if (wtxIn.tx->HasWitness() && !wtx.tx->HasWitness()) {
wtx.SetTx(wtxIn.tx);
if (tx->HasWitness() && !wtx.tx->HasWitness()) {
wtx.SetTx(tx);
fUpdated = true;
}
}

//// debug print
WalletLogPrintf("AddToWallet %s %s%s\n", wtxIn.GetHash().ToString(), (fInsertedNew ? "new" : ""), (fUpdated ? "update" : ""));
WalletLogPrintf("AddToWallet %s %s%s\n", hash.ToString(), (fInsertedNew ? "new" : ""), (fUpdated ? "update" : ""));

// Write to disk
if (fInsertedNew || fUpdated)
if (!batch.WriteTx(wtx))
return false;
return nullptr;

// Break debit/credit balance caches:
wtx.MarkDirty();
Expand All @@ -866,7 +862,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)

if (!strCmd.empty())
{
boost::replace_all(strCmd, "%s", wtxIn.GetHash().GetHex());
boost::replace_all(strCmd, "%s", hash.GetHex());
#ifndef WIN32
// Substituting the wallet name isn't currently supported on windows
// because windows shell escaping has not been implemented yet:
Expand All @@ -880,7 +876,7 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
}
#endif

return true;
return &wtx;
}

void CWallet::LoadToWallet(CWalletTx& wtxIn)
Expand Down Expand Up @@ -960,13 +956,9 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co
}
}

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;
Expand Down Expand Up @@ -3007,29 +2999,30 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> 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;

WalletLogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString()); /* Continued */
WalletLogPrintf("CommitTransaction:\n%s", tx->ToString()); /* Continued */

// 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.hash);
coin.BindWallet(this);
NotifyTransactionChanged(this, coin.GetHash(), CT_UPDATED);
}

// Get the inserted-CWalletTx from mapWallet so that the
// fInMempool flag is cached properly
CWalletTx& wtx = mapWallet.at(wtxNew.GetHash());
CWalletTx& wtx = mapWallet.at(tx->GetHash());

if (!fBroadcastTransactions) {
// Don't submit tx to the mempool
Expand Down
11 changes: 10 additions & 1 deletion src/wallet/wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,16 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
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<bool(CWalletTx& wtx, bool new_tx)>;

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;
Expand Down

0 comments on commit 2b9cba2

Please sign in to comment.