From 5971d3848e09abf571e5308185275296127efca4 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Sat, 20 Apr 2019 11:22:59 -0400 Subject: [PATCH] Add block_height field in struct Confirmation At wallet loading, we rely on chain state querying to retrieve height of txn, to do so we ensure that lock order is respected between cs_main and cs_wallet. If wallet loaded is the wallet-tool one, all wallet txn will show up with a height of zero. It doesn't matter as confirmation height is not used by wallet-tool. Reorder arguments and document Confirmation calls to avoid ambiguity. Fixes nits left from #16624 --- src/wallet/rpcdump.cpp | 6 +++-- src/wallet/test/wallet_tests.cpp | 6 ++--- src/wallet/wallet.cpp | 42 ++++++++++++++++++++------------ src/wallet/wallet.h | 15 +++++++----- 4 files changed, 43 insertions(+), 26 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index bc6df1cc99e..ddb8bfaf179 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -364,10 +364,12 @@ UniValue importprunedfunds(const JSONRPCRequest& request) std::vector vMatch; std::vector vIndex; unsigned int txnIndex = 0; + Optional height; if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) == merkleBlock.header.hashMerkleRoot) { auto locked_chain = pwallet->chain().lock(); - if (locked_chain->getBlockHeight(merkleBlock.header.GetHash()) == nullopt) { + height = locked_chain->getBlockHeight(merkleBlock.header.GetHash()); + if (height == nullopt) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain"); } @@ -382,7 +384,7 @@ UniValue importprunedfunds(const JSONRPCRequest& request) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Something wrong with merkleblock"); } - CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, merkleBlock.header.GetHash(), txnIndex); + CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, *height, merkleBlock.header.GetHash(), txnIndex); wtx.m_confirm = confirm; auto locked_chain = pwallet->chain().lock(); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index b6acaf4d999..3f0e40149cf 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -276,7 +276,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) AssertLockHeld(spk_man->cs_wallet); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); - CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, ::ChainActive().Tip()->GetBlockHash(), 0); + CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, ::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash(), 0); wtx.m_confirm = confirm; // Call GetImmatureCredit() once before adding the key to the wallet to @@ -318,7 +318,7 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64 wallet.AddToWallet(wtx); } if (block) { - CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, block->GetBlockHash(), 0); + CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, block->nHeight, block->GetBlockHash(), 0); wtx.m_confirm = confirm; } wallet.AddToWallet(wtx); @@ -499,7 +499,7 @@ public: wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, ::ChainActive().Tip()->GetBlockHash()); auto it = wallet->mapWallet.find(tx->GetHash()); BOOST_CHECK(it != wallet->mapWallet.end()); - CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, ::ChainActive().Tip()->GetBlockHash(), 1); + CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, ::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash(), 1); it->second.m_confirm = confirm; return it->second; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2ee1d001b91..512273aa822 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -766,10 +766,12 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) 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; 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) { @@ -820,12 +822,22 @@ void CWallet::LoadToWallet(CWalletTx& wtxIn) { // If wallet doesn't have a chain (e.g wallet-tool), lock can't be taken. auto locked_chain = LockChain(); - // If tx hasn't been reorged out of chain while wallet being shutdown - // change tx status to UNCONFIRMED and reset hashBlock/nIndex. - if (!wtxIn.m_confirm.hashBlock.IsNull()) { - if (locked_chain && !locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock)) { + if (locked_chain) { + Optional block_height = locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock); + if (block_height) { + // Update cached block height variable since it not stored in the + // serialized transaction. + wtxIn.m_confirm.block_height = *block_height; + } else if (wtxIn.isConflicted() || wtxIn.isConfirmed()) { + // If tx block (or conflicting block) was reorged out of chain + // while the wallet was shutdown, change tx status to UNCONFIRMED + // and reset block height, hash, and index. ABANDONED tx don't have + // associated blocks and don't need to be updated. The case where a + // transaction was reorged out while online and then reconfirmed + // while offline is covered by the rescan logic. wtxIn.setUnconfirmed(); wtxIn.m_confirm.hashBlock = uint256(); + wtxIn.m_confirm.block_height = 0; wtxIn.m_confirm.nIndex = 0; } } @@ -842,7 +854,7 @@ void CWallet::LoadToWallet(CWalletTx& wtxIn) if (it != mapWallet.end()) { CWalletTx& prevtx = it->second; if (prevtx.isConflicted()) { - MarkConflicted(prevtx.m_confirm.hashBlock, wtx.GetHash()); + MarkConflicted(prevtx.m_confirm.hashBlock, prevtx.m_confirm.block_height, wtx.GetHash()); } } } @@ -860,7 +872,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co while (range.first != range.second) { if (range.first->second != tx.GetHash()) { WalletLogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), confirm.hashBlock.ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); - MarkConflicted(confirm.hashBlock, range.first->second); + MarkConflicted(confirm.hashBlock, confirm.block_height, range.first->second); } range.first++; } @@ -948,7 +960,6 @@ bool CWallet::AbandonTransaction(interfaces::Chain::Lock& locked_chain, const ui if (currentconfirm == 0 && !wtx.isAbandoned()) { // If the orig tx was not in block/mempool, none of its spends can be in mempool assert(!wtx.InMempool()); - wtx.m_confirm.nIndex = 0; wtx.setAbandoned(); wtx.MarkDirty(); batch.WriteTx(wtx); @@ -970,7 +981,7 @@ bool CWallet::AbandonTransaction(interfaces::Chain::Lock& locked_chain, const ui return true; } -void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) +void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx) { auto locked_chain = chain().lock(); LOCK(cs_wallet); @@ -1004,6 +1015,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) // Mark transaction as conflicted with this block. wtx.m_confirm.nIndex = 0; wtx.m_confirm.hashBlock = hashBlock; + wtx.m_confirm.block_height = conflicting_height; wtx.setConflicted(); wtx.MarkDirty(); batch.WriteTx(wtx); @@ -1036,7 +1048,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Confirmatio void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { auto locked_chain = chain().lock(); LOCK(cs_wallet); - CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, {}, 0); + CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0); SyncTransaction(ptx, confirm); auto it = mapWallet.find(ptx->GetHash()); @@ -1061,10 +1073,10 @@ void CWallet::BlockConnected(const CBlock& block, const std::vectorGetHash(); } bool IsCoinBase() const { return tx->IsCoinBase(); } @@ -644,7 +647,7 @@ private: bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Confirmation confirm, bool fUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /* Mark a transaction (and its in-wallet descendants) as conflicting with a particular block. */ - void MarkConflicted(const uint256& hashBlock, const uint256& hashTx); + void MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx); /* Mark a transaction's inputs dirty, thus forcing the outputs to be recomputed */ void MarkInputsDirty(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);