From c5ccf9ec01af48dff80b6da228966222f4d0223d Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 29 Jun 2021 15:54:20 +0200 Subject: [PATCH 1/3] Use TransactionLocationReference consistently. This refactors some parts of the code (mainly the UTXO tracking) to use TransactionLocationReference more consistently instead of e.g. ad-hoc passing of a raw block height. Describing the "details" of a transaction in the blockchain (including transaction hash and confirmed block height) is what TransactionLocationReference is meant for, so this makes sense. With these changes, it will also be easier to use the UTXO hash of a transaction in the future, as that will just be added seamlessly to TransactionLocationReference and then won't need to be customarily wired through. --- divi/src/ActiveChainManager.cpp | 4 ++-- divi/src/BlockTransactionChecker.cpp | 2 +- divi/src/IndexDatabaseUpdates.cpp | 8 +++++--- divi/src/IndexDatabaseUpdates.h | 4 +++- divi/src/UtxoCheckingAndUpdating.cpp | 11 ++++++----- divi/src/UtxoCheckingAndUpdating.h | 4 +++- 6 files changed, 20 insertions(+), 13 deletions(-) diff --git a/divi/src/ActiveChainManager.cpp b/divi/src/ActiveChainManager.cpp index 7f7753064..ba41b5ed1 100644 --- a/divi/src/ActiveChainManager.cpp +++ b/divi/src/ActiveChainManager.cpp @@ -92,15 +92,15 @@ bool ActiveChainManager::DisconnectBlock( // undo transactions in reverse order for (int transactionIndex = block.vtx.size() - 1; transactionIndex >= 0; transactionIndex--) { const CTransaction& tx = block.vtx[transactionIndex]; + const TransactionLocationReference txLocationReference(tx, pindex->nHeight, transactionIndex); const auto* undo = (transactionIndex > 0 ? &blockUndo.vtxundo[transactionIndex - 1] : nullptr); - const TxReversalStatus status = UpdateCoinsReversingTransaction(tx, view, undo, pindex->nHeight); + const TxReversalStatus status = UpdateCoinsReversingTransaction(tx, txLocationReference, view, undo); if(!CheckTxReversalStatus(status,fClean)) { return false; } if(!pfClean) { - TransactionLocationReference txLocationReference(tx.GetHash(),pindex->nHeight,transactionIndex); IndexDatabaseUpdateCollector::ReverseTransaction(tx,txLocationReference,view,indexDBUpdates); } } diff --git a/divi/src/BlockTransactionChecker.cpp b/divi/src/BlockTransactionChecker.cpp index d2216f4d9..0873282e1 100644 --- a/divi/src/BlockTransactionChecker.cpp +++ b/divi/src/BlockTransactionChecker.cpp @@ -60,7 +60,7 @@ bool BlockTransactionChecker::Check(const CBlockRewards& nExpectedMint,bool fJus pindex_->nMint = 0; for (unsigned int i = 0; i < block_.vtx.size(); i++) { const CTransaction& tx = block_.vtx[i]; - const TransactionLocationReference txLocationRef(tx.GetHash(),pindex_->nHeight,i); + const TransactionLocationReference txLocationRef(tx, pindex_->nHeight, i); if(!txInputChecker_.TotalSigOpsAreBelowMaximum(tx)) { diff --git a/divi/src/IndexDatabaseUpdates.cpp b/divi/src/IndexDatabaseUpdates.cpp index b29a9896d..032a69c3c 100644 --- a/divi/src/IndexDatabaseUpdates.cpp +++ b/divi/src/IndexDatabaseUpdates.cpp @@ -1,5 +1,7 @@ #include +#include + IndexDatabaseUpdates::IndexDatabaseUpdates( ): addressIndex() , addressUnspentIndex() @@ -9,11 +11,11 @@ IndexDatabaseUpdates::IndexDatabaseUpdates( } TransactionLocationReference::TransactionLocationReference( - uint256 hashValue, + const CTransaction& tx, unsigned blockheightValue, int transactionIndexValue - ): hash(hashValue) + ): hash(tx.GetHash()) , blockHeight(blockheightValue) , transactionIndex(transactionIndexValue) { -} \ No newline at end of file +} diff --git a/divi/src/IndexDatabaseUpdates.h b/divi/src/IndexDatabaseUpdates.h index d66414cd1..0e77d08cc 100644 --- a/divi/src/IndexDatabaseUpdates.h +++ b/divi/src/IndexDatabaseUpdates.h @@ -6,6 +6,8 @@ #include #include +class CTransaction; + /** One entry in the tx index, which locates transactions on disk by their txid * or bare txid (both keys are possible). */ struct TxIndexEntry @@ -36,7 +38,7 @@ struct TransactionLocationReference int transactionIndex; TransactionLocationReference( - uint256 hashValue, + const CTransaction& tx, unsigned blockheightValue, int transactionIndexValue); }; diff --git a/divi/src/UtxoCheckingAndUpdating.cpp b/divi/src/UtxoCheckingAndUpdating.cpp index a497f6dd7..f2f094b6e 100644 --- a/divi/src/UtxoCheckingAndUpdating.cpp +++ b/divi/src/UtxoCheckingAndUpdating.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -29,7 +30,7 @@ void UpdateCoinsWithTransaction(const CTransaction& tx, CCoinsViewCache& inputs, static bool RemoveTxOutputsFromCache( const CTransaction& tx, - const int blockHeight, + const TransactionLocationReference& txLocationReference, CCoinsViewCache& view) { bool outputsAvailable = true; @@ -38,10 +39,10 @@ static bool RemoveTxOutputsFromCache( // have outputs available even in the block itself, so we handle that case // specially with outsEmpty. CCoins outsEmpty; - CCoinsModifier outs = view.ModifyCoins(tx.GetHash()); + CCoinsModifier outs = view.ModifyCoins(txLocationReference.hash); outs->ClearUnspendable(); - CCoins outsBlock(tx, blockHeight); + CCoins outsBlock(tx, txLocationReference.blockHeight); // The CCoins serialization does not serialize negative numbers. // No network rules currently depend on the version here, so an inconsistency is harmless // but it must be corrected before txout nversion ever influences a network rule. @@ -86,10 +87,10 @@ static void UpdateCoinsForRestoredInputs( coins->vout[out.n] = undo.txout; } -TxReversalStatus UpdateCoinsReversingTransaction(const CTransaction& tx, CCoinsViewCache& inputs, const CTxUndo* txundo, int nHeight) +TxReversalStatus UpdateCoinsReversingTransaction(const CTransaction& tx, const TransactionLocationReference& txLocationReference, CCoinsViewCache& inputs, const CTxUndo* txundo) { bool fClean = true; - fClean = fClean && RemoveTxOutputsFromCache(tx,nHeight,inputs); + fClean = fClean && RemoveTxOutputsFromCache(tx, txLocationReference, inputs); if(tx.IsCoinBase()) return fClean? TxReversalStatus::OK : TxReversalStatus::CONTINUE_WITH_ERRORS; assert(txundo != nullptr); if (txundo->vprevout.size() != tx.vin.size()) diff --git a/divi/src/UtxoCheckingAndUpdating.h b/divi/src/UtxoCheckingAndUpdating.h index dfa4fba5f..82ad3ac58 100644 --- a/divi/src/UtxoCheckingAndUpdating.h +++ b/divi/src/UtxoCheckingAndUpdating.h @@ -9,6 +9,8 @@ class CTransaction; class CValidationState; class CCoinsViewCache; class CTxUndo; +class TransactionLocationReference; + enum class TxReversalStatus { ABORT_NO_OTHER_ERRORS, @@ -17,7 +19,7 @@ enum class TxReversalStatus OK, }; void UpdateCoinsWithTransaction(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo& txundo, int nHeight); -TxReversalStatus UpdateCoinsReversingTransaction(const CTransaction& tx, CCoinsViewCache& inputs, const CTxUndo* txundo, int nHeight); +TxReversalStatus UpdateCoinsReversingTransaction(const CTransaction& tx, const TransactionLocationReference& txLocationReference, CCoinsViewCache& inputs, const CTxUndo* txundo); bool CheckInputs( const CTransaction& tx, CValidationState& state, From 401059efea042ef907087ffa31048fdf561f89d3 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 29 Jun 2021 16:01:24 +0200 Subject: [PATCH 2/3] Define CTxMemPool::lookupOutpoint. The memory pool mainly tracks transactions by txid; however, in some places, we actually need to look up the transaction by the output hash it creates (which will be different from the txid after segwit light). This introduces a new method CTxMemPool::lookupOutpoint, which is meant to be used for these situations (even though for now it does the same as lookup). It also updates the code to use the new method where this is appropriate. --- .../BlockMemoryPoolTransactionCollector.cpp | 2 +- divi/src/rpcblockchain.cpp | 5 +++-- divi/src/txmempool.cpp | 19 +++++++++++++++---- divi/src/txmempool.h | 4 ++++ 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/divi/src/BlockMemoryPoolTransactionCollector.cpp b/divi/src/BlockMemoryPoolTransactionCollector.cpp index 51d22f95f..1a7a82c32 100644 --- a/divi/src/BlockMemoryPoolTransactionCollector.cpp +++ b/divi/src/BlockMemoryPoolTransactionCollector.cpp @@ -206,7 +206,7 @@ std::vector BlockMemoryPoolTransactionCollector::PrioritizeMempoolTr // Read prev transaction if (!view.HaveCoins(txin.prevout.hash)) { CTransaction prevTx; - if(!mempool_.lookup(txin.prevout.hash, prevTx)) { + if(!mempool_.lookupOutpoint(txin.prevout.hash, prevTx)) { // This should never happen; all transactions in the memory // pool should connect to either transactions in the chain // or other transactions in the memory pool. diff --git a/divi/src/rpcblockchain.cpp b/divi/src/rpcblockchain.cpp index cec0b5f7d..6f7dc730d 100644 --- a/divi/src/rpcblockchain.cpp +++ b/divi/src/rpcblockchain.cpp @@ -218,8 +218,9 @@ Value getrawmempool(const Array& params, bool fHelp) info.push_back(Pair("currentpriority", e.GetPriority(chainActive.Height()))); const CTransaction& tx = e.GetTx(); set setDepends; - BOOST_FOREACH (const CTxIn& txin, tx.vin) { - if (mempool.exists(txin.prevout.hash)) + for (const CTxIn& txin : tx.vin) { + CTransaction dummyResult; + if (mempool.lookupOutpoint(txin.prevout.hash, dummyResult)) setDepends.insert(txin.prevout.hash.ToString()); } Array depends(setDepends.begin(), setDepends.end()); diff --git a/divi/src/txmempool.cpp b/divi/src/txmempool.cpp index 8e3455734..92f37ee0b 100644 --- a/divi/src/txmempool.cpp +++ b/divi/src/txmempool.cpp @@ -638,7 +638,7 @@ void CTxMemPool::removeCoinbaseSpends(const CCoinsViewCache* pcoins, unsigned in const CTransaction& tx = entry.second.GetTx(); for (const auto& txin : tx.vin) { CTransaction tx2; - if (lookup(txin.prevout.hash, tx2)) + if (lookupOutpoint(txin.prevout.hash, tx2)) continue; const CCoins* coins = pcoins->AccessCoins(txin.prevout.hash); if (fSanityCheck) assert(coins); @@ -723,7 +723,7 @@ void CTxMemPool::check(const CCoinsViewCache* pcoins, const BlockMap& blockIndex for (const auto& txin : tx.vin) { // Check that every mempool transaction's inputs refer to available coins, or other mempool tx's. CTransaction tx2; - if (lookup(txin.prevout.hash, tx2)) { + if (lookupOutpoint(txin.prevout.hash, tx2)) { assert(tx2.vout.size() > txin.prevout.n && !tx2.vout[txin.prevout.n].IsNull()); fDependsWait = true; } else { @@ -803,6 +803,13 @@ bool CTxMemPool::lookupBareTxid(const uint256& btxid, CTransaction& result) cons return true; } +bool CTxMemPool::lookupOutpoint(const uint256& hash, CTransaction& result) const +{ + /* For now (until we add the UTXO hasher and segwit light), the outpoint + is just the transaction ID. */ + return lookup(hash, result); +} + CFeeRate CTxMemPool::estimateFee(int nBlocks) const { LOCK(cs); @@ -882,7 +889,7 @@ bool CCoinsViewMemPool::GetCoins(const uint256& txid, CCoins& coins) const // conflict with the underlying cache, and it cannot have pruned entries (as it contains full) // transactions. First checking the underlying cache risks returning a pruned entry instead. CTransaction tx; - if (mempool.lookup(txid, tx)) { + if (mempool.lookupOutpoint(txid, tx)) { coins = CCoins(tx, MEMPOOL_HEIGHT); return true; } @@ -891,5 +898,9 @@ bool CCoinsViewMemPool::GetCoins(const uint256& txid, CCoins& coins) const bool CCoinsViewMemPool::HaveCoins(const uint256& txid) const { - return mempool.exists(txid) || base->HaveCoins(txid); + CTransaction dummy; + if (mempool.lookupOutpoint(txid, dummy)) + return true; + + return base->HaveCoins(txid); } diff --git a/divi/src/txmempool.h b/divi/src/txmempool.h index ca2f512db..4c55af44e 100644 --- a/divi/src/txmempool.h +++ b/divi/src/txmempool.h @@ -201,6 +201,10 @@ class CTxMemPool bool lookup(const uint256& hash, CTransaction& result) const; bool lookupBareTxid(const uint256& btxid, CTransaction& result) const; + /** Looks up a transaction by its outpoint for spending, taking potential changes + * from the raw txid (e.g. segwit light) into account. */ + bool lookupOutpoint(const uint256& hash, CTransaction& result) const; + /** Estimate fee rate needed to get into the next nBlocks */ CFeeRate estimateFee(int nBlocks) const; From 2b59d362f2388954f764c5dac229036f2bf809ad Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Wed, 30 Jun 2021 15:16:02 +0200 Subject: [PATCH 3/3] Mempool tests for lookup. This extends the mempool unit tests to explicitly verify lookup both by normal and bare txid, the two exists calls (with normal and bare txid) as well as the new lookupOutpoint method. --- divi/src/test/mempool_tests.cpp | 50 ++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/divi/src/test/mempool_tests.cpp b/divi/src/test/mempool_tests.cpp index 53db4b4ff..326072fe2 100644 --- a/divi/src/test/mempool_tests.cpp +++ b/divi/src/test/mempool_tests.cpp @@ -163,7 +163,7 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) removed.clear(); } -BOOST_AUTO_TEST_CASE(MempoolIndexByBareTxid) +BOOST_AUTO_TEST_CASE(MempoolDirectLookup) { CTransaction tx; std::list removed; @@ -172,14 +172,62 @@ BOOST_AUTO_TEST_CASE(MempoolIndexByBareTxid) BOOST_CHECK(testPool.lookupBareTxid(txParent.GetBareTxid(), tx)); BOOST_CHECK(tx.GetHash() == txParent.GetHash()); + BOOST_CHECK(testPool.lookup(txParent.GetHash(), tx)); + BOOST_CHECK(tx.GetHash() == txParent.GetHash()); + + BOOST_CHECK(!testPool.lookup(txParent.GetBareTxid(), tx)); BOOST_CHECK(!testPool.lookupBareTxid(txParent.GetHash(), tx)); testPool.remove(txParent, removed, true); + BOOST_CHECK(!testPool.lookup(txParent.GetHash(), tx)); + BOOST_CHECK(!testPool.lookup(txChild[0].GetHash(), tx)); + BOOST_CHECK(!testPool.lookup(txGrandChild[0].GetHash(), tx)); BOOST_CHECK(!testPool.lookupBareTxid(txParent.GetBareTxid(), tx)); BOOST_CHECK(!testPool.lookupBareTxid(txChild[0].GetBareTxid(), tx)); BOOST_CHECK(!testPool.lookupBareTxid(txGrandChild[0].GetBareTxid(), tx)); } +BOOST_AUTO_TEST_CASE(MempoolOutpointLookup) +{ + CTransaction tx; + CCoins c; + + AddAll(); + CCoinsViewMemPool viewPool(&coins, testPool); + + BOOST_CHECK(testPool.lookupOutpoint(txParent.GetHash(), tx)); + BOOST_CHECK(!testPool.lookupOutpoint(txParent.GetBareTxid(), tx)); + BOOST_CHECK(testPool.lookupOutpoint(txChild[0].GetHash(), tx)); + BOOST_CHECK(!testPool.lookupOutpoint(txChild[0].GetBareTxid(), tx)); + + BOOST_CHECK(viewPool.HaveCoins(txParent.GetHash())); + BOOST_CHECK(viewPool.GetCoins(txParent.GetHash(), c)); + BOOST_CHECK(!viewPool.HaveCoins(txParent.GetBareTxid())); + BOOST_CHECK(!viewPool.GetCoins(txParent.GetBareTxid(), c)); + + BOOST_CHECK(viewPool.HaveCoins(txChild[0].GetHash())); + BOOST_CHECK(viewPool.GetCoins(txChild[0].GetHash(), c)); + BOOST_CHECK(!viewPool.HaveCoins(txChild[0].GetBareTxid())); + BOOST_CHECK(!viewPool.GetCoins(txChild[0].GetBareTxid(), c)); +} + +BOOST_AUTO_TEST_CASE(MempoolExists) +{ + CTransaction tx; + std::list removed; + + AddAll(); + + BOOST_CHECK(testPool.exists(txParent.GetHash())); + BOOST_CHECK(!testPool.exists(txParent.GetBareTxid())); + BOOST_CHECK(!testPool.existsBareTxid(txParent.GetHash())); + BOOST_CHECK(testPool.existsBareTxid(txParent.GetBareTxid())); + + testPool.remove(txParent, removed, true); + BOOST_CHECK(!testPool.exists(txParent.GetHash())); + BOOST_CHECK(!testPool.existsBareTxid(txParent.GetBareTxid())); +} + BOOST_AUTO_TEST_CASE(MempoolSpentIndex) { spentIndex = true;