From da3da1bb1bf4f868642cc4ca5aed6f8e2e0d5f10 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 16 Mar 2021 15:38:42 +0100 Subject: [PATCH 1/5] Move internal functions into anonymous namespace. Wrap some of the functions used only in main into an anonymous namespace; some of them were already marked as "static", others were not (but they were still not needed from anywhere else). This makes it immediately clear that those functions are only used in the context of main. FindUndoPos had a declaration first and the definition later, but it is fine to just move the definition to where the declaration was. This simplifies the code further (and is a pure move in the context of this commit). --- divi/src/main.cpp | 69 ++++++++++++++++++++++++----------------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/divi/src/main.cpp b/divi/src/main.cpp index a66520311..717071c42 100755 --- a/divi/src/main.cpp +++ b/divi/src/main.cpp @@ -1202,7 +1202,10 @@ void Misbehaving(NodeId pnode, int howmuch) LogPrintf("Misbehaving: %s (%d -> %d)\n", state->name, state->nMisbehavior - howmuch, state->nMisbehavior); } -void static InvalidChainFound(CBlockIndex* pindexNew) +namespace +{ + +void InvalidChainFound(CBlockIndex* pindexNew) { if (!pindexBestInvalid || pindexNew->nChainWork > pindexBestInvalid->nChainWork) pindexBestInvalid = pindexNew; @@ -1217,7 +1220,7 @@ void static InvalidChainFound(CBlockIndex* pindexNew) CheckForkWarningConditions(); } -void static InvalidBlockFound(CBlockIndex* pindex, const CValidationState& state) +void InvalidBlockFound(CBlockIndex* pindex, const CValidationState& state) { int nDoS = 0; if (state.IsInvalid(nDoS)) { @@ -1237,7 +1240,7 @@ void static InvalidBlockFound(CBlockIndex* pindex, const CValidationState& state } } -void static FlushBlockFile(bool fFinalize = false) +void FlushBlockFile(bool fFinalize = false) { LOCK(cs_LastBlockFile); @@ -1260,9 +1263,35 @@ void static FlushBlockFile(bool fFinalize = false) } } -bool FindUndoPos(CValidationState& state, int nFile, CDiskBlockPos& pos, unsigned int nAddSize); +bool FindUndoPos(CValidationState& state, int nFile, CDiskBlockPos& pos, unsigned int nAddSize) +{ + pos.nFile = nFile; + + LOCK(cs_LastBlockFile); + + unsigned int nNewSize; + pos.nPos = vinfoBlockFile[nFile].nUndoSize; + nNewSize = vinfoBlockFile[nFile].nUndoSize += nAddSize; + setDirtyFileInfo.insert(nFile); + + unsigned int nOldChunks = (pos.nPos + UNDOFILE_CHUNK_SIZE - 1) / UNDOFILE_CHUNK_SIZE; + unsigned int nNewChunks = (nNewSize + UNDOFILE_CHUNK_SIZE - 1) / UNDOFILE_CHUNK_SIZE; + if (nNewChunks > nOldChunks) { + if (CheckDiskSpace(nNewChunks * UNDOFILE_CHUNK_SIZE - pos.nPos)) { + FILE* file = OpenUndoFile(pos); + if (file) { + LogPrintf("Pre-allocating up to position 0x%x in rev%05u.dat\n", nNewChunks * UNDOFILE_CHUNK_SIZE, pos.nFile); + AllocateFileRange(file, pos.nPos, nNewChunks * UNDOFILE_CHUNK_SIZE - pos.nPos); + fclose(file); + } + } else + return state.Error("out of disk space"); + } + + return true; +} -static int64_t nTimeTotal = 0; +int64_t nTimeTotal = 0; void VerifyBestBlockIsAtPreviousBlock(const CBlockIndex* pindex, CCoinsViewCache& view) { @@ -1391,6 +1420,8 @@ bool UpdateDBIndicesForNewBlock( return true; } +} // anonymous namespace + bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, bool fJustCheck, bool fAlreadyChecked) { AssertLockHeld(cs_main); @@ -2140,34 +2171,6 @@ bool FindBlockPos(CValidationState& state, CDiskBlockPos& pos, unsigned int nAdd return true; } -bool FindUndoPos(CValidationState& state, int nFile, CDiskBlockPos& pos, unsigned int nAddSize) -{ - pos.nFile = nFile; - - LOCK(cs_LastBlockFile); - - unsigned int nNewSize; - pos.nPos = vinfoBlockFile[nFile].nUndoSize; - nNewSize = vinfoBlockFile[nFile].nUndoSize += nAddSize; - setDirtyFileInfo.insert(nFile); - - unsigned int nOldChunks = (pos.nPos + UNDOFILE_CHUNK_SIZE - 1) / UNDOFILE_CHUNK_SIZE; - unsigned int nNewChunks = (nNewSize + UNDOFILE_CHUNK_SIZE - 1) / UNDOFILE_CHUNK_SIZE; - if (nNewChunks > nOldChunks) { - if (CheckDiskSpace(nNewChunks * UNDOFILE_CHUNK_SIZE - pos.nPos)) { - FILE* file = OpenUndoFile(pos); - if (file) { - LogPrintf("Pre-allocating up to position 0x%x in rev%05u.dat\n", nNewChunks * UNDOFILE_CHUNK_SIZE, pos.nFile); - AllocateFileRange(file, pos.nPos, nNewChunks * UNDOFILE_CHUNK_SIZE - pos.nPos); - fclose(file); - } - } else - return state.Error("out of disk space"); - } - - return true; -} - bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckMerkleRoot) { // These are checks that are independent of context. From 4c491ecdb4ffb2645fa758c6d0d780c4cf8cfd38 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Wed, 7 Apr 2021 14:20:40 +0200 Subject: [PATCH 2/5] Clean up some mn config code. This is a bunch of related and straight-forward cleanups to the code around masternode configuration: Pass arguments as const& instead of by value, mark functions as const, and move helper functions from being static in a class and declared inside the header to just being inside an anonymous namespace in the cpp file. --- divi/src/MasternodeBroadcastFactory.cpp | 35 ++++++++------- divi/src/MasternodeBroadcastFactory.h | 60 +++++++------------------ divi/src/masternode-sync.h | 2 +- divi/src/masternode.cpp | 5 ++- divi/src/masternode.h | 16 ++++--- divi/src/masternodeconfig.cpp | 12 ++--- divi/src/masternodeconfig.h | 9 ++-- 7 files changed, 63 insertions(+), 76 deletions(-) diff --git a/divi/src/MasternodeBroadcastFactory.cpp b/divi/src/MasternodeBroadcastFactory.cpp index fac4d3730..7207e8863 100644 --- a/divi/src/MasternodeBroadcastFactory.cpp +++ b/divi/src/MasternodeBroadcastFactory.cpp @@ -17,7 +17,10 @@ extern CChain chainActive; extern bool fReindex; extern bool fImporting; -bool CMasternodeBroadcastFactory::checkBlockchainSync(std::string& strErrorRet, bool fOffline) +namespace +{ + +bool checkBlockchainSync(std::string& strErrorRet, bool fOffline) { if (!fOffline && !IsBlockchainSynced()) { strErrorRet = "Sync in progress. Must wait until sync is complete to start Masternode"; @@ -26,7 +29,7 @@ bool CMasternodeBroadcastFactory::checkBlockchainSync(std::string& strErrorRet, } return true; } -bool CMasternodeBroadcastFactory::setMasternodeKeys( +bool setMasternodeKeys( const std::string& strKeyMasternode, std::pair& masternodeKeyPair, std::string& strErrorRet) @@ -38,7 +41,7 @@ bool CMasternodeBroadcastFactory::setMasternodeKeys( } return true; } -bool CMasternodeBroadcastFactory::setMasternodeCollateralKeys( +bool setMasternodeCollateralKeys( const std::string& txHash, const std::string& outputIndex, const std::string& service, @@ -63,7 +66,7 @@ bool CMasternodeBroadcastFactory::setMasternodeCollateralKeys( return true; } -bool CMasternodeBroadcastFactory::checkMasternodeCollateral( +bool checkMasternodeCollateral( const CTxIn& txin, const std::string& txHash, const std::string& outputIndex, @@ -95,7 +98,7 @@ bool CMasternodeBroadcastFactory::checkMasternodeCollateral( return true; } -bool CMasternodeBroadcastFactory::createArgumentsFromConfig( +bool createArgumentsFromConfig( const CMasternodeConfig::CMasternodeEntry configEntry, std::string& strErrorRet, bool fOffline, @@ -121,6 +124,8 @@ bool CMasternodeBroadcastFactory::createArgumentsFromConfig( return true; } +} // anonymous namespace + bool CMasternodeBroadcastFactory::Create( const CMasternodeConfig::CMasternodeEntry configEntry, CPubKey pubkeyCollateralAddress, @@ -278,10 +283,10 @@ CMasternodePing createDelayedMasternodePing(const CMasternodeBroadcast& mnb) } // anonymous namespace void CMasternodeBroadcastFactory::createWithoutSignatures( - CTxIn txin, - CService service, - CPubKey pubKeyCollateralAddressNew, - CPubKey pubKeyMasternodeNew, + const CTxIn& txin, + const CService& service, + const CPubKey& pubKeyCollateralAddressNew, + const CPubKey& pubKeyMasternodeNew, const MasternodeTier nMasternodeTier, bool deferRelay, CMasternodeBroadcast& mnbRet) @@ -299,12 +304,12 @@ void CMasternodeBroadcastFactory::createWithoutSignatures( } bool CMasternodeBroadcastFactory::Create( - CTxIn txin, - CService service, - CKey keyCollateralAddressNew, - CPubKey pubKeyCollateralAddressNew, - CKey keyMasternodeNew, - CPubKey pubKeyMasternodeNew, + const CTxIn& txin, + const CService& service, + const CKey& keyCollateralAddressNew, + const CPubKey& pubKeyCollateralAddressNew, + const CKey& keyMasternodeNew, + const CPubKey& pubKeyMasternodeNew, const MasternodeTier nMasternodeTier, std::string& strErrorRet, CMasternodeBroadcast& mnbRet, diff --git a/divi/src/MasternodeBroadcastFactory.h b/divi/src/MasternodeBroadcastFactory.h index e7edd478a..2f576f060 100644 --- a/divi/src/MasternodeBroadcastFactory.h +++ b/divi/src/MasternodeBroadcastFactory.h @@ -37,10 +37,10 @@ class CMasternodeBroadcastFactory std::string& strErrorRet); private: static void createWithoutSignatures( - CTxIn txin, - CService service, - CPubKey pubKeyCollateralAddressNew, - CPubKey pubKeyMasternodeNew, + const CTxIn& txin, + const CService& service, + const CPubKey& pubKeyCollateralAddressNew, + const CPubKey& pubKeyMasternodeNew, MasternodeTier nMasternodeTier, bool deferRelay, CMasternodeBroadcast& mnbRet); @@ -57,45 +57,15 @@ class CMasternodeBroadcastFactory CMasternodeBroadcast& mnb, std::string& strErrorRet); - static bool Create(CTxIn vin, - CService service, - CKey keyCollateralAddressNew, - CPubKey pubKeyCollateralAddressNew, - CKey keyMasternodeNew, - CPubKey pubKeyMasternodeNew, - MasternodeTier nMasternodeTier, - std::string& strErrorRet, - CMasternodeBroadcast& mnbRet, - bool deferRelay); - static bool checkBlockchainSync( - std::string& strErrorRet, bool fOffline); - static bool setMasternodeKeys( - const std::string& strKeyMasternode, - std::pair& masternodeKeyPair, - std::string& strErrorRet); - static bool setMasternodeCollateralKeys( - const std::string& txHash, - const std::string& outputIndex, - const std::string& service, - bool collateralPrivKeyIsRemote, - CTxIn& txin, - std::pair& masternodeCollateralKeyPair, - std::string& error); - static bool checkMasternodeCollateral( - const CTxIn& txin, - const std::string& txHash, - const std::string& outputIndex, - const std::string& service, - MasternodeTier& nMasternodeTier, - std::string& strErrorRet); - static bool createArgumentsFromConfig( - const CMasternodeConfig::CMasternodeEntry configEntry, - std::string& strErrorRet, - bool fOffline, - bool collateralPrivKeyIsRemote, - CTxIn& txin, - std::pair& masternodeKeyPair, - std::pair& masternodeCollateralKeyPair, - MasternodeTier& nMasternodeTier); + static bool Create(const CTxIn& vin, + const CService& service, + const CKey& keyCollateralAddressNew, + const CPubKey& pubKeyCollateralAddressNew, + const CKey& keyMasternodeNew, + const CPubKey& pubKeyMasternodeNew, + MasternodeTier nMasternodeTier, + std::string& strErrorRet, + CMasternodeBroadcast& mnbRet, + bool deferRelay); }; -#endif //MASTERNODE_BROADCAST_FACTORY_H \ No newline at end of file +#endif //MASTERNODE_BROADCAST_FACTORY_H diff --git a/divi/src/masternode-sync.h b/divi/src/masternode-sync.h index 1ce4a3b33..c15d852e1 100644 --- a/divi/src/masternode-sync.h +++ b/divi/src/masternode-sync.h @@ -85,7 +85,7 @@ class CMasternodeSync bool MasternodeWinnersListIsSync(CNode* pnode, const int64_t now); void Process(bool networkIsRegtest); bool IsSynced() const; - bool IsMasternodeListSynced() { return RequestedMasternodeAssets > MASTERNODE_SYNC_LIST; } + bool IsMasternodeListSynced() const { return RequestedMasternodeAssets > MASTERNODE_SYNC_LIST; } void AskForMN(CNode* pnode, const CTxIn& vin) const; }; diff --git a/divi/src/masternode.cpp b/divi/src/masternode.cpp index d2391d2c2..cf457f42b 100644 --- a/divi/src/masternode.cpp +++ b/divi/src/masternode.cpp @@ -258,7 +258,10 @@ bool CMasternode::IsValidNetAddr() const (IsReachable(addr) && addr.IsRoutable()); } -CMasternodeBroadcast::CMasternodeBroadcast(CService newAddr, CTxIn newVin, CPubKey pubKeyCollateralAddressNew, CPubKey pubKeyMasternodeNew, const MasternodeTier nMasternodeTier, int protocolVersionIn) +CMasternodeBroadcast::CMasternodeBroadcast( + const CService& newAddr, const CTxIn& newVin, + const CPubKey& pubKeyCollateralAddressNew, const CPubKey& pubKeyMasternodeNew, + const MasternodeTier nMasternodeTier, const int protocolVersionIn) { vin = newVin; addr = newAddr; diff --git a/divi/src/masternode.h b/divi/src/masternode.h index f987234c1..b561097fc 100644 --- a/divi/src/masternode.h +++ b/divi/src/masternode.h @@ -146,15 +146,19 @@ class CMasternode class CMasternodeBroadcast : public CMasternode { -public: - CMasternodeBroadcast() = default; +private: CMasternodeBroadcast( - CService newAddr, - CTxIn newVin, - CPubKey pubKeyCollateralAddress, - CPubKey pubKeyMasternode, + const CService& newAddr, + const CTxIn& newVin, + const CPubKey& pubKeyCollateralAddress, + const CPubKey& pubKeyMasternode, MasternodeTier nMasternodeTier, int protocolVersionIn); + + friend class CMasternodeBroadcastFactory; + +public: + CMasternodeBroadcast() = default; CMasternodeBroadcast(const CMasternode& mn); void Relay() const; diff --git a/divi/src/masternodeconfig.cpp b/divi/src/masternodeconfig.cpp index 91b9debb9..c48f8304d 100644 --- a/divi/src/masternodeconfig.cpp +++ b/divi/src/masternodeconfig.cpp @@ -23,10 +23,11 @@ boost::filesystem::path GetMasternodeConfigFile() if (!pathConfigFile.is_complete()) pathConfigFile = GetDataDir() / pathConfigFile; return pathConfigFile; } -void CMasternodeConfig::add(std::string alias, std::string ip, std::string privKey, std::string txHash, std::string outputIndex) + +void CMasternodeConfig::add(const std::string& alias, const std::string& ip, const std::string& privKey, + const std::string& txHash, const std::string& outputIndex) { - CMasternodeEntry cme(alias, ip, privKey, txHash, outputIndex); - entries.push_back(cme); + entries.emplace_back(alias, ip, privKey, txHash, outputIndex); } bool CMasternodeConfig::read(std::string& strErr) @@ -94,15 +95,16 @@ CMasternodeConfig::CMasternodeConfig() { entries = std::vector(); } + const std::vector& CMasternodeConfig::getEntries() const { return entries; } -int CMasternodeConfig::getCount() +int CMasternodeConfig::getCount() const { int c = -1; - BOOST_FOREACH (CMasternodeEntry e, entries) { + for (const auto& e : entries) { if (e.getAlias() != "") c++; } return c; diff --git a/divi/src/masternodeconfig.h b/divi/src/masternodeconfig.h index d5ac185d9..2d34edb4a 100644 --- a/divi/src/masternodeconfig.h +++ b/divi/src/masternodeconfig.h @@ -25,7 +25,9 @@ class CMasternodeConfig std::string outputIndex; public: - CMasternodeEntry(std::string alias, std::string ip, std::string privKey, std::string txHash, std::string outputIndex) + CMasternodeEntry(const std::string& alias, const std::string& ip, + const std::string& privKey, + const std::string& txHash, const std::string& outputIndex) { this->alias = alias; this->ip = ip; @@ -91,9 +93,10 @@ class CMasternodeConfig void clear(); bool read(std::string& strErr); - void add(std::string alias, std::string ip, std::string privKey, std::string txHash, std::string outputIndex); + void add(const std::string& alias, const std::string& ip, const std::string& privKey, + const std::string& txHash, const std::string& outputIndex); const std::vector& getEntries() const; - int getCount(); + int getCount() const; private: std::vector entries; From aba067e25a56f1a0613dd013eae7147457e44163 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 19 Jan 2021 13:17:29 +0100 Subject: [PATCH 3/5] Add TransactionUtxoHasher abstraction. Some places in the code need to determine the hash by which the UTXOs of a given transaction will be referred to; in particular, we need that when processing UTXO set updates for block connects and disconnects, in the mempool and for assembling transactions into a new block. This commit introduces a class TransactionUtxoHasher, which abstracts this step and is used in all those places in the code instead of just getting the txid directly. For now, this has no effects on behaviour; but it makes it more clear in the code where we need this particular logical feature; it will allow us to add some more unit tests for those parts with explicit mocks of the hasher class; and it will make it easier to implement segwit-light in the future (where we basically just need to flip the hasher implementation but no other parts in the code). --- divi/src/ActiveChainManager.cpp | 8 ++- .../BlockMemoryPoolTransactionCollector.cpp | 6 +- divi/src/BlockTransactionChecker.cpp | 8 ++- divi/src/BlockTransactionChecker.h | 5 +- divi/src/IndexDatabaseUpdateCollector.cpp | 6 +- divi/src/IndexDatabaseUpdates.cpp | 11 +++- divi/src/IndexDatabaseUpdates.h | 9 ++- divi/src/UtxoCheckingAndUpdating.cpp | 23 +++++--- divi/src/UtxoCheckingAndUpdating.h | 50 +++++++++++++++- divi/src/main.cpp | 13 +++-- divi/src/txmempool.cpp | 58 ++++++++++++++++--- divi/src/txmempool.h | 11 ++++ 12 files changed, 166 insertions(+), 42 deletions(-) diff --git a/divi/src/ActiveChainManager.cpp b/divi/src/ActiveChainManager.cpp index 62e6b476d..867074ef7 100644 --- a/divi/src/ActiveChainManager.cpp +++ b/divi/src/ActiveChainManager.cpp @@ -87,19 +87,21 @@ bool ActiveChainManager::DisconnectBlock( return error("DisconnectBlock() : block and undo data inconsistent"); } + const BlockUtxoHasher utxoHasher; + bool fClean = true; IndexDatabaseUpdates indexDBUpdates; // undo transactions in reverse order for (int transactionIndex = block.vtx.size() - 1; transactionIndex >= 0; transactionIndex--) { const CTransaction& tx = block.vtx[transactionIndex]; - const TxReversalStatus status = UpdateCoinsReversingTransaction(tx,view,blockUndo.vtxundo[transactionIndex-1],pindex->nHeight); + const TransactionLocationReference txLocationReference(utxoHasher, tx, pindex->nHeight, transactionIndex); + const TxReversalStatus status = UpdateCoinsReversingTransaction(tx, txLocationReference, view, blockUndo.vtxundo[transactionIndex-1]); if(!CheckTxReversalStatus(status,fClean)) { return false; } if(!pfClean) { - TransactionLocationReference txLocationReference(tx.GetHash(),pindex->nHeight,transactionIndex); IndexDatabaseUpdateCollector::ReverseTransaction(tx,txLocationReference,view,indexDBUpdates); } } @@ -136,4 +138,4 @@ void ActiveChainManager::DisconnectBlock( int64_t nStart = GetTimeMicros(); status = DisconnectBlock(block,state,pindex,coins); LogPrint("bench", "- Disconnect block: %.2fms\n", (GetTimeMicros() - nStart) * 0.001); -} \ No newline at end of file +} diff --git a/divi/src/BlockMemoryPoolTransactionCollector.cpp b/divi/src/BlockMemoryPoolTransactionCollector.cpp index 4ba830f52..1407823f1 100644 --- a/divi/src/BlockMemoryPoolTransactionCollector.cpp +++ b/divi/src/BlockMemoryPoolTransactionCollector.cpp @@ -203,7 +203,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. @@ -338,7 +338,7 @@ std::vector BlockMemoryPoolTransactionCollector::Pri nBlockSigOps += nTxSigOps; CTxUndo txundo; - UpdateCoinsWithTransaction(tx, view, txundo, nHeight); + UpdateCoinsWithTransaction(tx, view, txundo, mempool_.GetUtxoHasher(), nHeight); if (fPrintPriority) { LogPrintf("priority %.1f fee %s txid %s\n", @@ -346,7 +346,7 @@ std::vector BlockMemoryPoolTransactionCollector::Pri } // Add transactions that depend on this one to the priority queue - AddDependingTransactionsToPriorityQueue(dependentTransactions, hash, vecPriority, comparer); + AddDependingTransactionsToPriorityQueue(dependentTransactions, mempool_.GetUtxoHasher().GetUtxoHash(tx), vecPriority, comparer); } LogPrintf("CreateNewBlock(): total size %u\n", nBlockSize); diff --git a/divi/src/BlockTransactionChecker.cpp b/divi/src/BlockTransactionChecker.cpp index 7b75aaae3..98fa9793e 100644 --- a/divi/src/BlockTransactionChecker.cpp +++ b/divi/src/BlockTransactionChecker.cpp @@ -38,12 +38,14 @@ void TransactionLocationRecorder::RecordTxLocationData( BlockTransactionChecker::BlockTransactionChecker( const CBlock& block, + const TransactionUtxoHasher& utxoHasher, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, const int blocksToSkipChecksFor ): blockundo_(block.vtx.size() - 1) , block_(block) + , utxoHasher_(utxoHasher) , state_(state) , pindex_(pindex) , view_(view) @@ -59,7 +61,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(utxoHasher_, tx, pindex_->nHeight, i); if(!txInputChecker_.TotalSigOpsAreBelowMaximum(tx)) { @@ -79,7 +81,7 @@ bool BlockTransactionChecker::Check(const CBlockRewards& nExpectedMint,bool fJus } IndexDatabaseUpdateCollector::RecordTransaction(tx,txLocationRef,view_, indexDatabaseUpdates); - UpdateCoinsWithTransaction(tx, view_, blockundo_.vtxundo[i>0u? i-1: 0u], pindex_->nHeight); + UpdateCoinsWithTransaction(tx, view_, blockundo_.vtxundo[i>0u? i-1: 0u], utxoHasher_, pindex_->nHeight); txLocationRecorder_.RecordTxLocationData(tx,indexDatabaseUpdates.txLocationData); } return true; @@ -93,4 +95,4 @@ bool BlockTransactionChecker::WaitForScriptsToBeChecked() CBlockUndo& BlockTransactionChecker::getBlockUndoData() { return blockundo_; -} \ No newline at end of file +} diff --git a/divi/src/BlockTransactionChecker.h b/divi/src/BlockTransactionChecker.h index 68bbbf4a0..43335788d 100644 --- a/divi/src/BlockTransactionChecker.h +++ b/divi/src/BlockTransactionChecker.h @@ -13,6 +13,7 @@ class CBlock; class CValidationState; class CCoinsViewCache; class CBlockRewards; +class TransactionUtxoHasher; class TransactionLocationRecorder { @@ -35,6 +36,7 @@ class BlockTransactionChecker private: CBlockUndo blockundo_; const CBlock& block_; + const TransactionUtxoHasher& utxoHasher_; CValidationState& state_; CBlockIndex* pindex_; CCoinsViewCache& view_; @@ -43,6 +45,7 @@ class BlockTransactionChecker public: BlockTransactionChecker( const CBlock& block, + const TransactionUtxoHasher& utxoHasher, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, @@ -56,4 +59,4 @@ class BlockTransactionChecker CBlockUndo& getBlockUndoData(); }; -#endif// BLOCK_TRANSACTION_CHECKER_H \ No newline at end of file +#endif// BLOCK_TRANSACTION_CHECKER_H diff --git a/divi/src/IndexDatabaseUpdateCollector.cpp b/divi/src/IndexDatabaseUpdateCollector.cpp index 8d0a8466c..73eb56909 100644 --- a/divi/src/IndexDatabaseUpdateCollector.cpp +++ b/divi/src/IndexDatabaseUpdateCollector.cpp @@ -81,7 +81,7 @@ void CollectUpdatesFromOutputs( std::make_pair(CAddressIndexKey(addressType, uint160(hashBytes), txLocationRef.blockHeight, txLocationRef.transactionIndex, txLocationRef.hash, k, false), out.nValue)); // record unspent output indexDatabaseUpdates.addressUnspentIndex.push_back( - std::make_pair(CAddressUnspentKey(addressType, uint160(hashBytes), txLocationRef.hash, k), CAddressUnspentValue(out.nValue, out.scriptPubKey, txLocationRef.blockHeight))); + std::make_pair(CAddressUnspentKey(addressType, uint160(hashBytes), txLocationRef.utxoHash, k), CAddressUnspentValue(out.nValue, out.scriptPubKey, txLocationRef.blockHeight))); } else { continue; } @@ -156,7 +156,7 @@ static void CollectUpdatesFromOutputs( // undo unspent index indexDBUpdates.addressUnspentIndex.push_back( std::make_pair( - CAddressUnspentKey(addressType, uint160(hashBytes), txLocationReference.hash, k), + CAddressUnspentKey(addressType, uint160(hashBytes), txLocationReference.utxoHash, k), CAddressUnspentValue())); } @@ -182,4 +182,4 @@ void IndexDatabaseUpdateCollector::ReverseTransaction( { ReverseSpending::CollectUpdatesFromOutputs(tx,txLocationRef,indexDatabaseUpdates); ReverseSpending::CollectUpdatesFromInputs(tx,txLocationRef,view, indexDatabaseUpdates); -} \ No newline at end of file +} diff --git a/divi/src/IndexDatabaseUpdates.cpp b/divi/src/IndexDatabaseUpdates.cpp index b29a9896d..7ed210ee0 100644 --- a/divi/src/IndexDatabaseUpdates.cpp +++ b/divi/src/IndexDatabaseUpdates.cpp @@ -1,5 +1,8 @@ #include +#include +#include + IndexDatabaseUpdates::IndexDatabaseUpdates( ): addressIndex() , addressUnspentIndex() @@ -9,11 +12,13 @@ IndexDatabaseUpdates::IndexDatabaseUpdates( } TransactionLocationReference::TransactionLocationReference( - uint256 hashValue, + const TransactionUtxoHasher& utxoHasher, + const CTransaction& tx, unsigned blockheightValue, int transactionIndexValue - ): hash(hashValue) + ): hash(tx.GetHash()) + , utxoHash(utxoHasher.GetUtxoHash(tx)) , blockHeight(blockheightValue) , transactionIndex(transactionIndexValue) { -} \ No newline at end of file +} diff --git a/divi/src/IndexDatabaseUpdates.h b/divi/src/IndexDatabaseUpdates.h index 677130e3a..a724b1a7e 100644 --- a/divi/src/IndexDatabaseUpdates.h +++ b/divi/src/IndexDatabaseUpdates.h @@ -6,6 +6,9 @@ #include #include +class CTransaction; +class TransactionUtxoHasher; + struct IndexDatabaseUpdates { std::vector > addressIndex; @@ -19,12 +22,14 @@ struct IndexDatabaseUpdates struct TransactionLocationReference { uint256 hash; + uint256 utxoHash; unsigned blockHeight; int transactionIndex; TransactionLocationReference( - uint256 hashValue, + const TransactionUtxoHasher& utxoHasher, + const CTransaction& tx, unsigned blockheightValue, int transactionIndexValue); }; -#endif// INDEX_DATABASE_UPDATES_H \ No newline at end of file +#endif// INDEX_DATABASE_UPDATES_H diff --git a/divi/src/UtxoCheckingAndUpdating.cpp b/divi/src/UtxoCheckingAndUpdating.cpp index 26e71d9aa..b0aeef600 100644 --- a/divi/src/UtxoCheckingAndUpdating.cpp +++ b/divi/src/UtxoCheckingAndUpdating.cpp @@ -10,15 +10,22 @@ #include #include #include +#include extern BlockMap mapBlockIndex; -void UpdateCoinsWithTransaction(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo& txundo, int nHeight) +uint256 BlockUtxoHasher::GetUtxoHash(const CTransaction& tx) const +{ + return tx.GetHash(); +} + +void UpdateCoinsWithTransaction(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo& txundo, + const TransactionUtxoHasher& hasher, const int nHeight) { // mark inputs spent if (!tx.IsCoinBase() ) { txundo.vprevout.reserve(tx.vin.size()); - BOOST_FOREACH (const CTxIn& txin, tx.vin) { + for (const auto& txin : tx.vin) { txundo.vprevout.push_back(CTxInUndo()); bool ret = inputs.ModifyCoins(txin.prevout.hash)->Spend(txin.prevout.n, txundo.vprevout.back()); assert(ret); @@ -26,12 +33,12 @@ void UpdateCoinsWithTransaction(const CTransaction& tx, CCoinsViewCache& inputs, } // add outputs - inputs.ModifyCoins(tx.GetHash())->FromTx(tx, nHeight); + inputs.ModifyCoins(hasher.GetUtxoHash(tx))->FromTx(tx, nHeight); } static bool RemoveTxOutputsFromCache( const CTransaction& tx, - const int blockHeight, + const TransactionLocationReference& txLocationReference, CCoinsViewCache& view) { bool outputsAvailable = true; @@ -40,10 +47,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.utxoHash); 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. @@ -88,10 +95,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; if (txundo.vprevout.size() != tx.vin.size()) { diff --git a/divi/src/UtxoCheckingAndUpdating.h b/divi/src/UtxoCheckingAndUpdating.h index e68ef7177..318286292 100644 --- a/divi/src/UtxoCheckingAndUpdating.h +++ b/divi/src/UtxoCheckingAndUpdating.h @@ -3,11 +3,14 @@ #include #include #include +#include class CTransaction; class CValidationState; class CCoinsViewCache; class CTxUndo; +class TransactionLocationReference; + enum class TxReversalStatus { ABORT_NO_OTHER_ERRORS, @@ -15,8 +18,49 @@ enum class TxReversalStatus CONTINUE_WITH_ERRORS, OK, }; -void UpdateCoinsWithTransaction(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo& txundo, int nHeight); -TxReversalStatus UpdateCoinsReversingTransaction(const CTransaction& tx, CCoinsViewCache& inputs, const CTxUndo& txundo, int nHeight); + +/** Implementations of this interface translate transactions into the hashes + * that will be used to refer to the UTXOs their outputs create. + * + * This class abstracts away the segwit-light fork and its activation + * from the places that need to refer to / update UTXOs. + * + * For unit tests, this class can be subclassed and mocked. */ +class TransactionUtxoHasher +{ + +public: + + TransactionUtxoHasher() = default; + virtual ~TransactionUtxoHasher() = default; + + TransactionUtxoHasher(const TransactionUtxoHasher&) = delete; + void operator=(const TransactionUtxoHasher&) = delete; + + virtual uint256 GetUtxoHash(const CTransaction& tx) const = 0; + +}; + +/** A TransactionUtxoHasher for transactions contained in a particular + * block, e.g. for processing that block's connect or disconnect. Initially + * these are just the txid (as also with upstream Bitcoin), but for + * segwit-light, they are changed to the bare txid. + * + * This class abstracts away the segwit-light fork and its activation + * from the places that need to refer to / update UTXOs. + * + * For unit tests, this class can be subclassed and mocked. */ +class BlockUtxoHasher : public TransactionUtxoHasher +{ + +public: + + uint256 GetUtxoHash(const CTransaction& tx) const override; + +}; + +void UpdateCoinsWithTransaction(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo& txundo, const TransactionUtxoHasher& hasher, int nHeight); +TxReversalStatus UpdateCoinsReversingTransaction(const CTransaction& tx, const TransactionLocationReference& txLocationReference, CCoinsViewCache& inputs, const CTxUndo& txundo); bool CheckInputs( const CTransaction& tx, CValidationState& state, @@ -36,4 +80,4 @@ bool CheckInputs( bool cacheStore, std::vector* pvChecks = nullptr, bool connectBlockDoS = false); -#endif// UTXO_CHECKING_AND_UPDATING_H \ No newline at end of file +#endif// UTXO_CHECKING_AND_UPDATING_H diff --git a/divi/src/main.cpp b/divi/src/main.cpp index 717071c42..091e9928c 100755 --- a/divi/src/main.cpp +++ b/divi/src/main.cpp @@ -1301,7 +1301,7 @@ void VerifyBestBlockIsAtPreviousBlock(const CBlockIndex* pindex, CCoinsViewCache assert(hashPrevBlock == view.GetBestBlock()); } -bool CheckEnforcedPoSBlocksAndBIP30(const CChainParams& chainParameters, const CBlock& block, CValidationState& state, const CBlockIndex* pindex, const CCoinsViewCache& view) +bool CheckEnforcedPoSBlocksAndBIP30(const CChainParams& chainParameters, const TransactionUtxoHasher& utxoHasher, const CBlock& block, CValidationState& state, const CBlockIndex* pindex, const CCoinsViewCache& view) { if (pindex->nHeight <= chainParameters.LAST_POW_BLOCK() && block.IsProofOfStake()) return state.DoS(100, error("%s : PoS period not active",__func__), @@ -1313,7 +1313,7 @@ bool CheckEnforcedPoSBlocksAndBIP30(const CChainParams& chainParameters, const C // Enforce BIP30. for (const auto& tx : block.vtx) { - const CCoins* coins = view.AccessCoins(tx.GetHash()); + const CCoins* coins = view.AccessCoins(utxoHasher.GetUtxoHash(tx)); if (coins && !coins->IsPruned()) return state.DoS(100, error("%s : tried to overwrite transaction",__func__), REJECT_INVALID, "bad-txns-BIP30"); @@ -1433,6 +1433,8 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin LogWalletBalance(pwalletMain); static const CChainParams& chainParameters = Params(); + const BlockUtxoHasher utxoHasher; + VerifyBestBlockIsAtPreviousBlock(pindex,view); if (block.GetHash() == Params().HashGenesisBlock()) { @@ -1440,7 +1442,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin view.SetBestBlock(pindex->GetBlockHash()); return true; } - if(!CheckEnforcedPoSBlocksAndBIP30(chainParameters,block,state,pindex,view)) + if(!CheckEnforcedPoSBlocksAndBIP30(chainParameters,utxoHasher,block,state,pindex,view)) { return false; } @@ -1458,7 +1460,7 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin const int blocksToSkipChecksFor = checkpointsVerifier.GetTotalBlocksEstimate(); IndexDatabaseUpdates indexDatabaseUpdates; CBlockRewards nExpectedMint = subsidiesContainer.blockSubsidiesProvider().GetBlockSubsidity(pindex->nHeight); - BlockTransactionChecker blockTxChecker(block,state,pindex,view,blocksToSkipChecksFor); + BlockTransactionChecker blockTxChecker(block, utxoHasher, state, pindex, view, blocksToSkipChecksFor); if(!blockTxChecker.Check(nExpectedMint,fJustCheck,indexDatabaseUpdates)) { @@ -2737,6 +2739,7 @@ bool static LoadBlockIndexDB(string& strError) strError = "The wallet has been not been closed gracefully and has caused corruption of blocks stored to disk. Data directory is in an unusable state"; return false; } + const BlockUtxoHasher utxoHasher; std::vector vtxundo; vtxundo.reserve(block.vtx.size() - 1); @@ -2746,7 +2749,7 @@ bool static LoadBlockIndexDB(string& strError) CTxUndo undoDummy; if (i > 0) vtxundo.push_back(CTxUndo()); - UpdateCoinsWithTransaction(block.vtx[i], view, i == 0 ? undoDummy : vtxundo.back(), pindex->nHeight); + UpdateCoinsWithTransaction(block.vtx[i], view, i == 0 ? undoDummy : vtxundo.back(), utxoHasher, pindex->nHeight); view.SetBestBlock(hashBlock); } diff --git a/divi/src/txmempool.cpp b/divi/src/txmempool.cpp index 86b7e1e7f..812237717 100644 --- a/divi/src/txmempool.cpp +++ b/divi/src/txmempool.cpp @@ -359,6 +359,23 @@ class CMinerPolicyEstimator } }; +namespace +{ + +/** The UTXO hasher used in mempool logic. */ +class MempoolUtxoHasher : public TransactionUtxoHasher +{ + +public: + + uint256 GetUtxoHash(const CTransaction& tx) const override + { + return tx.GetHash(); + } + +}; + +} // anonymous namespace CTxMemPool::CTxMemPool(const CFeeRate& _minRelayFee) : nTransactionsUpdated(0), minRelayFee(_minRelayFee) @@ -374,6 +391,8 @@ CTxMemPool::CTxMemPool(const CFeeRate& _minRelayFee) : nTransactionsUpdated(0), // Confirmation times for very-low-fee transactions that take more // than an hour or three to confirm are highly variable. minerPolicyEstimator = new CMinerPolicyEstimator(25); + + utxoHasher.reset(new MempoolUtxoHasher()); } CTxMemPool::~CTxMemPool() @@ -426,6 +445,11 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry& entry) return true; } +const TransactionUtxoHasher& CTxMemPool::GetUtxoHasher() const +{ + return *utxoHasher; +} + void CTxMemPool::addAddressIndex(const CTxMemPoolEntry &entry, const CCoinsViewCache &view) { LOCK(cs); @@ -578,7 +602,7 @@ void CTxMemPool::remove(const CTransaction& origTx, std::list& rem // happen during chain re-orgs if origTx isn't re-accepted into // the mempool for any reason. for (unsigned int i = 0; i < origTx.vout.size(); i++) { - std::map::iterator it = mapNextTx.find(COutPoint(origTx.GetHash(), i)); + auto it = mapNextTx.find(COutPoint(utxoHasher->GetUtxoHash(origTx), i)); if (it == mapNextTx.end()) continue; txToRemove.push_back(it->second.ptx->GetHash()); @@ -592,7 +616,7 @@ void CTxMemPool::remove(const CTransaction& origTx, std::list& rem const CTransaction& tx = mapTx[hash].GetTx(); if (fRecursive) { for (unsigned int i = 0; i < tx.vout.size(); i++) { - std::map::iterator it = mapNextTx.find(COutPoint(hash, i)); + auto it = mapNextTx.find(COutPoint(utxoHasher->GetUtxoHash(tx), i)); if (it == mapNextTx.end()) continue; txToRemove.push_back(it->second.ptx->GetHash()); @@ -619,7 +643,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); @@ -704,7 +728,7 @@ void CTxMemPool::check(const CCoinsViewCache* pcoins) const 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 { @@ -724,7 +748,7 @@ void CTxMemPool::check(const CCoinsViewCache* pcoins) const CValidationState state; CTxUndo undo; assert(CheckInputs(tx, state, mempoolDuplicate, false, 0, false, NULL)); - UpdateCoinsWithTransaction(tx, mempoolDuplicate, undo, 1000000); + UpdateCoinsWithTransaction(tx, mempoolDuplicate, undo, *utxoHasher, 1000000); } } unsigned int stepsSinceLastRemove = 0; @@ -739,7 +763,7 @@ void CTxMemPool::check(const CCoinsViewCache* pcoins) const } else { assert(CheckInputs(entry->GetTx(), state, mempoolDuplicate, false, 0, false, NULL)); CTxUndo undo; - UpdateCoinsWithTransaction(entry->GetTx(), mempoolDuplicate, undo, 1000000); + UpdateCoinsWithTransaction(entry->GetTx(), mempoolDuplicate, undo, *utxoHasher, 1000000); stepsSinceLastRemove = 0; } } @@ -784,6 +808,20 @@ bool CTxMemPool::lookupBareTxid(const uint256& btxid, CTransaction& result) cons return true; } +bool CTxMemPool::lookupOutpoint(const uint256& hash, CTransaction& result) const +{ + /* The TransactionUtxoHasher can only tell us the txid to use once we + know the transaction already. Thus we check both txid and bare txid + in our index; if one of them matches, we then cross-check with the + then-known transaction that it actually should hash to that UTXO. */ + if (lookup(hash, result) && utxoHasher->GetUtxoHash(result) == hash) + return true; + if (lookupBareTxid(hash, result) && utxoHasher->GetUtxoHash(result) == hash) + return true; + + return false; +} + CFeeRate CTxMemPool::estimateFee(int nBlocks) const { LOCK(cs); @@ -863,7 +901,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; } @@ -872,5 +910,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 538cc8e2b..16634cf41 100644 --- a/divi/src/txmempool.h +++ b/divi/src/txmempool.h @@ -8,6 +8,7 @@ #define BITCOIN_TXMEMPOOL_H #include +#include #include "addressindex.h" #include "spentindex.h" @@ -18,6 +19,7 @@ #include "sync.h" class CAutoFile; +class TransactionUtxoHasher; inline double AllowFreeThreshold() { @@ -101,6 +103,7 @@ class CTxMemPool bool fSanityCheck; //! Normally false, true if -checkmempool or -regtest unsigned int nTransactionsUpdated; CMinerPolicyEstimator* minerPolicyEstimator; + std::unique_ptr utxoHasher; CFeeRate minRelayFee; //! Passed to constructor to avoid dependency on main uint64_t totalTxSize; //! sum of all mempool tx' byte sizes @@ -152,6 +155,9 @@ class CTxMemPool unsigned int GetTransactionsUpdated() const; void AddTransactionsUpdated(unsigned int n); + /** Returns the UTXO hasher instance used in the mempool. */ + const TransactionUtxoHasher& GetUtxoHasher() const; + void addAddressIndex(const CTxMemPoolEntry &entry, const CCoinsViewCache &view); bool getAddressIndex(std::vector > &addresses, std::vector > &results); @@ -192,6 +198,11 @@ 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. This takes the + * state of segwit light activation into account for deciding whether to + * use the normal txid or the bare txid map. */ + bool lookupOutpoint(const uint256& hash, CTransaction& result) const; + /** Estimate fee rate needed to get into the next nBlocks */ CFeeRate estimateFee(int nBlocks) const; From 7130c3d891e7a8f60f5906116876df77a441353f Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Wed, 20 Jan 2021 16:53:02 +0100 Subject: [PATCH 4/5] Unit test for UpdateCoins UTXO creation. Add a unit test (together with the necessary framework around) for UTXO creation from UpdateCoins, based on the UTXO hasher (rather than the txid directly). --- divi/src/Makefile.test.include | 2 + divi/src/test/MockUtxoHasher.cpp | 21 ++++++++ divi/src/test/MockUtxoHasher.h | 39 ++++++++++++++ .../test/UtxoCheckingAndUpdating_tests.cpp | 54 +++++++++++++++++++ 4 files changed, 116 insertions(+) create mode 100644 divi/src/test/MockUtxoHasher.cpp create mode 100644 divi/src/test/MockUtxoHasher.h create mode 100644 divi/src/test/UtxoCheckingAndUpdating_tests.cpp diff --git a/divi/src/Makefile.test.include b/divi/src/Makefile.test.include index 140e12caf..994155373 100644 --- a/divi/src/Makefile.test.include +++ b/divi/src/Makefile.test.include @@ -63,6 +63,7 @@ BITCOIN_TESTS =\ test/MockBlockIncentivesPopulator.h \ test/MockBlockSubsidyProvider.h \ test/MockPoSStakeModifierService.h \ + test/MockUtxoHasher.cpp \ test/MockVaultManagerDatabase.h \ test/monthlywalletbackupcreator_tests.cpp \ test/ActiveMasternode_tests.cpp \ @@ -101,6 +102,7 @@ BITCOIN_TESTS =\ test/PoSStakeModifierService_tests.cpp \ test/LegacyPoSStakeModifierService_tests.cpp \ test/LotteryWinnersCalculatorTests.cpp \ + test/UtxoCheckingAndUpdating_tests.cpp \ test/VaultManager_tests.cpp \ test/multi_wallet_tests.cpp diff --git a/divi/src/test/MockUtxoHasher.cpp b/divi/src/test/MockUtxoHasher.cpp new file mode 100644 index 000000000..70a5d1572 --- /dev/null +++ b/divi/src/test/MockUtxoHasher.cpp @@ -0,0 +1,21 @@ +#include "MockUtxoHasher.h" + +#include "hash.h" +#include "primitives/transaction.h" + +uint256 MockUtxoHasher::Add(const CTransaction& tx) +{ + ++cnt; + const uint256 value = Hash(&cnt, (&cnt) + 1); + customHashes.emplace(tx.GetHash(), value); + return value; +} + +uint256 MockUtxoHasher::GetUtxoHash(const CTransaction& tx) const +{ + const uint256 txid = tx.GetHash(); + const auto mit = customHashes.find(txid); + if (mit != customHashes.end()) + return mit->second; + return txid; +} diff --git a/divi/src/test/MockUtxoHasher.h b/divi/src/test/MockUtxoHasher.h new file mode 100644 index 000000000..d746dfadd --- /dev/null +++ b/divi/src/test/MockUtxoHasher.h @@ -0,0 +1,39 @@ +#ifndef MOCKUTXOHASHER_H +#define MOCKUTXOHASHER_H + +#include "UtxoCheckingAndUpdating.h" + +#include "uint256.h" + +#include + +class CTransaction; + +/** A TransactionUtxoHasher instance that returns normal txid's (as per before + * segwit-light) by default, but can be instructed to return custom hashes + * for certain transactions. */ +class MockUtxoHasher : public TransactionUtxoHasher +{ + +private: + + /** Custom hashes to return for given txid's. */ + std::map customHashes; + + /** Internal counter to produce unique fake hashes. */ + unsigned cnt = 0; + +public: + + MockUtxoHasher() + {} + + /** Marks the given transaction for returning a custom hash. The hash + * is generated uniquely and returned from the function. */ + uint256 Add(const CTransaction& tx); + + uint256 GetUtxoHash(const CTransaction& tx) const override; + +}; + +#endif // MOCKUTXOHASHER_H diff --git a/divi/src/test/UtxoCheckingAndUpdating_tests.cpp b/divi/src/test/UtxoCheckingAndUpdating_tests.cpp new file mode 100644 index 000000000..241b08866 --- /dev/null +++ b/divi/src/test/UtxoCheckingAndUpdating_tests.cpp @@ -0,0 +1,54 @@ +#include +#include + +#include "coins.h" +#include "MockUtxoHasher.h" +#include "primitives/transaction.h" + +namespace +{ + +class UpdateCoinsTestFixture +{ + +private: + + /** Empty coins used as base in the cached view. */ + CCoinsView dummyCoins; + +protected: + + CCoinsViewCache coins; + MockUtxoHasher utxoHasher; + + UpdateCoinsTestFixture() + : coins(&dummyCoins) + {} + +}; + +BOOST_FIXTURE_TEST_SUITE(UpdateCoins_tests, UpdateCoinsTestFixture) + +BOOST_AUTO_TEST_CASE(addsCorrectOutputs) +{ + CMutableTransaction mtx; + mtx.vout.emplace_back(1, CScript() << OP_TRUE); + const CTransaction tx1(mtx); + + mtx.vout.clear(); + mtx.vout.emplace_back(2, CScript() << OP_TRUE); + const CTransaction tx2(mtx); + const auto id2 = utxoHasher.Add(tx2); + + CTxUndo txundo; + UpdateCoins(tx1, coins, txundo, utxoHasher, 101); + UpdateCoins(tx2, coins, txundo, utxoHasher, 102); + + BOOST_CHECK(coins.HaveCoins(tx1.GetHash())); + BOOST_CHECK(!coins.HaveCoins(tx2.GetHash())); + BOOST_CHECK(coins.HaveCoins(id2)); +} + +BOOST_AUTO_TEST_SUITE_END() + +} // anonymous namespace From b69df5aa88f13a9661319d2e971c914df3b5a57e Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Thu, 21 Jan 2021 15:25:58 +0100 Subject: [PATCH 5/5] Unit test for mempool with UTXO hasher. This extends the mempool unit tests to explicitly verify that adding transactions, removing transactions, checking the pool and looking up coins / transactions still works even if we use the bare txid for some transactions as UTXO hash (as will be the case with segwit-light in the future). --- divi/src/test/MockUtxoHasher.cpp | 5 + divi/src/test/MockUtxoHasher.h | 4 + .../test/UtxoCheckingAndUpdating_tests.cpp | 4 +- divi/src/test/mempool_tests.cpp | 106 ++++++++++++++---- divi/src/txmempool.cpp | 5 + divi/src/txmempool.h | 4 + 6 files changed, 106 insertions(+), 22 deletions(-) diff --git a/divi/src/test/MockUtxoHasher.cpp b/divi/src/test/MockUtxoHasher.cpp index 70a5d1572..2170dd638 100644 --- a/divi/src/test/MockUtxoHasher.cpp +++ b/divi/src/test/MockUtxoHasher.cpp @@ -11,6 +11,11 @@ uint256 MockUtxoHasher::Add(const CTransaction& tx) return value; } +void MockUtxoHasher::UseBareTxid(const CTransaction& tx) +{ + customHashes.emplace(tx.GetHash(), tx.GetBareTxid()); +} + uint256 MockUtxoHasher::GetUtxoHash(const CTransaction& tx) const { const uint256 txid = tx.GetHash(); diff --git a/divi/src/test/MockUtxoHasher.h b/divi/src/test/MockUtxoHasher.h index d746dfadd..9ea8f0ade 100644 --- a/divi/src/test/MockUtxoHasher.h +++ b/divi/src/test/MockUtxoHasher.h @@ -32,6 +32,10 @@ class MockUtxoHasher : public TransactionUtxoHasher * is generated uniquely and returned from the function. */ uint256 Add(const CTransaction& tx); + /** Marks the given transaction for using the bare txid rather than + * the normal txid. */ + void UseBareTxid(const CTransaction& tx); + uint256 GetUtxoHash(const CTransaction& tx) const override; }; diff --git a/divi/src/test/UtxoCheckingAndUpdating_tests.cpp b/divi/src/test/UtxoCheckingAndUpdating_tests.cpp index 241b08866..4c2324971 100644 --- a/divi/src/test/UtxoCheckingAndUpdating_tests.cpp +++ b/divi/src/test/UtxoCheckingAndUpdating_tests.cpp @@ -41,8 +41,8 @@ BOOST_AUTO_TEST_CASE(addsCorrectOutputs) const auto id2 = utxoHasher.Add(tx2); CTxUndo txundo; - UpdateCoins(tx1, coins, txundo, utxoHasher, 101); - UpdateCoins(tx2, coins, txundo, utxoHasher, 102); + UpdateCoinsWithTransaction(tx1, coins, txundo, utxoHasher, 101); + UpdateCoinsWithTransaction(tx2, coins, txundo, utxoHasher, 102); BOOST_CHECK(coins.HaveCoins(tx1.GetHash())); BOOST_CHECK(!coins.HaveCoins(tx2.GetHash())); diff --git a/divi/src/test/mempool_tests.cpp b/divi/src/test/mempool_tests.cpp index af1b6ad18..32a73712a 100644 --- a/divi/src/test/mempool_tests.cpp +++ b/divi/src/test/mempool_tests.cpp @@ -2,39 +2,67 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include "main.h" #include "txmempool.h" +#include "blockmap.h" +#include "MockUtxoHasher.h" + #include #include +extern CChain chainActive; +extern BlockMap mapBlockIndex; + class MempoolTestFixture { +private: + + /** Empty coins view used to back the cached view we actually use. */ + CCoinsView emptyView; + + /** Tip of our fake chain. */ + CBlockIndex tip; + protected: /** A parent transaction. */ CMutableTransaction txParent; - /** Three children of the parent. */ + /** Three children of the parent. They use the bare txid for their UTXOs + * in our UTXO hasher. */ CMutableTransaction txChild[3]; /** Three grand children. */ CMutableTransaction txGrandChild[3]; + /** Coins view with the parent inputs. */ + CCoinsViewCache view; + /** The test mempool instance. */ CTxMemPool testPool; public: MempoolTestFixture() - : testPool(CFeeRate(0)) + : view(&emptyView), testPool(CFeeRate(0)) { - txParent.vin.resize(2); + std::unique_ptr utxoHasher(new MockUtxoHasher()); + + CMutableTransaction base; + base.vout.emplace_back(99000LL, CScript() << OP_11 << OP_EQUAL); + view.ModifyCoins(base.GetHash())->FromTx(base, 0); + + tip.pprev = nullptr; + tip.nHeight = 0; + mapBlockIndex[tip.GetBlockHeader().GetHash()] = &tip; + view.SetBestBlock(tip.GetBlockHeader().GetHash()); + chainActive.SetTip(&tip); + + txParent.vin.resize(1); txParent.vin[0].scriptSig = CScript() << OP_11; - /* Add a second input to make sure the transaction does not qualify as - coinbase and thus has a bare txid unequal to its normal hash. */ - txParent.vin[1].scriptSig = CScript() << OP_12; + txParent.vin[0].prevout.hash = base.GetHash(); + txParent.vin[0].prevout.n = 0; txParent.vout.resize(3); for (int i = 0; i < 3; i++) { @@ -52,18 +80,39 @@ class MempoolTestFixture txChild[i].vout.resize(1); txChild[i].vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; txChild[i].vout[0].nValue = 11000LL; + utxoHasher->UseBareTxid(txChild[i]); } for (int i = 0; i < 3; i++) { txGrandChild[i].vin.resize(1); txGrandChild[i].vin[0].scriptSig = CScript() << OP_11; - txGrandChild[i].vin[0].prevout.hash = txChild[i].GetHash(); + txGrandChild[i].vin[0].prevout.hash = txChild[i].GetBareTxid(); txGrandChild[i].vin[0].prevout.n = 0; txGrandChild[i].vout.resize(1); txGrandChild[i].vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; txGrandChild[i].vout[0].nValue = 11000LL; } + + testPool.setSanityCheck(true); + testPool.SetUtxoHasherForTesting(std::move(utxoHasher)); + testPool.clear(); + } + + ~MempoolTestFixture() + { + mapBlockIndex.clear(); + } + + /** Adds the parent, childs and grandchilds to the mempool. */ + void AddAll() + { + testPool.addUnchecked(txParent.GetHash(), CTxMemPoolEntry(txParent, 0, 0, 0.0, 1)); + for (int i = 0; i < 3; i++) + { + testPool.addUnchecked(txChild[i].GetHash(), CTxMemPoolEntry(txChild[i], 0, 0, 0.0, 1)); + testPool.addUnchecked(txGrandChild[i].GetHash(), CTxMemPoolEntry(txGrandChild[i], 0, 0, 0.0, 1)); + } } }; @@ -87,12 +136,10 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) removed.clear(); // Parent, children, grandchildren: - testPool.addUnchecked(txParent.GetHash(), CTxMemPoolEntry(txParent, 0, 0, 0.0, 1)); - for (int i = 0; i < 3; i++) - { - testPool.addUnchecked(txChild[i].GetHash(), CTxMemPoolEntry(txChild[i], 0, 0, 0.0, 1)); - testPool.addUnchecked(txGrandChild[i].GetHash(), CTxMemPoolEntry(txGrandChild[i], 0, 0, 0.0, 1)); - } + AddAll(); + + testPool.check(&view); + // Remove Child[0], GrandChild[0] should be removed: testPool.remove(txChild[0], removed, true); BOOST_CHECK_EQUAL(removed.size(), 2); @@ -127,12 +174,7 @@ BOOST_AUTO_TEST_CASE(MempoolIndexByBareTxid) CTransaction tx; std::list removed; - testPool.addUnchecked(txParent.GetHash(), CTxMemPoolEntry(txParent, 0, 0, 0.0, 1)); - for (int i = 0; i < 3; ++i) - { - testPool.addUnchecked(txChild[i].GetHash(), CTxMemPoolEntry(txChild[i], 0, 0, 0.0, 1)); - testPool.addUnchecked(txGrandChild[i].GetHash(), CTxMemPoolEntry(txGrandChild[i], 0, 0, 0.0, 1)); - } + AddAll(); BOOST_CHECK(testPool.lookupBareTxid(txParent.GetBareTxid(), tx)); BOOST_CHECK(tx.GetHash() == txParent.GetHash()); @@ -144,4 +186,28 @@ BOOST_AUTO_TEST_CASE(MempoolIndexByBareTxid) BOOST_CHECK(!testPool.lookupBareTxid(txGrandChild[0].GetBareTxid(), tx)); } +BOOST_AUTO_TEST_CASE(MempoolOutpointLookup) +{ + CTransaction tx; + CCoins coins; + + AddAll(); + CCoinsViewMemPool viewPool(&view, 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(), coins)); + BOOST_CHECK(!viewPool.HaveCoins(txParent.GetBareTxid())); + BOOST_CHECK(!viewPool.GetCoins(txParent.GetBareTxid(), coins)); + + BOOST_CHECK(!viewPool.HaveCoins(txChild[0].GetHash())); + BOOST_CHECK(!viewPool.GetCoins(txChild[0].GetHash(), coins)); + BOOST_CHECK(viewPool.HaveCoins(txChild[0].GetBareTxid())); + BOOST_CHECK(viewPool.GetCoins(txChild[0].GetBareTxid(), coins)); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/divi/src/txmempool.cpp b/divi/src/txmempool.cpp index 812237717..b32f834d1 100644 --- a/divi/src/txmempool.cpp +++ b/divi/src/txmempool.cpp @@ -450,6 +450,11 @@ const TransactionUtxoHasher& CTxMemPool::GetUtxoHasher() const return *utxoHasher; } +void CTxMemPool::SetUtxoHasherForTesting(std::unique_ptr hasher) +{ + utxoHasher = std::move(hasher); +} + void CTxMemPool::addAddressIndex(const CTxMemPoolEntry &entry, const CCoinsViewCache &view) { LOCK(cs); diff --git a/divi/src/txmempool.h b/divi/src/txmempool.h index 16634cf41..d42002253 100644 --- a/divi/src/txmempool.h +++ b/divi/src/txmempool.h @@ -158,6 +158,10 @@ class CTxMemPool /** Returns the UTXO hasher instance used in the mempool. */ const TransactionUtxoHasher& GetUtxoHasher() const; + /** Replaces the UTXO hasher used in the mempool with the given instance, + * which allows dependency injection for unit tests. */ + void SetUtxoHasherForTesting(std::unique_ptr hasher); + void addAddressIndex(const CTxMemPoolEntry &entry, const CCoinsViewCache &view); bool getAddressIndex(std::vector > &addresses, std::vector > &results);