From da3da1bb1bf4f868642cc4ca5aed6f8e2e0d5f10 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 16 Mar 2021 15:38:42 +0100 Subject: [PATCH 01/16] 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 02/16] 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 03/16] 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 04/16] 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 05/16] 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); From df5944d372c9bfa2ac05c2af80392c3f02074722 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 26 Jan 2021 15:28:48 +0100 Subject: [PATCH 06/16] Use UTXO hasher for spending coins. Use the UTXO hasher abstraction in the wallet and for staking (i.e. for places where coins are spent). The wallet gets its own instance, which will allow for dependency injection in tests. For now, the hasher used in the wallet is just the normal hasher, i.e. there are no actual changes in behaviour. In the future, the wallet hasher can be changed accordingly for the activation of segwit light. --- divi/src/PoSTransactionCreator.cpp | 10 ++++---- divi/src/rpcmasternode.cpp | 2 +- divi/src/rpcrawtransaction.cpp | 4 +++ divi/src/wallet.cpp | 39 +++++++++++++++++++++++++----- divi/src/wallet.h | 8 ++++++ 5 files changed, 51 insertions(+), 12 deletions(-) diff --git a/divi/src/PoSTransactionCreator.cpp b/divi/src/PoSTransactionCreator.cpp index 6b1d3c19d..7c37f580e 100644 --- a/divi/src/PoSTransactionCreator.cpp +++ b/divi/src/PoSTransactionCreator.cpp @@ -127,8 +127,8 @@ bool PoSTransactionCreator::SetSuportedStakingScript( const StakableCoin& stakableCoin, CMutableTransaction& txNew) { - txNew.vin.push_back(CTxIn(stakableCoin.tx->GetHash(), stakableCoin.outputIndex)); - txNew.vout.push_back(CTxOut(0, stakableCoin.tx->vout[stakableCoin.outputIndex].scriptPubKey )); + txNew.vin.emplace_back(wallet_.GetUtxoHash(*stakableCoin.tx), stakableCoin.outputIndex); + txNew.vout.emplace_back(0, stakableCoin.tx->vout[stakableCoin.outputIndex].scriptPubKey); return true; } @@ -145,7 +145,7 @@ void PoSTransactionCreator::CombineUtxos( for(const StakableCoin& pcoin : stakedCoins_->asSet()) { if(pcoin.tx->vout[pcoin.outputIndex].scriptPubKey == txNew.vout[1].scriptPubKey && - pcoin.tx->GetHash() != txNew.vin[0].prevout.hash) + wallet_.GetUtxoHash(*pcoin.tx) != txNew.vin[0].prevout.hash) { if(pcoin.tx->vout[pcoin.outputIndex].nValue + nCredit > nCombineThreshold) continue; @@ -166,7 +166,7 @@ void PoSTransactionCreator::CombineUtxos( nCredit + pcoin.tx->vout[pcoin.outputIndex].nValue > nCombineThreshold) break; - txNew.vin.push_back(CTxIn(pcoin.tx->GetHash(), pcoin.outputIndex)); + txNew.vin.emplace_back(wallet_.GetUtxoHash(*pcoin.tx), pcoin.outputIndex); nCredit += pcoin.tx->vout[pcoin.outputIndex].nValue; walletTransactions.push_back(pcoin.tx); } @@ -334,4 +334,4 @@ bool PoSTransactionCreator::CreateProofOfStake( stakedCoins_->resetTimestamp(); //this will trigger stake set to repopulate next round return true; -} \ No newline at end of file +} diff --git a/divi/src/rpcmasternode.cpp b/divi/src/rpcmasternode.cpp index be6621964..759c35f64 100644 --- a/divi/src/rpcmasternode.cpp +++ b/divi/src/rpcmasternode.cpp @@ -99,7 +99,7 @@ Value allocatefunds(const Array& params, bool fHelp) SendMoney(acctAddr.Get(), CMasternode::GetTierCollateralAmount(nMasternodeTier), wtx); Object obj; - obj.push_back(Pair("txhash", wtx.GetHash().GetHex())); + obj.push_back(Pair("txhash", pwalletMain->GetUtxoHash(wtx).GetHex())); return obj; } diff --git a/divi/src/rpcrawtransaction.cpp b/divi/src/rpcrawtransaction.cpp index 34763add1..a2dd2b227 100644 --- a/divi/src/rpcrawtransaction.cpp +++ b/divi/src/rpcrawtransaction.cpp @@ -322,6 +322,8 @@ Value listunspent(const Array& params, bool fHelp) "[ (array of json object)\n" " {\n" " \"txid\" : \"txid\", (string) the transaction id \n" + " \"baretxid\" : \"baretxid\", (string) The bare txid (without signatures)\n" + " \"outputhash\" : \"outputhash\", (string) The hash (txid or bare txid) that should be used for spending\n" " \"vout\" : n, (numeric) the vout value\n" " \"address\" : \"address\", (string) the divi address\n" " \"account\" : \"account\", (string) The associated account, or \"\" for the default account\n" @@ -379,6 +381,8 @@ Value listunspent(const Array& params, bool fHelp) const CScript& pk = out.tx->vout[out.i].scriptPubKey; Object entry; entry.push_back(Pair("txid", out.tx->GetHash().GetHex())); + entry.push_back(Pair("baretxid", out.tx->GetBareTxid().GetHex())); + entry.push_back(Pair("outputhash", pwalletMain->GetUtxoHash(*out.tx).GetHex())); entry.push_back(Pair("vout", out.i)); CTxDestination address; if (ExtractDestination(out.tx->vout[out.i].scriptPubKey, address)) { diff --git a/divi/src/wallet.cpp b/divi/src/wallet.cpp index d08f16cb6..e3673da8d 100644 --- a/divi/src/wallet.cpp +++ b/divi/src/wallet.cpp @@ -36,6 +36,7 @@ #include #include #include +#include #include #include @@ -177,10 +178,31 @@ bool WriteTxToDisk(const CWallet* walletPtr, const CWalletTx& transactionToWrite return CWalletDB(walletPtr->strWalletFile).WriteTx(transactionToWrite.GetHash(),transactionToWrite); } +namespace +{ + +/** Dummy UTXO hasher for the wallet. For now, this just always returns + * the normal txid, but we will later change it to return the proper hash + * for a WalletTx. */ +class WalletUtxoHasher : public TransactionUtxoHasher +{ + +public: + + uint256 GetUtxoHash(const CTransaction& tx) const override + { + return tx.GetHash(); + } + +}; + +} // anonymous namespace + CWallet::CWallet(const CChain& chain, const BlockMap& blockMap ): cs_wallet() , transactionRecord_(new WalletTransactionRecord(cs_wallet,strWalletFile) ) , outputTracker_( new SpentOutputTracker(*transactionRecord_) ) + , utxoHasher(new WalletUtxoHasher() ) , chainActive_(chain) , mapBlockIndex_(blockMap) , orderedTransactionIndex() @@ -336,7 +358,7 @@ CAmount CWallet::GetDebit(const CWalletTx& tx, const isminefilter& filter) const CAmount CWallet::ComputeCredit(const CWalletTx& tx, const isminefilter& filter, int creditFilterFlags) const { CAmount nCredit = 0; - uint256 hash = tx.GetHash(); + const uint256 hash = GetUtxoHash(tx); for (unsigned int i = 0; i < tx.vout.size(); i++) { if( (creditFilterFlags & REQUIRE_UNSPENT) && IsSpent(tx,i)) continue; if( (creditFilterFlags & REQUIRE_UNLOCKED) && IsLockedCoin(hash,i)) continue; @@ -934,7 +956,7 @@ set CWallet::GetConflicts(const uint256& txid) const */ bool CWallet::IsSpent(const CWalletTx& wtx, unsigned int n) const { - return outputTracker_->IsSpent(wtx.GetHash(), n); + return outputTracker_->IsSpent(GetUtxoHash(wtx), n); } bool CWallet::GetMasternodeVinAndKeys(CTxIn& txinRet, CPubKey& pubKeyRet, CKey& keyRet, std::string strTxHash, std::string strOutputIndex) @@ -979,7 +1001,7 @@ bool CWallet::GetVinAndKeysFromOutput(COutput out, CTxIn& txinRet, CPubKey& pubK CScript pubScript; - txinRet = CTxIn(out.tx->GetHash(), out.i); + txinRet = CTxIn(GetUtxoHash(*out.tx), out.i); pubScript = out.tx->vout[out.i].scriptPubKey; // the inputs PubKey CTxDestination address1; @@ -1709,7 +1731,7 @@ bool CWallet::IsAvailableForSpending(const CWalletTx* pcoin, unsigned int i, con } } - const uint256 hash = pcoin->GetHash(); + const uint256 hash = GetUtxoHash(*pcoin); if (IsSpent(*pcoin, i)) return false; @@ -2223,8 +2245,8 @@ bool CWallet::CreateTransaction(const vector >& vecSend, } // Fill vin - BOOST_FOREACH (const PAIRTYPE(const CWalletTx*, unsigned int) & coin, setCoins) - txNew.vin.push_back(CTxIn(coin.first->GetHash(), coin.second)); + for (const auto& coin : setCoins) + txNew.vin.emplace_back(GetUtxoHash(*coin.first), coin.second); // Sign int nIn = 0; @@ -2383,6 +2405,11 @@ CAmount CWallet::GetTotalValue(std::vector vCoins) return nTotalValue; } +uint256 CWallet::GetUtxoHash(const CMerkleTx& tx) const +{ + return utxoHasher->GetUtxoHash(tx); +} + DBErrors CWallet::LoadWallet(bool& fFirstRunRet) { if (!fFileBacked) diff --git a/divi/src/wallet.h b/divi/src/wallet.h index 8eacc7b71..aa2e20cd1 100644 --- a/divi/src/wallet.h +++ b/divi/src/wallet.h @@ -26,6 +26,8 @@ #include #include +#include + class CKeyMetadata; class CKey; class CBlock; @@ -35,6 +37,7 @@ class CBlockIndex; struct StakableCoin; class WalletTransactionRecord; class SpentOutputTracker; +class TransactionUtxoHasher; //! -paytxfee default static const CAmount DEFAULT_TRANSACTION_FEE = 0; @@ -133,6 +136,7 @@ class CWallet : public CCryptoKeyStore, public NotificationInterface, public I_K private: std::unique_ptr transactionRecord_; std::unique_ptr outputTracker_; + std::unique_ptr utxoHasher; const CChain& chainActive_; const BlockMap& mapBlockIndex_; int64_t orderedTransactionIndex; @@ -239,6 +243,10 @@ class CWallet : public CCryptoKeyStore, public NotificationInterface, public I_K void ListLockedCoins(CoinVector& vOutpts); CAmount GetTotalValue(Inputs vCoins); + /** Returns the UTXO hash that should be used for spending outputs + * from the given transaction (which should be part of the wallet). */ + uint256 GetUtxoHash(const CMerkleTx& tx) const; + // keystore implementation // Generate a new key CPubKey GenerateNewKey(uint32_t nAccountIndex, bool fInternal); From de25400278c1c921eb8c9ea525b78f2c041248e0 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Thu, 28 Jan 2021 13:30:10 +0100 Subject: [PATCH 07/16] Test UTXO hasher in PoSTransactionCreator. This adds basic unit tests for PoSTransactionCreator, with a fake environment that allows a real PoSTransactionCreator to create staking transactions. In particular, we use that to unit test that it uses the UTXO hasher of the wallet properly. --- divi/src/Makefile.test.include | 1 + divi/src/ProofOfStakeModule.cpp | 6 +- divi/src/ProofOfStakeModule.h | 6 +- divi/src/test/FakeWallet.cpp | 5 + divi/src/test/FakeWallet.h | 6 + divi/src/test/PoSTransactionCreator_tests.cpp | 145 ++++++++++++++++++ divi/src/wallet.cpp | 5 + divi/src/wallet.h | 3 + 8 files changed, 171 insertions(+), 6 deletions(-) create mode 100644 divi/src/test/PoSTransactionCreator_tests.cpp diff --git a/divi/src/Makefile.test.include b/divi/src/Makefile.test.include index 994155373..76b5d1805 100644 --- a/divi/src/Makefile.test.include +++ b/divi/src/Makefile.test.include @@ -100,6 +100,7 @@ BITCOIN_TESTS =\ test/ProofOfStake_tests.cpp \ test/IsMine_tests.cpp \ test/PoSStakeModifierService_tests.cpp \ + test/PoSTransactionCreator_tests.cpp \ test/LegacyPoSStakeModifierService_tests.cpp \ test/LotteryWinnersCalculatorTests.cpp \ test/UtxoCheckingAndUpdating_tests.cpp \ diff --git a/divi/src/ProofOfStakeModule.cpp b/divi/src/ProofOfStakeModule.cpp index 257a5aa00..e5e65c03f 100644 --- a/divi/src/ProofOfStakeModule.cpp +++ b/divi/src/ProofOfStakeModule.cpp @@ -7,8 +7,8 @@ ProofOfStakeModule::ProofOfStakeModule( const CChainParams& chainParameters, - CChain& activeChain, - BlockMap& blockIndexByHash + const CChain& activeChain, + const BlockMap& blockIndexByHash ): legacyStakeModifierService_(new LegacyPoSStakeModifierService(blockIndexByHash,activeChain)) , stakeModifierService_(new PoSStakeModifierService(*legacyStakeModifierService_, blockIndexByHash)) , proofGenerator_(new ProofOfStakeGenerator(*stakeModifierService_,chainParameters.GetMinCoinAgeForStaking())) @@ -25,4 +25,4 @@ ProofOfStakeModule::~ProofOfStakeModule() const I_ProofOfStakeGenerator& ProofOfStakeModule::proofOfStakeGenerator() const { return *proofGenerator_; -} \ No newline at end of file +} diff --git a/divi/src/ProofOfStakeModule.h b/divi/src/ProofOfStakeModule.h index f3986ea54..08a31c50f 100644 --- a/divi/src/ProofOfStakeModule.h +++ b/divi/src/ProofOfStakeModule.h @@ -16,9 +16,9 @@ class ProofOfStakeModule public: ProofOfStakeModule( const CChainParams& chainParameters, - CChain& activeChain, - BlockMap& blockIndexByHash); + const CChain& activeChain, + const BlockMap& blockIndexByHash); ~ProofOfStakeModule(); const I_ProofOfStakeGenerator& proofOfStakeGenerator() const; }; -#endif// PROOF_OF_STAKE_MODULE_H \ No newline at end of file +#endif// PROOF_OF_STAKE_MODULE_H diff --git a/divi/src/test/FakeWallet.cpp b/divi/src/test/FakeWallet.cpp index 29eaa0e00..d912d234f 100644 --- a/divi/src/test/FakeWallet.cpp +++ b/divi/src/test/FakeWallet.cpp @@ -104,6 +104,11 @@ void FakeWallet::AddBlock() fakeChain.addBlocks(1, version); } +void FakeWallet::AddConfirmations(const unsigned numConf, const int64_t minAge) +{ + fakeChain.addBlocks(numConf, version, fakeChain.activeChain->Tip()->nTime + minAge); +} + const CWalletTx& FakeWallet::AddDefaultTx(const CScript& scriptToPayTo, unsigned& outputIndex, const CAmount amount) { diff --git a/divi/src/test/FakeWallet.h b/divi/src/test/FakeWallet.h index fe2456767..1beff3b06 100644 --- a/divi/src/test/FakeWallet.h +++ b/divi/src/test/FakeWallet.h @@ -38,6 +38,12 @@ class FakeWallet : public CWallet /** Adds a new block to our fake chain. */ void AddBlock(); + /** Adds a couple of new blocks and bumps the time at least + * the given amount. This can be used to make sure some + * coins fake added to the chain have at least a given age + * and number of confirmations. */ + void AddConfirmations(unsigned numConf, int64_t minAge = 0); + /** Adds a new ordinary transaction to the wallet, paying a given amount * to a given script. The transaction is returned, and the output index * with the output to the requested script is set. */ diff --git a/divi/src/test/PoSTransactionCreator_tests.cpp b/divi/src/test/PoSTransactionCreator_tests.cpp new file mode 100644 index 000000000..7c26f3aa8 --- /dev/null +++ b/divi/src/test/PoSTransactionCreator_tests.cpp @@ -0,0 +1,145 @@ +// Copyright (c) 2021 The Divi developers +// Distributed under the MIT/X11 software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include "PoSTransactionCreator.h" + +#include "chain.h" +#include "chainparams.h" +#include "ProofOfStakeModule.h" +#include "script/standard.h" +#include "WalletTx.h" + +#include "test/FakeBlockIndexChain.h" +#include "test/FakeWallet.h" +#include "test/MockBlockIncentivesPopulator.h" +#include "test/MockBlockSubsidyProvider.h" +#include "test/MockUtxoHasher.h" + +#include +#include "test/test_only.h" + +#include + +#include + +namespace +{ + +using testing::_; +using testing::AtLeast; +using testing::Return; + +class PoSTransactionCreatorTestFixture +{ + +protected: + + FakeBlockIndexWithHashes fakeChain; + FakeWallet wallet; + +private: + + const CChainParams& chainParams; + + MockBlockIncentivesPopulator blockIncentivesPopulator; + MockBlockSubsidyProvider blockSubsidyProvider; + ProofOfStakeModule posModule; + + std::map hashedBlockTimestamps; + + PoSTransactionCreator txCreator; + +protected: + + /** A script from the wallet for convenience. */ + const CScript walletScript; + + /** UTXO hasher used in the wallet. The instance is owned by the wallet, + * and a reference is kept here. */ + MockUtxoHasher* utxoHasher; + + /* Convenience variables for tests to use in calls to CreatePoS. */ + CMutableTransaction mtx; + unsigned txTime; + + /* Convenience variable for the wallet's AddDefaultTx. */ + unsigned outputIndex; + + PoSTransactionCreatorTestFixture() + : fakeChain(1, 1600000000, 1), + wallet(fakeChain), + chainParams(Params(CBaseChainParams::REGTEST)), + posModule(chainParams, *fakeChain.activeChain, *fakeChain.blockIndexByHash), + txCreator(chainParams, *fakeChain.activeChain, *fakeChain.blockIndexByHash, + blockSubsidyProvider, blockIncentivesPopulator, + posModule.proofOfStakeGenerator(), wallet, hashedBlockTimestamps), + walletScript(GetScriptForDestination(wallet.getNewKey().GetID())) + { + std::unique_ptr hasher(new MockUtxoHasher()); + utxoHasher = hasher.get(); + wallet.SetUtxoHasherForTesting(std::move (hasher)); + + /* Set up a default block reward if we don't need anything else. */ + EXPECT_CALL(blockSubsidyProvider, GetFullBlockValue(_)) + .WillRepeatedly(Return(11 * COIN)); + EXPECT_CALL(blockSubsidyProvider, GetBlockSubsidity(_)) + .WillRepeatedly(Return(CBlockRewards(10 * COIN, COIN, 0, 0, 0, 0))); + + /* We don't care about the block payments. */ + EXPECT_CALL(blockIncentivesPopulator, FillBlockPayee(_, _, _, _)).Times(AtLeast(0)); + EXPECT_CALL(blockIncentivesPopulator, IsBlockValueValid(_, _, _)) + .WillRepeatedly(Return(true)); + EXPECT_CALL(blockIncentivesPopulator, HasValidPayees(_, _)) + .WillRepeatedly(Return(true)); + } + + /** Calls CreateProofOfStake on our PoSTransactionCreator with the + * fake wallet's chain tip and regtest difficulty. */ + bool CreatePoS(CMutableTransaction& txCoinStake, unsigned& nTxNewTime) + { + return txCreator.CreateProofOfStake(fakeChain.activeChain->Tip(), 0x207fffff, txCoinStake, nTxNewTime); + } + + bool CreatePoS() + { + return CreatePoS(mtx, txTime); + } + +}; + +BOOST_FIXTURE_TEST_SUITE(PoSTransactionCreator_tests, PoSTransactionCreatorTestFixture) + +BOOST_AUTO_TEST_CASE(failsWithoutCoinsInWallet) +{ + BOOST_CHECK(!CreatePoS()); +} + +BOOST_AUTO_TEST_CASE(checksForConfirmationsAndAge) +{ + const auto& tx = wallet.AddDefaultTx(walletScript, outputIndex, 1000 * COIN); + wallet.FakeAddToChain(tx); + + BOOST_CHECK(!CreatePoS()); + + wallet.AddConfirmations(20, 1000); + BOOST_CHECK(CreatePoS()); +} + +BOOST_AUTO_TEST_CASE(usesUtxoHashForInputs) +{ + const auto& tx = wallet.AddDefaultTx(walletScript, outputIndex, 1000 * COIN); + wallet.FakeAddToChain(tx); + wallet.AddConfirmations(20, 1000); + + const auto hash = utxoHasher->Add(tx); + BOOST_CHECK(CreatePoS()); + + BOOST_CHECK_EQUAL(mtx.vin.size(), 1); + BOOST_CHECK_EQUAL(mtx.vin[0].prevout.n, outputIndex); + BOOST_CHECK(mtx.vin[0].prevout.hash == hash); +} + +BOOST_AUTO_TEST_SUITE_END() + +} // anonymous namespace diff --git a/divi/src/wallet.cpp b/divi/src/wallet.cpp index e3673da8d..7c15b7f13 100644 --- a/divi/src/wallet.cpp +++ b/divi/src/wallet.cpp @@ -2410,6 +2410,11 @@ uint256 CWallet::GetUtxoHash(const CMerkleTx& tx) const return utxoHasher->GetUtxoHash(tx); } +void CWallet::SetUtxoHasherForTesting(std::unique_ptr hasher) +{ + utxoHasher = std::move(hasher); +} + DBErrors CWallet::LoadWallet(bool& fFirstRunRet) { if (!fFileBacked) diff --git a/divi/src/wallet.h b/divi/src/wallet.h index aa2e20cd1..ab4d4d12f 100644 --- a/divi/src/wallet.h +++ b/divi/src/wallet.h @@ -247,6 +247,9 @@ class CWallet : public CCryptoKeyStore, public NotificationInterface, public I_K * from the given transaction (which should be part of the wallet). */ uint256 GetUtxoHash(const CMerkleTx& tx) const; + /** Replaces the UTXO hasher used in the wallet, for testing purposes. */ + void SetUtxoHasherForTesting(std::unique_ptr hasher); + // keystore implementation // Generate a new key CPubKey GenerateNewKey(uint32_t nAccountIndex, bool fInternal); From e4c26c08e6337dab33305d89867ad6fd04f96c20 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 9 Feb 2021 15:41:24 +0100 Subject: [PATCH 08/16] Unit tests for UTXO hasher in wallet. Extend the unit tests in wallet_coinmanagement_tests.cpp to include also explicit checks for situations in which the wallet is supposed to the UTXO hasher rather than e.g. a plain transaction hash. --- divi/src/test/FakeWallet.cpp | 10 ++- divi/src/test/FakeWallet.h | 3 + divi/src/test/wallet_coinmanagement_tests.cpp | 68 ++++++++++++++++++- 3 files changed, 77 insertions(+), 4 deletions(-) diff --git a/divi/src/test/FakeWallet.cpp b/divi/src/test/FakeWallet.cpp index d912d234f..35c83045d 100644 --- a/divi/src/test/FakeWallet.cpp +++ b/divi/src/test/FakeWallet.cpp @@ -109,10 +109,8 @@ void FakeWallet::AddConfirmations(const unsigned numConf, const int64_t minAge) fakeChain.addBlocks(numConf, version, fakeChain.activeChain->Tip()->nTime + minAge); } -const CWalletTx& FakeWallet::AddDefaultTx(const CScript& scriptToPayTo, unsigned& outputIndex, - const CAmount amount) +const CWalletTx& FakeWallet::Add(const CTransaction& tx) { - const CMutableTransaction tx = createDefaultTransaction(scriptToPayTo, outputIndex, amount); const CMerkleTx merkleTx(tx, *fakeChain.activeChain, *fakeChain.blockIndexByHash); const CWalletTx wtx(merkleTx); AddToWallet(wtx); @@ -121,6 +119,12 @@ const CWalletTx& FakeWallet::AddDefaultTx(const CScript& scriptToPayTo, unsigned return *txPtr; } +const CWalletTx& FakeWallet::AddDefaultTx(const CScript& scriptToPayTo, unsigned& outputIndex, + const CAmount amount) +{ + return Add(createDefaultTransaction(scriptToPayTo, outputIndex, amount)); +} + void FakeWallet::FakeAddToChain(const CWalletTx& tx) { auto* txPtr = const_cast(&tx); diff --git a/divi/src/test/FakeWallet.h b/divi/src/test/FakeWallet.h index 1beff3b06..5f2962ca7 100644 --- a/divi/src/test/FakeWallet.h +++ b/divi/src/test/FakeWallet.h @@ -44,6 +44,9 @@ class FakeWallet : public CWallet * and number of confirmations. */ void AddConfirmations(unsigned numConf, int64_t minAge = 0); + /** Adds a given transaction to the wallet. */ + const CWalletTx& Add(const CTransaction& tx); + /** Adds a new ordinary transaction to the wallet, paying a given amount * to a given script. The transaction is returned, and the output index * with the output to the requested script is set. */ diff --git a/divi/src/test/wallet_coinmanagement_tests.cpp b/divi/src/test/wallet_coinmanagement_tests.cpp index 9b9843b76..34043d26e 100644 --- a/divi/src/test/wallet_coinmanagement_tests.cpp +++ b/divi/src/test/wallet_coinmanagement_tests.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include class WalletCoinManagementTestFixture @@ -22,10 +23,15 @@ class WalletCoinManagementTestFixture public: FakeBlockIndexWithHashes fakeChain; FakeWallet wallet; + MockUtxoHasher* utxoHasher; WalletCoinManagementTestFixture() : fakeChain(1, 1600000000, 1), wallet(fakeChain) - {} + { + std::unique_ptr hasher(new MockUtxoHasher()); + utxoHasher = hasher.get(); + wallet.SetUtxoHasherForTesting(std::move(hasher)); + } CScript vaultScriptAsOwner() const { @@ -394,4 +400,64 @@ BOOST_AUTO_TEST_CASE(willEnsureStakingBalanceAndTotalBalanceAgreeEvenIfTxsBelong BOOST_CHECK_EQUAL_MESSAGE(stakableCoins.size(),2,"Missing coins in the stakable set"); } +BOOST_AUTO_TEST_CASE(willUseUtxoHasherForCoinLock) +{ + CScript defaultScript = GetScriptForDestination(wallet.getNewKey().GetID()); + unsigned outputIndex=0; + const CWalletTx& wtx = wallet.AddDefaultTx(defaultScript,outputIndex, COIN); + const auto id = utxoHasher->Add(wtx); + + wallet.LockCoin(COutPoint(id, outputIndex)); + + bool fIsSpendable; + BOOST_CHECK(!wallet.IsAvailableForSpending(&wtx,outputIndex,nullptr,false,fIsSpendable)); + + BOOST_CHECK_EQUAL(wallet.ComputeCredit(wtx, ISMINE_SPENDABLE, REQUIRE_UNLOCKED), 0); + BOOST_CHECK_EQUAL(wallet.ComputeCredit(wtx, ISMINE_SPENDABLE, REQUIRE_LOCKED), COIN); +} + +BOOST_AUTO_TEST_CASE(willMarkOutputsSpentBasedOnUtxoHasher) +{ + CScript defaultScript = GetScriptForDestination(wallet.getNewKey().GetID()); + unsigned outputIndex=0; + const CWalletTx& wtx = wallet.AddDefaultTx(defaultScript,outputIndex, COIN); + const auto id = utxoHasher->Add(wtx); + + CMutableTransaction mtx; + mtx.vin.emplace_back(id, outputIndex); + const auto& spendTx = wallet.Add(mtx); + + /* The wallet's IsSpent check verifies also the spending tx' number + of confirmations, which requires it to be either in the chain or + in the mempool at least. */ + wallet.FakeAddToChain(wtx); + wallet.FakeAddToChain(spendTx); + + BOOST_CHECK(wallet.IsSpent(wtx, outputIndex)); + bool fIsSpendable; + BOOST_CHECK(!wallet.IsAvailableForSpending(&wtx,outputIndex,nullptr,false,fIsSpendable)); +} + +BOOST_AUTO_TEST_CASE(willUseUtxoHashForSpendingCoins) +{ + CScript defaultScript = GetScriptForDestination(wallet.getNewKey().GetID()); + unsigned outputIndex=0; + const CWalletTx& wtx = wallet.AddDefaultTx(defaultScript,outputIndex, COIN); + const auto id = utxoHasher->Add(wtx); + wallet.FakeAddToChain(wtx); + + const CScript sendTo = CScript() << OP_TRUE; + std::string strError; + CReserveKey reserveKey(wallet); + CAmount nFeeRequired; + CWalletTx wtxNew; + BOOST_CHECK(wallet.CreateTransaction(sendTo, COIN / 10, wtxNew, reserveKey, nFeeRequired, + strError, nullptr, ALL_SPENDABLE_COINS, false, 0)); + + BOOST_CHECK_EQUAL(wtxNew.vin.size(), 1); + const auto& prevout = wtxNew.vin[0].prevout; + BOOST_CHECK(prevout.hash == id); + BOOST_CHECK_EQUAL(prevout.n, outputIndex); +} + BOOST_AUTO_TEST_SUITE_END() From 676f1c2a5af6a1325b2f4bec0bbe1e1fa39b399c Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Thu, 17 Dec 2020 15:18:57 +0100 Subject: [PATCH 09/16] Use outputhash in regtests. This updates the regtests to use "outputhash" for listunspent results in some places, to make sure the code will also work after activating segwit-light. Some other places remain where outputs are not from listunspent and that still needs to be updated when segwit-light gets activated generally, but this is a first step to reduce the amount of required changes then. --- divi/qa/rpc-tests/BlocksOnlyHaveSingleCoinstake.py | 2 +- divi/qa/rpc-tests/op_meta.py | 11 ++++++++--- divi/qa/rpc-tests/rawtransactions.py | 2 +- divi/qa/rpc-tests/smartfees.py | 14 ++++++++++++-- divi/qa/rpc-tests/util.py | 13 +------------ divi/qa/rpc-tests/vaultfork.py | 5 +++-- divi/qa/rpc-tests/wallet.py | 2 +- 7 files changed, 27 insertions(+), 22 deletions(-) diff --git a/divi/qa/rpc-tests/BlocksOnlyHaveSingleCoinstake.py b/divi/qa/rpc-tests/BlocksOnlyHaveSingleCoinstake.py index 190aa7a75..08a4b0563 100755 --- a/divi/qa/rpc-tests/BlocksOnlyHaveSingleCoinstake.py +++ b/divi/qa/rpc-tests/BlocksOnlyHaveSingleCoinstake.py @@ -42,7 +42,7 @@ def build_coinstake_tx (self): tx = CTransaction () tx.vout.append( CTxOut(0, CScript() ) ) tx.vout.append( CTxOut(amountToSend, scriptToSendTo ) ) - tx.vin.append (CTxIn (COutPoint (txid=inp["txid"], n=inp["vout"]))) + tx.vin.append (CTxIn (COutPoint (txid=inp["outputhash"], n=inp["vout"]))) unsigned = tx.serialize ().hex () diff --git a/divi/qa/rpc-tests/op_meta.py b/divi/qa/rpc-tests/op_meta.py index 87b016cf7..ae90f6f02 100755 --- a/divi/qa/rpc-tests/op_meta.py +++ b/divi/qa/rpc-tests/op_meta.py @@ -46,7 +46,11 @@ def build_op_meta_tx (self, utxos, payload, fee): inp = None for i in range (len (utxos)): if utxos[i]["amount"] >= required: - inp = utxos[i] + inp = { + "txid": utxos[i]["outputhash"], + "vout": utxos[i]["vout"], + "amount": utxos[i]["amount"], + } del utxos[i] break assert inp is not None, "found no suitable output" @@ -59,8 +63,9 @@ def build_op_meta_tx (self, utxos, payload, fee): tx = self.node.createrawtransaction ([inp], {changeAddr: change}) signed = self.node.signrawtransaction (tx) assert_equal (signed["complete"], True) - txid = self.node.sendrawtransaction (signed["hex"]) - inp["txid"] = txid + data = self.node.decoderawtransaction (signed["hex"]) + self.node.sendrawtransaction (signed["hex"]) + inp["txid"] = data["txid"] inp["vout"] = 0 inp["amount"] = change diff --git a/divi/qa/rpc-tests/rawtransactions.py b/divi/qa/rpc-tests/rawtransactions.py index 1696f51ba..6a83c084a 100755 --- a/divi/qa/rpc-tests/rawtransactions.py +++ b/divi/qa/rpc-tests/rawtransactions.py @@ -26,7 +26,7 @@ def find_output (self, node, value): for u in node.listunspent (): if u["amount"] == value: - return {"txid": u["txid"], "vout": u["vout"]} + return {"txid": u["outputhash"], "vout": u["vout"]} raise AssertionError ("no output with value %s found" % str (value)) diff --git a/divi/qa/rpc-tests/smartfees.py b/divi/qa/rpc-tests/smartfees.py index d8ce056ff..376c1fb18 100755 --- a/divi/qa/rpc-tests/smartfees.py +++ b/divi/qa/rpc-tests/smartfees.py @@ -12,6 +12,17 @@ from util import * +def find_output(node, txid, amount): + """ + Return index to output of txid with value amount + Raises exception if there is none. + """ + txdata = node.getrawtransaction(txid, 1) + for i in range(len(txdata["vout"])): + if txdata["vout"][i]["value"] == amount: + return {"txid": txdata["txid"], "vout": i} + raise RuntimeError("find_output txid %s : %s not found"%(txid,str(amount))) + def send_zeropri_transaction(from_node, to_node, amount, fee): """ Create&broadcast a zero-priority transaction. @@ -31,10 +42,9 @@ def send_zeropri_transaction(from_node, to_node, amount, fee): self_signresult = from_node.signrawtransaction(self_rawtx) self_txid = from_node.sendrawtransaction(self_signresult["hex"], True) - vout = find_output(from_node, self_txid, amount+fee) # Now immediately spend the output to create a 1-input, 1-output # zero-priority transaction: - inputs = [ { "txid" : self_txid, "vout" : vout } ] + inputs = [find_output(from_node, self_txid, amount + fee)] outputs = { to_node.getnewaddress() : float(amount) } rawtx = from_node.createrawtransaction(inputs, outputs) diff --git a/divi/qa/rpc-tests/util.py b/divi/qa/rpc-tests/util.py index a4f1f07bc..294b65301 100644 --- a/divi/qa/rpc-tests/util.py +++ b/divi/qa/rpc-tests/util.py @@ -192,17 +192,6 @@ def connect_nodes_bi(nodes, a, b): connect_nodes(nodes[a], b) connect_nodes(nodes[b], a) -def find_output(node, txid, amount): - """ - Return index to output of txid with value amount - Raises exception if there is none. - """ - txdata = node.getrawtransaction(txid, 1) - for i in range(len(txdata["vout"])): - if txdata["vout"][i]["value"] == amount: - return i - raise RuntimeError("find_output txid %s : %s not found"%(txid,str(amount))) - def gather_inputs(from_node, amount_needed, confirmations_required=1): """ Return a random set of unspent txouts that are enough to pay amount_needed @@ -215,7 +204,7 @@ def gather_inputs(from_node, amount_needed, confirmations_required=1): while total_in < amount_needed and len(utxo) > 0: t = utxo.pop() total_in += t["amount"] - inputs.append({ "txid" : t["txid"], "vout" : t["vout"], "address" : t["address"] } ) + inputs.append({ "txid" : t["outputhash"], "vout" : t["vout"], "address" : t["address"] } ) if total_in < amount_needed: raise RuntimeError("Insufficient funds: need %d, have %d"%(amount_needed, total_in)) return (total_in, inputs) diff --git a/divi/qa/rpc-tests/vaultfork.py b/divi/qa/rpc-tests/vaultfork.py index be5ac735b..638aeab64 100755 --- a/divi/qa/rpc-tests/vaultfork.py +++ b/divi/qa/rpc-tests/vaultfork.py @@ -31,10 +31,11 @@ def fund_vault (self, owner, staker, amount): """ txid = owner.fundvault (staker.getnewaddress (), amount)["txhash"] - outputs = owner.getrawtransaction (txid, 1)["vout"] + data = owner.getrawtransaction (txid, 1) + outputs = data["vout"] for n in range (len (outputs)): if outputs[n]["scriptPubKey"]["type"] == "vault": - return {"txid": txid, "vout": n} + return {"txid": data["txid"], "vout": n} raise AssertionError ("constructed transaction has no vault output") diff --git a/divi/qa/rpc-tests/wallet.py b/divi/qa/rpc-tests/wallet.py index 06038a16b..2beaeeb6a 100755 --- a/divi/qa/rpc-tests/wallet.py +++ b/divi/qa/rpc-tests/wallet.py @@ -70,7 +70,7 @@ def run_test (self): for utxo in node0utxos: inputs = [] outputs = {} - inputs.append({ "txid" : utxo["txid"], "vout" : utxo["vout"]}) + inputs.append({ "txid" : utxo["outputhash"], "vout" : utxo["vout"]}) outputs[self.nodes[2].getnewaddress("from1")] = utxo["amount"] raw_tx = self.nodes[0].createrawtransaction(inputs, outputs) txns_to_send.append(self.nodes[0].signrawtransaction(raw_tx)) From 31ca4045fe5bcc68dd368a39bfd38cc371f39804 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Thu, 21 Jan 2021 15:26:15 +0100 Subject: [PATCH 10/16] Consensus and mempool parts of segwit-light. This implements "segwit-light" in the node (consensus and mempool): After activation of a new fork, outputs created by new transactions are referred to by the creating transaction's bare txid rather than the normal hash (txid). This makes chains of constructed transactions immune to transaction malleability. In the mempool, we use the current chain tip to determine the activation state of the fork, as we can't know for sure when a transaction will be confirmed (and thus what activation state will be in effect then). This will lead to issues right around the fork activation time, but we will simply disallow spending of unconfirmed outputs in the mempool and wallet "around" the fork time temporarily to resolve this. The wallet is also not yet updated to take the change into account when selecting coins. --- divi/src/ActiveChainManager.cpp | 4 +++- divi/src/ForkActivation.cpp | 5 ++++- divi/src/ForkActivation.h | 12 ++++++++++-- divi/src/UtxoCheckingAndUpdating.cpp | 3 +++ divi/src/UtxoCheckingAndUpdating.h | 10 ++++++++++ divi/src/main.cpp | 7 +++++-- divi/src/rpcblockchain.cpp | 2 +- divi/src/txmempool.cpp | 10 +++++++++- 8 files changed, 45 insertions(+), 8 deletions(-) diff --git a/divi/src/ActiveChainManager.cpp b/divi/src/ActiveChainManager.cpp index 867074ef7..2ff0131eb 100644 --- a/divi/src/ActiveChainManager.cpp +++ b/divi/src/ActiveChainManager.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -87,7 +88,8 @@ bool ActiveChainManager::DisconnectBlock( return error("DisconnectBlock() : block and undo data inconsistent"); } - const BlockUtxoHasher utxoHasher; + const ActivationState as(pindex); + const BlockUtxoHasher utxoHasher(as); bool fClean = true; IndexDatabaseUpdates indexDBUpdates; diff --git a/divi/src/ForkActivation.cpp b/divi/src/ForkActivation.cpp index a05e2dd35..ea12e21f1 100644 --- a/divi/src/ForkActivation.cpp +++ b/divi/src/ForkActivation.cpp @@ -25,12 +25,15 @@ const std::unordered_map> ACTIVATION_TIMES = { {Fork::TestByTimestamp, 1000000000}, {Fork::HardenedStakeModifier, unixTimestampForDec31stMidnight}, {Fork::UniformLotteryWinners, unixTimestampForDec31stMidnight}, + /* FIXME: Set real activation time for segwit light. It is after + staking vaults. */ + {Fork::SegwitLight, 2000000000}, }; } // anonymous namespace ActivationState::ActivationState(const CBlockIndex* pi) - : nTime(pi->nTime) + : nTime(pi == nullptr ? 0 : pi->nTime) {} ActivationState::ActivationState(const CBlockHeader& block) diff --git a/divi/src/ForkActivation.h b/divi/src/ForkActivation.h index 79aeb7ba6..7dabc35da 100644 --- a/divi/src/ForkActivation.h +++ b/divi/src/ForkActivation.h @@ -18,10 +18,18 @@ class CBlockIndex; */ enum Fork { - /* Test forks not actually deployed / active but used for unit tests. */ - TestByTimestamp, HardenedStakeModifier, UniformLotteryWinners, + + /** + * Start of "segwit light": The UTXOs created by transactions from after + * the fork will be indexed by a "bare txid", which does not include + * any signature data. This fixes transaction malleability. + */ + SegwitLight, + + /* Test forks not actually deployed / active but used for unit tests. */ + TestByTimestamp, }; /** diff --git a/divi/src/UtxoCheckingAndUpdating.cpp b/divi/src/UtxoCheckingAndUpdating.cpp index b0aeef600..fb969a9c5 100644 --- a/divi/src/UtxoCheckingAndUpdating.cpp +++ b/divi/src/UtxoCheckingAndUpdating.cpp @@ -1,5 +1,6 @@ #include +#include #include #include #include @@ -16,6 +17,8 @@ extern BlockMap mapBlockIndex; uint256 BlockUtxoHasher::GetUtxoHash(const CTransaction& tx) const { + if (as.IsActive(Fork::SegwitLight)) + return tx.GetBareTxid(); return tx.GetHash(); } diff --git a/divi/src/UtxoCheckingAndUpdating.h b/divi/src/UtxoCheckingAndUpdating.h index 318286292..98055eb70 100644 --- a/divi/src/UtxoCheckingAndUpdating.h +++ b/divi/src/UtxoCheckingAndUpdating.h @@ -5,6 +5,7 @@ #include #include +class ActivationState; class CTransaction; class CValidationState; class CCoinsViewCache; @@ -53,8 +54,17 @@ class TransactionUtxoHasher class BlockUtxoHasher : public TransactionUtxoHasher { +private: + + /** The activation state used for checking on segwit light. */ + const ActivationState& as; + public: + explicit BlockUtxoHasher(const ActivationState& a) + : as(a) + {} + uint256 GetUtxoHash(const CTransaction& tx) const override; }; diff --git a/divi/src/main.cpp b/divi/src/main.cpp index 091e9928c..5aedd02fa 100755 --- a/divi/src/main.cpp +++ b/divi/src/main.cpp @@ -15,6 +15,7 @@ #include "BlockSigning.h" #include "checkpoints.h" #include "checkqueue.h" +#include "ForkActivation.h" #include "init.h" #include "kernel.h" #include "masternode-payments.h" @@ -1433,7 +1434,8 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin LogWalletBalance(pwalletMain); static const CChainParams& chainParameters = Params(); - const BlockUtxoHasher utxoHasher; + const ActivationState as(block); + const BlockUtxoHasher utxoHasher(as); VerifyBestBlockIsAtPreviousBlock(pindex,view); if (block.GetHash() == Params().HashGenesisBlock()) @@ -2739,7 +2741,8 @@ 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; + const ActivationState as(block); + const BlockUtxoHasher utxoHasher(as); std::vector vtxundo; vtxundo.reserve(block.vtx.size() - 1); diff --git a/divi/src/rpcblockchain.cpp b/divi/src/rpcblockchain.cpp index fe476dded..8ddef22e7 100644 --- a/divi/src/rpcblockchain.cpp +++ b/divi/src/rpcblockchain.cpp @@ -406,7 +406,7 @@ Value gettxout(const Array& params, bool fHelp) "gettxout \"txid\" n ( includemempool )\n" "\nReturns details about an unspent transaction output.\n" "\nArguments:\n" - "1. \"txid\" (string, required) The transaction id\n" + "1. \"txid\" (string, required) The transaction id or bare txid (after segwit-light)\n" "2. n (numeric, required) vout value\n" "3. includemempool (boolean, optional) Whether to included the mem pool\n" "\nResult:\n" diff --git a/divi/src/txmempool.cpp b/divi/src/txmempool.cpp index b32f834d1..3ab33c621 100644 --- a/divi/src/txmempool.cpp +++ b/divi/src/txmempool.cpp @@ -7,6 +7,7 @@ #include "txmempool.h" #include "clientversion.h" +#include "ForkActivation.h" #include "main.h" #include "streams.h" #include "Logging.h" @@ -362,7 +363,11 @@ class CMinerPolicyEstimator namespace { -/** The UTXO hasher used in mempool logic. */ +/** The UTXO hasher used in mempool logic. It uses the state of segwit-light + * based on the latest chain tip (since we can't know when a particular + * transaction will be confirmed / what the real activation state will be then). + * Spending of unconfirmed change around the fork will be discouraged by + * policy, so that this should not be an issue in practice. */ class MempoolUtxoHasher : public TransactionUtxoHasher { @@ -370,6 +375,9 @@ class MempoolUtxoHasher : public TransactionUtxoHasher uint256 GetUtxoHash(const CTransaction& tx) const override { + const ActivationState as(chainActive.Tip()); + if (as.IsActive(Fork::SegwitLight)) + return tx.GetBareTxid(); return tx.GetHash(); } From 8467c6efcd0d036d84a3ff4c55168dd18b53f095 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Fri, 15 Jan 2021 15:59:32 +0100 Subject: [PATCH 11/16] Restrict unconfirmed change around fork. Around the segwit-light fork (some hours before and after), we should avoid spending unconfirmed outputs: If we do that, it is unclear whether or not the fork will be active in the block that confirms those spends, and thus we can't reliably construct a valid chain. With this change, we introduce a (temporary) measure to disallow those spends in the wallet and mempool policy in a window of time around the planned fork activation. --- divi/qa/rpc-tests/around_segwit_light.py | 136 +++++++++++++++++++++++ divi/qa/rpc-tests/test_runner.py | 1 + divi/src/ForkActivation.cpp | 9 ++ divi/src/ForkActivation.h | 9 ++ divi/src/main.cpp | 25 ++++- divi/src/wallet.cpp | 18 ++- divi/src/wallet.h | 5 + 7 files changed, 200 insertions(+), 3 deletions(-) create mode 100755 divi/qa/rpc-tests/around_segwit_light.py diff --git a/divi/qa/rpc-tests/around_segwit_light.py b/divi/qa/rpc-tests/around_segwit_light.py new file mode 100755 index 000000000..8eb831c74 --- /dev/null +++ b/divi/qa/rpc-tests/around_segwit_light.py @@ -0,0 +1,136 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 The DIVI developers +# Distributed under the MIT/X11 software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +# Tests the policy changes in wallet and mempool around the +# segwit-light activation time that prevent spending of +# unconfirmed outputs. + +from test_framework import BitcoinTestFramework +from authproxy import JSONRPCException +from util import * + +ACTIVATION_TIME = 2_100_000_000 + +# Offset around the activation time where no restrictions are in place. +NO_RESTRICTIONS = 24 * 3_600 + +# Offset around the activation time where the wallet disallows +# spending unconfirmed change, but the mempool still accepts it. +WALLET_RESTRICTED = 10 * 3_600 + +# Offset around the activation time where wallet and mempool +# disallow spending unconfirmed outputs. +MEMPOOL_RESTRICTED = 3_600 + + +class AroundSegwitLightTest (BitcoinTestFramework): + + def setup_network (self, split=False): + args = ["-debug", "-spendzeroconfchange"] + self.nodes = start_nodes (1, self.options.tmpdir, extra_args=[args]) + self.node = self.nodes[0] + self.is_network_split = False + + def build_spending_chain (self, key, addr, initialValue): + """ + Spends the initialValue to addr, and then builds a follow-up + transaction that spends that output again back to addr with + a smaller value (for fees). The second transaction is built + using the raw-transactions API and returned as hex, not + submitted to the node already. + """ + + txid = self.node.sendtoaddress (addr, initialValue) + data = self.node.getrawtransaction (txid, 1) + outputIndex = None + for i in range (len (data["vout"])): + if data["vout"][i]["value"] == initialValue: + outputIndex = i + break + assert outputIndex is not None + + inputs = [{"txid": data[key], "vout": outputIndex}] + outputs = {addr: initialValue - 10} + tx = self.node.createrawtransaction (inputs, outputs) + signed = self.node.signrawtransaction (tx) + assert signed["complete"] + + return signed["hex"] + + def expect_unrestricted (self): + """ + Checks that spending of unconfirmed change is possible without + restrictions of wallet or mempool. + """ + + balance = self.node.getbalance () + addr = self.node.getnewaddress () + + self.node.sendtoaddress (addr, balance - 10) + self.node.sendtoaddress (addr, balance - 20) + self.node.setgenerate (True, 1) + + def expect_wallet_restricted (self, key): + """ + Checks that the wallet forbids spending unconfirmed change, + while the mempool still allows it. + """ + + balance = self.node.getbalance () + addr = self.node.getnewaddress () + + self.node.sendtoaddress (addr, balance - 10) + assert_raises (JSONRPCException, self.node.sendtoaddress, addr, balance - 20) + self.node.setgenerate (True, 1) + + balance = self.node.getbalance () + tx = self.build_spending_chain (key, addr, balance - 10) + self.node.sendrawtransaction (tx) + self.node.setgenerate (True, 1) + + def expect_mempool_restricted (self, key): + """ + Checks that the mempool does not allow spending unconfirmed + outputs (even if the transaction is built and submitted directly), + while blocks should still allow it. + """ + + balance = self.node.getbalance () + addr = self.node.getnewaddress () + + tx = self.build_spending_chain (key, addr, balance - 10) + assert_raises (JSONRPCException, self.node.sendrawtransaction, tx) + self.node.generateblock ({"extratx": [tx]}) + + def run_test (self): + self.node.setgenerate (True, 30) + + # Before restrictions come into force, doing a normal + # spend of unconfirmed change through the wallet is fine. + set_node_times (self.nodes, ACTIVATION_TIME - NO_RESTRICTIONS) + self.expect_unrestricted () + + # Next the wallet doesn't allow those spends, but the mempool + # will (if done directly). + set_node_times (self.nodes, ACTIVATION_TIME - WALLET_RESTRICTED) + self.expect_wallet_restricted ("txid") + + # Very close to the fork (on both sides), even the mempool won't + # allow spending unconfirmed change. If we include it directly in + # a block, it works. + set_node_times (self.nodes, ACTIVATION_TIME - MEMPOOL_RESTRICTED) + self.expect_mempool_restricted ("txid") + set_node_times (self.nodes, ACTIVATION_TIME + MEMPOOL_RESTRICTED) + self.expect_mempool_restricted ("txid") + + # Finally, we should run into mempool-only or no restrictions + # at all if we go further into the future, away from the fork. + set_node_times (self.nodes, ACTIVATION_TIME + WALLET_RESTRICTED) + self.expect_wallet_restricted ("txid") + set_node_times (self.nodes, ACTIVATION_TIME + NO_RESTRICTIONS) + self.expect_unrestricted () + +if __name__ == '__main__': + AroundSegwitLightTest ().main () diff --git a/divi/qa/rpc-tests/test_runner.py b/divi/qa/rpc-tests/test_runner.py index ab807ee38..f5033bb0c 100755 --- a/divi/qa/rpc-tests/test_runner.py +++ b/divi/qa/rpc-tests/test_runner.py @@ -78,6 +78,7 @@ 'StakingVaultStaking.py', 'StakingVaultDeactivation.py', 'StakingVaultSpam.py', + 'around_segwit_light.py', 'forknotify.py', 'getchaintips.py', 'httpbasics.py', diff --git a/divi/src/ForkActivation.cpp b/divi/src/ForkActivation.cpp index ea12e21f1..5ea053f9c 100644 --- a/divi/src/ForkActivation.cpp +++ b/divi/src/ForkActivation.cpp @@ -6,7 +6,9 @@ #include "chain.h" #include "primitives/block.h" +#include "timedata.h" +#include #include #include @@ -52,3 +54,10 @@ bool ActivationState::IsActive(const Fork f) const assert(mit != ACTIVATION_TIMES.end()); return nTime >= mit->second; } + +bool ActivationState::CloseToSegwitLight(const int maxSeconds) +{ + const int64_t now = GetAdjustedTime(); + const int64_t activation = ACTIVATION_TIMES.at(Fork::SegwitLight); + return std::abs(now - activation) <= maxSeconds; +} diff --git a/divi/src/ForkActivation.h b/divi/src/ForkActivation.h index 7dabc35da..d84ab26c3 100644 --- a/divi/src/ForkActivation.h +++ b/divi/src/ForkActivation.h @@ -62,6 +62,15 @@ class ActivationState */ bool IsActive(Fork f) const; + /** + * Returns true if the current time is "close" to the activation of + * segwit-light (before or after), with close being within the given + * number of seconds. This is used for a temporary measure to disallow + * (by mempool and wallet policy) spending of unconfirmed change + * around the fork. + */ + static bool CloseToSegwitLight(int maxSeconds); + }; #endif // FORK_ACTIVATION_H diff --git a/divi/src/main.cpp b/divi/src/main.cpp index 5aedd02fa..57627b981 100755 --- a/divi/src/main.cpp +++ b/divi/src/main.cpp @@ -93,6 +93,14 @@ bool fIsBareMultisigStd = true; bool fCheckBlockIndex = false; bool fVerifyingBlocks = false; unsigned int nCoinCacheSize = 5000; + +/** Number of seconds around the "segwit light" fork for which we disallow + * spending unconfirmed outputs (to avoid messing up with the change + * itself). This should be smaller than the matching constant for the + * wallet (so the wallet does not construct things the mempool won't + * accept in the end). */ +constexpr int SEGWIT_LIGHT_FORBID_SPENDING_ZERO_CONF_SECONDS = 14400; + bool fAlerts = DEFAULT_ALERTS; bool IsFinalTx(const CTransaction& tx, const CChain& activeChain, int nBlockHeight = 0 , int64_t nBlockTime = 0); @@ -894,7 +902,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState& state, const CTransa // do all inputs exist? // Note that this does not check for the presence of actual outputs (see the next check for that), // only helps filling in pfMissingInputs (to determine missing vs spent). - for (const CTxIn txin : tx.vin) { + for (const auto& txin : tx.vin) { if (!view.HaveCoins(txin.prevout.hash)) { if (pfMissingInputs) *pfMissingInputs = true; @@ -907,6 +915,21 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState& state, const CTransa return state.Invalid(error("AcceptToMemoryPool : inputs already spent"), REJECT_DUPLICATE, "bad-txns-inputs-spent"); + // If we are close to the segwit-light activation (before or after), + // do not allow spending of zero-confirmation outputs. This would + // mess up with the fork, as it is not clear whether the fork will be + // active or not when they get confirmed (and thus it is not possible + // to reliably construct a chain of transactions). + if (ActivationState::CloseToSegwitLight(SEGWIT_LIGHT_FORBID_SPENDING_ZERO_CONF_SECONDS)) { + for (const auto& txin : tx.vin) { + const auto* coins = view.AccessCoins(txin.prevout.hash); + assert(coins != nullptr && coins->IsAvailable(txin.prevout.n)); + if (coins->nHeight == MEMPOOL_HEIGHT) + return state.DoS(0, error("%s : spending zero-confirmation output around segwit light", __func__), + REJECT_NONSTANDARD, "zero-conf-segwit-light"); + } + } + // Bring the best block into scope view.GetBestBlock(); diff --git a/divi/src/wallet.cpp b/divi/src/wallet.cpp index 7c15b7f13..357f90a38 100644 --- a/divi/src/wallet.cpp +++ b/divi/src/wallet.cpp @@ -14,6 +14,7 @@ #include #include "coincontrol.h" #include +#include "ForkActivation.h" #include "masternode-payments.h" #include "net.h" #include "script/script.h" @@ -60,6 +61,13 @@ bool fSendFreeTransactions = false; bool fPayAtLeastCustomFee = true; static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000; +/** Number of seconds around the "segwit light" fork for which we force + * not spending unconfirmed change (to avoid messing up with the change + * itself). This should be larger than the matching constant for the + * mempool (so the wallet does not construct things the mempool won't + * accept in the end). */ +constexpr int SEGWIT_LIGHT_DISABLE_SPENDING_ZERO_CONF_SECONDS = 43200; + /** * Fees smaller than this (in duffs) are considered zero fee (for transaction creation) * We are ~100 times smaller then bitcoin now (2015-06-23), set minTxFee 10 times higher @@ -2066,6 +2074,12 @@ void CWallet::UpdateNextTransactionIndexAvailable(int64_t transactionIndex) orderedTransactionIndex = transactionIndex; } +bool CWallet::AllowSpendingZeroConfirmationChange() const +{ + return allowSpendingZeroConfirmationOutputs + && !ActivationState::CloseToSegwitLight(SEGWIT_LIGHT_DISABLE_SPENDING_ZERO_CONF_SECONDS); +} + bool CWallet::SelectCoins(const CAmount& nTargetValue, set >& setCoinsRet, CAmount& nValueRet, const CCoinControl* coinControl, AvailableCoinsType coin_type, bool useIX) const { // Note: this function should never be used for "always free" tx types like dstx @@ -2087,7 +2101,7 @@ bool CWallet::SelectCoins(const CAmount& nTargetValue, set >& vecSend, @@ -2920,7 +2934,7 @@ bool CWallet::IsTrusted(const CWalletTx& walletTransaction) const return true; if (nDepth < 0) return false; - if (!allowSpendingZeroConfirmationOutputs || !DebitsFunds(walletTransaction, ISMINE_ALL)) // using wtx's cached debit + if (!AllowSpendingZeroConfirmationChange() || !DebitsFunds(walletTransaction, ISMINE_ALL)) // using wtx's cached debit return false; // Trusted if all inputs are from us and are in the mempool: diff --git a/divi/src/wallet.h b/divi/src/wallet.h index ab4d4d12f..85094daac 100644 --- a/divi/src/wallet.h +++ b/divi/src/wallet.h @@ -185,6 +185,11 @@ class CWallet : public CCryptoKeyStore, public NotificationInterface, public I_K //it was public bool SelectCoins(int64_t nTargetValue, std::set >& setCoinsRet, int64_t& nValueRet, const CCoinControl *coinControl = NULL, AvailableCoinsType coin_type=ALL_SPENDABLE_COINS, bool useIX = true) const; void DeriveNewChildKey(const CKeyMetadata& metadata, CKey& secretRet, uint32_t nAccountIndex, bool fInternal /*= false*/); + /** Returns true if the wallet should allow spending of unconfirmed change. + * This is mostly determined by allowSpendingZeroConfirmationOutputs, + * but is forced to off around the segwit-light fork. */ + bool AllowSpendingZeroConfirmationChange() const; + public: bool MoveFundsBetweenAccounts(std::string from, std::string to, CAmount amount, std::string comment); From f6b7ac8be62dba2087896389527627b58b52ec70 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 26 Jan 2021 16:15:43 +0100 Subject: [PATCH 12/16] Segwit light for spending coins. Handle segwit-light properly when spending coins, either for staking or normal transactions in the wallet. Depending on when a transaction was confirmed, we need to make sure to include the right outpoint (with txid or bare txid) in the transaction spending an output. This is done through a custom UTXO hasher used in the wallet, which specifically works for CWalletTx. --- divi/src/StakableCoin.h | 8 +++---- divi/src/kernel.cpp | 2 +- divi/src/merkletx.h | 2 +- divi/src/primitives/transaction.h | 2 ++ divi/src/wallet.cpp | 36 +++++++++++++++++++++++++------ 5 files changed, 38 insertions(+), 12 deletions(-) diff --git a/divi/src/StakableCoin.h b/divi/src/StakableCoin.h index 78f0a3ff2..5d48e052a 100644 --- a/divi/src/StakableCoin.h +++ b/divi/src/StakableCoin.h @@ -1,10 +1,10 @@ #ifndef STAKABLE_COIN_H #define STAKABLE_COIN_H +#include #include -#include struct StakableCoin { - const CTransaction* tx; + const CMerkleTx* tx; unsigned outputIndex; uint256 blockHashOfFirstConfirmation; COutPoint utxo; @@ -18,7 +18,7 @@ struct StakableCoin } StakableCoin( - const CTransaction* txIn, + const CMerkleTx* txIn, unsigned outputIndexIn, uint256 blockHashIn ): tx(txIn) @@ -32,4 +32,4 @@ struct StakableCoin return utxo < other.utxo; } }; -#endif//STAKABLE_COIN_H \ No newline at end of file +#endif//STAKABLE_COIN_H diff --git a/divi/src/kernel.cpp b/divi/src/kernel.cpp index fbb9b8a89..115cc5d2e 100644 --- a/divi/src/kernel.cpp +++ b/divi/src/kernel.cpp @@ -254,7 +254,7 @@ bool CheckProofOfStakeContextAndRecoverStakingData( uint256 hashBlock; CTransaction txPrev; if (!GetTransaction(txin.prevout.hash, txPrev, hashBlock, true)) - return error("CheckProofOfStake() : INFO: read txPrev failed"); + return error("CheckProofOfStake() : INFO: read txPrev failed for %s", txin.prevout.hash.GetHex()); const CScript &kernelScript = txPrev.vout[txin.prevout.n].scriptPubKey; diff --git a/divi/src/merkletx.h b/divi/src/merkletx.h index fa448ea35..99433907d 100644 --- a/divi/src/merkletx.h +++ b/divi/src/merkletx.h @@ -58,4 +58,4 @@ class CMerkleTx : public CTransaction int GetTransactionLockSignatures() const; bool IsTransactionLockTimedOut() const; }; -#endif// MERKLE_TX_H \ No newline at end of file +#endif// MERKLE_TX_H diff --git a/divi/src/primitives/transaction.h b/divi/src/primitives/transaction.h index 6a51bb50a..338411293 100644 --- a/divi/src/primitives/transaction.h +++ b/divi/src/primitives/transaction.h @@ -200,6 +200,8 @@ class CTransaction CTransaction& operator=(const CTransaction& tx); + virtual ~CTransaction() = default; + ADD_SERIALIZE_METHODS; template diff --git a/divi/src/wallet.cpp b/divi/src/wallet.cpp index 357f90a38..b36e50e76 100644 --- a/divi/src/wallet.cpp +++ b/divi/src/wallet.cpp @@ -189,28 +189,52 @@ bool WriteTxToDisk(const CWallet* walletPtr, const CWalletTx& transactionToWrite namespace { -/** Dummy UTXO hasher for the wallet. For now, this just always returns - * the normal txid, but we will later change it to return the proper hash - * for a WalletTx. */ +/** UTXO hasher for wallet transactions. It uses the transaction's known block + * hash (from CMerkleTx) to determine the activation state of segwit light. */ class WalletUtxoHasher : public TransactionUtxoHasher { +private: + + const CChain& chainActive_; + public: + WalletUtxoHasher(const CChain& chain) + : chainActive_(chain) + {} + uint256 GetUtxoHash(const CTransaction& tx) const override { - return tx.GetHash(); + const CMerkleTx* mtx = dynamic_cast(&tx); + assert(mtx != nullptr); + + const CBlockIndex* pindexBlock = nullptr; + const int depth = mtx->GetNumberOfBlockConfirmations(pindexBlock); + + /* If the transaction is not yet confirmed in a block, we use the current + tip to determine the segwit-light activation status. This is not + perfect around the activation time, but there is nothing we can do + in that case anyway. Mempool and wallet discourage spending unconfirmed + outputs around the segwit-light fork anyway. */ + if (depth <= 0) + pindexBlock = chainActive_.Tip(); + + assert(pindexBlock != nullptr); + const ActivationState as(pindexBlock); + + return as.IsActive(Fork::SegwitLight) ? mtx->GetBareTxid() : mtx->GetHash(); } }; -} // anonymous namespace +} CWallet::CWallet(const CChain& chain, const BlockMap& blockMap ): cs_wallet() , transactionRecord_(new WalletTransactionRecord(cs_wallet,strWalletFile) ) , outputTracker_( new SpentOutputTracker(*transactionRecord_) ) - , utxoHasher(new WalletUtxoHasher() ) + , utxoHasher(new WalletUtxoHasher(chain) ) , chainActive_(chain) , mapBlockIndex_(blockMap) , orderedTransactionIndex() From d4d1d6aad5115ba6be8e68116100450417ce2abc Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Thu, 10 Dec 2020 15:15:38 +0100 Subject: [PATCH 13/16] Bare txid for GetTransaction / GetWalletTx. This extends the two functions GetWalletTx and GetTransaction to (also) find transactions by bare txid, not just by the normal txid. These methods are mainly used in places where we need to look up e.g. the previous transaction to a spend, so that we can know the address that is being spent or the value of the input. The change is fine to do (won't cause any extra consensus changes) because all it does is make those methods return the correct previous transaction (for after the fork) in cases where they would have failed otherwise (since both are SHA-256d hashes and thus cannot have collisions). Also actual checks that some spent coin actually exists are the explicitly on the consensus-level anyway. --- divi/src/TransactionDiskAccessor.cpp | 11 ++++++----- divi/src/WalletTransactionRecord.cpp | 26 +++++++++++++++++++++----- divi/src/WalletTransactionRecord.h | 10 +++++++++- divi/src/rpcmasternode.cpp | 6 +++--- 4 files changed, 39 insertions(+), 14 deletions(-) diff --git a/divi/src/TransactionDiskAccessor.cpp b/divi/src/TransactionDiskAccessor.cpp index fcf6d2697..e4608c875 100644 --- a/divi/src/TransactionDiskAccessor.cpp +++ b/divi/src/TransactionDiskAccessor.cpp @@ -24,7 +24,7 @@ bool GetTransaction(const uint256& hash, CTransaction& txOut, uint256& hashBlock { LOCK(cs_main); { - if (mempool.lookup(hash, txOut)) { + if (mempool.lookup(hash, txOut) || mempool.lookupBareTxid(hash, txOut)) { return true; } } @@ -49,8 +49,9 @@ bool GetTransaction(const uint256& hash, CTransaction& txOut, uint256& hashBlock return true; } - // transaction not found in the index, nothing more can be done - return false; + // The index only keys by txid. So even if we did not find the + // transaction here, it could be that the lookup is by bare txid + // and can be found through UTXO lookup. } if (fAllowSlow) { // use coin database to locate block that contains transaction, and scan it @@ -70,7 +71,7 @@ bool GetTransaction(const uint256& hash, CTransaction& txOut, uint256& hashBlock CBlock block; if (ReadBlockFromDisk(block, pindexSlow)) { BOOST_FOREACH (const CTransaction& tx, block.vtx) { - if (tx.GetHash() == hash) { + if (tx.GetHash() == hash || tx.GetBareTxid() == hash) { txOut = tx; hashBlock = pindexSlow->GetBlockHash(); return true; @@ -101,4 +102,4 @@ bool CollateralIsExpectedAmount(const COutPoint &outpoint, int64_t expectedAmoun return true; } assert(false); -} \ No newline at end of file +} diff --git a/divi/src/WalletTransactionRecord.cpp b/divi/src/WalletTransactionRecord.cpp index 14fee3e18..a94324812 100644 --- a/divi/src/WalletTransactionRecord.cpp +++ b/divi/src/WalletTransactionRecord.cpp @@ -30,11 +30,22 @@ WalletTransactionRecord::WalletTransactionRecord( const CWalletTx* WalletTransactionRecord::GetWalletTx(const uint256& hash) const { AssertLockHeld(cs_walletTxRecord); - std::map::const_iterator it = mapWallet.find(hash); - if (it == mapWallet.end()) - return NULL; - return &(it->second); + + { + const auto mit = mapWallet.find(hash); + if (mit != mapWallet.end()) + return &mit->second; + } + + { + const auto mit = mapBareTxid.find(hash); + if (mit != mapBareTxid.end()) + return mit->second; + } + + return nullptr; } + std::vector WalletTransactionRecord::GetWalletTransactionReferences() const { AssertLockHeld(cs_walletTxRecord); @@ -50,7 +61,12 @@ std::vector WalletTransactionRecord::GetWalletTransactionRefer std::pair::iterator, bool> WalletTransactionRecord::AddTransaction(const CWalletTx& newlyAddedTransaction) { AssertLockHeld(cs_walletTxRecord); - return mapWallet.insert(std::make_pair(newlyAddedTransaction.GetHash(), newlyAddedTransaction)); + + auto res = mapWallet.emplace(newlyAddedTransaction.GetHash(), newlyAddedTransaction); + if (res.second) + mapBareTxid.emplace(newlyAddedTransaction.GetBareTxid(), &res.first->second); + + return res; }; void WalletTransactionRecord::UpdateMetadata( diff --git a/divi/src/WalletTransactionRecord.h b/divi/src/WalletTransactionRecord.h index 53e5c7944..30a2d793e 100644 --- a/divi/src/WalletTransactionRecord.h +++ b/divi/src/WalletTransactionRecord.h @@ -10,15 +10,23 @@ struct WalletTransactionRecord CCriticalSection& cs_walletTxRecord; const std::string walletFilename_; const bool databaseWritesAreDisallowed_; + + /** Map from the bare txid of transactions in the wallet to the matching + * transactions themselves. */ + std::map mapBareTxid; + public: std::map mapWallet; WalletTransactionRecord(CCriticalSection& requiredWalletLock,const std::string& walletFilename); WalletTransactionRecord(CCriticalSection& requiredWalletLock); const CWalletTx* GetWalletTx(const uint256& hash) const; + + /** Tries to look up a transaction in the wallet, either by hash (txid) or + * the bare txid that is used after segwit-light to identify outputs. */ std::vector GetWalletTransactionReferences() const; std::pair::iterator, bool> AddTransaction(const CWalletTx& newlyAddedTransaction); void UpdateMetadata(const uint256& hashOfTransactionToUpdate, const CWalletTx& updatedTransaction, bool updateDiskAndTimestamp,bool writeToWalletDb=false); }; -#endif// WALLET_TRANSACTION_RECORD_H \ No newline at end of file +#endif// WALLET_TRANSACTION_RECORD_H diff --git a/divi/src/rpcmasternode.cpp b/divi/src/rpcmasternode.cpp index 759c35f64..ee8ac4fb1 100644 --- a/divi/src/rpcmasternode.cpp +++ b/divi/src/rpcmasternode.cpp @@ -78,7 +78,7 @@ Value allocatefunds(const Array& params, bool fHelp) " (numeric, required) amount of divi funded will also be accepted for partially funding master nodes and other purposes.\n" "\nResult:\n" - "\"vin\" (string) funding transaction id necessary for next step.\n"); + "\"vin\" (string) funding transaction id or bare txid necessary for next step.\n"); if (params[0].get_str() != "masternode") { @@ -114,7 +114,7 @@ Value fundmasternode(const Array& params, bool fHelp) "1. alias (string, required) helpful identifier to recognize this allocation later.\n" "2. amount (diamond, platinum, gold, silver, copper) tier of masternode. \n" " (numeric, required) amount of divi funded will also be accepted for partially funding master nodes and other purposes.\n" - "3. TxID (string, required) funding transaction id .\n" + "3. TxID (string, required) funding transaction id or bare txid.\n" "4. masternode (string, required) ip address of masternode.\n" "(use an empty string for the pay wallet if the same as the funding wallet and you wish to assign a different voting wallet).\n" @@ -229,7 +229,7 @@ Value setupmasternode(const Array& params, bool fHelp) "\nArguments:\n" "1. alias (string, required) Helpful identifier to recognize this masternode later. \n" - "2. txHash (string, required) Funding transaction. \n" + "2. txHash (string, required) Funding transaction hash or bare txid. \n" "3. outputIndex (string, required) Output index transaction. \n" "4. collateralPubkey (string, required) collateral pubkey. \n" "5. ip_address (string, required) Local ip address of this node\n" From 3dfff6f4ebe1fa4fbcd6e97b41895f0bb44e291d Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Tue, 30 Mar 2021 16:28:17 +0200 Subject: [PATCH 14/16] Add bare txid to transaction index. When the transaction index is enabled (-txindex), this change also keeps an index from bare txid to the disk position, so that lookups with the index work on both normal txids and bare txids. With this, the previous change made to GetTransaction will be improved for enabled -txindex, and it will also allow us to look up any transaction by bare txid, even if the fallbacks in GetTransaction fail. --- divi/src/BlockTransactionChecker.cpp | 4 ++-- divi/src/BlockTransactionChecker.h | 2 +- divi/src/IndexDatabaseUpdates.h | 15 +++++++++++- divi/src/TransactionDiskAccessor.cpp | 8 +++---- divi/src/txdb.cpp | 36 ++++++++++++++++++++++------ divi/src/txdb.h | 3 ++- 6 files changed, 52 insertions(+), 16 deletions(-) diff --git a/divi/src/BlockTransactionChecker.cpp b/divi/src/BlockTransactionChecker.cpp index 98fa9793e..f332d90db 100644 --- a/divi/src/BlockTransactionChecker.cpp +++ b/divi/src/BlockTransactionChecker.cpp @@ -25,14 +25,14 @@ TransactionLocationRecorder::TransactionLocationRecorder( void TransactionLocationRecorder::RecordTxLocationData( const CTransaction& tx, - std::vector >& txLocationData) + std::vector& txLocationData) { if(!txLocationDataSizeHasBeenPreallocated_) { txLocationData.reserve(numberOfTransactions_); txLocationDataSizeHasBeenPreallocated_ = true; } - txLocationData.emplace_back(tx.GetHash(), nextBlockTxOnDiskLocation_); + txLocationData.emplace_back(tx.GetHash(), tx.GetBareTxid(), nextBlockTxOnDiskLocation_); nextBlockTxOnDiskLocation_.nTxOffset += ::GetSerializeSize(tx, SER_DISK, CLIENT_VERSION); } diff --git a/divi/src/BlockTransactionChecker.h b/divi/src/BlockTransactionChecker.h index 43335788d..b01272c00 100644 --- a/divi/src/BlockTransactionChecker.h +++ b/divi/src/BlockTransactionChecker.h @@ -28,7 +28,7 @@ class TransactionLocationRecorder const CBlock& block); void RecordTxLocationData( const CTransaction& tx, - std::vector >& txLocationData); + std::vector& txLocationData); }; class BlockTransactionChecker diff --git a/divi/src/IndexDatabaseUpdates.h b/divi/src/IndexDatabaseUpdates.h index a724b1a7e..9abc9857e 100644 --- a/divi/src/IndexDatabaseUpdates.h +++ b/divi/src/IndexDatabaseUpdates.h @@ -9,12 +9,25 @@ class CTransaction; class TransactionUtxoHasher; +/** One entry in the tx index, which locates transactions on disk by their txid + * or bare txid (both keys are possible). */ +struct TxIndexEntry +{ + uint256 txid; + uint256 bareTxid; + CDiskTxPos diskPos; + + explicit TxIndexEntry(const uint256& t, const uint256& b, const CDiskTxPos& p) + : txid(t), bareTxid(b), diskPos(p) + {} +}; + struct IndexDatabaseUpdates { std::vector > addressIndex; std::vector > addressUnspentIndex; std::vector > spentIndex; - std::vector > txLocationData; + std::vector txLocationData; IndexDatabaseUpdates(); }; diff --git a/divi/src/TransactionDiskAccessor.cpp b/divi/src/TransactionDiskAccessor.cpp index e4608c875..20fd725a4 100644 --- a/divi/src/TransactionDiskAccessor.cpp +++ b/divi/src/TransactionDiskAccessor.cpp @@ -44,14 +44,14 @@ bool GetTransaction(const uint256& hash, CTransaction& txOut, uint256& hashBlock return error("%s : Deserialize or I/O error - %s", __func__, e.what()); } hashBlock = header.GetHash(); - if (txOut.GetHash() != hash) + if (txOut.GetHash() != hash && txOut.GetBareTxid() != hash) return error("%s : txid mismatch", __func__); return true; } - // The index only keys by txid. So even if we did not find the - // transaction here, it could be that the lookup is by bare txid - // and can be found through UTXO lookup. + // Transaction not found in the index (which works both with + // txid and bare txid), nothing more can be done. + return false; } if (fAllowSlow) { // use coin database to locate block that contains transaction, and scan it diff --git a/divi/src/txdb.cpp b/divi/src/txdb.cpp index 4eadb3987..0e59d168d 100644 --- a/divi/src/txdb.cpp +++ b/divi/src/txdb.cpp @@ -16,14 +16,22 @@ #include #include #include +#include #include using namespace std; -static const char DB_ADDRESSINDEX = 'a'; -static const char DB_SPENTINDEX = 'p'; -static const char DB_ADDRESSUNSPENTINDEX = 'u'; +namespace +{ + +constexpr char DB_ADDRESSINDEX = 'a'; +constexpr char DB_SPENTINDEX = 'p'; +constexpr char DB_ADDRESSUNSPENTINDEX = 'u'; +constexpr char DB_TXINDEX = 't'; +constexpr char DB_BARETXIDINDEX = 'T'; + +} // anonymous namespace extern BlockMap mapBlockIndex; extern std::set > setStakeSeen; @@ -185,14 +193,28 @@ bool CCoinsViewDB::GetStats(CCoinsStats& stats) const bool CBlockTreeDB::ReadTxIndex(const uint256& txid, CDiskTxPos& pos) { - return Read(std::make_pair('t', txid), pos); + /* This method looks up by txid or bare txid. Both are tried, and if + one succeeds, that must be the one. Note that it is not possible for + the same value to be both a bare txid and a txid (except where both + are the same for a single transaction), as that would otherwise be + a hash collision. */ + + if (Read(std::make_pair(DB_TXINDEX, txid), pos)) + return true; + if (Read(std::make_pair(DB_BARETXIDINDEX, txid), pos)) + return true; + + return false; } -bool CBlockTreeDB::WriteTxIndex(const std::vector >& vect) +bool CBlockTreeDB::WriteTxIndex(const std::vector& vect) { CLevelDBBatch batch; - for (std::vector >::const_iterator it = vect.begin(); it != vect.end(); it++) - batch.Write(std::make_pair('t', it->first), it->second); + for (const auto& entry : vect) + { + batch.Write(std::make_pair(DB_TXINDEX, entry.txid), entry.diskPos); + batch.Write(std::make_pair(DB_BARETXIDINDEX, entry.bareTxid), entry.diskPos); + } return WriteBatch(batch); } diff --git a/divi/src/txdb.h b/divi/src/txdb.h index 7e964efb6..df6305b48 100644 --- a/divi/src/txdb.h +++ b/divi/src/txdb.h @@ -28,6 +28,7 @@ struct CAddressUnspentValue; struct CDiskTxPos; struct CCoinsStats; struct CSpentIndexValue; +struct TxIndexEntry; //! -dbcache default (MiB) static const int64_t nDefaultDbCache = 100; @@ -71,7 +72,7 @@ class CBlockTreeDB : public CLevelDBWrapper bool WriteReindexing(bool fReindex); bool ReadReindexing(bool& fReindex); bool ReadTxIndex(const uint256& txid, CDiskTxPos& pos); - bool WriteTxIndex(const std::vector >& list); + bool WriteTxIndex(const std::vector& list); bool WriteAddressIndex(const std::vector > &vect); bool EraseAddressIndex(const std::vector > &vect); bool ReadAddressIndex(uint160 addressHash, int type, From 67f178b5da9b73328065b0c5f2ef498482427a63 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Wed, 31 Mar 2021 16:18:51 +0200 Subject: [PATCH 15/16] Update txindex.py test for bare txid lookups. This extends the txindex.py test, to check lookups also by bare txid where this is possible (mempool and txindex). Lookups by UTXO set only work on whatever hash is used for the UTXO before/after segwit light. --- divi/qa/rpc-tests/txindex.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/divi/qa/rpc-tests/txindex.py b/divi/qa/rpc-tests/txindex.py index 0ec75be09..728561cab 100755 --- a/divi/qa/rpc-tests/txindex.py +++ b/divi/qa/rpc-tests/txindex.py @@ -22,12 +22,12 @@ def setup_network (self): self.is_network_split = False self.sync_all () - def expect_found (self, node, txid): - data = node.getrawtransaction (txid, 1) - assert_equal (data["txid"], txid) + def expect_found (self, node, key, value): + data = node.getrawtransaction (value, 1) + assert_equal (data[key], value) - def expect_not_found (self, node, txid): - assert_raises (JSONRPCException, node.getrawtransaction, txid) + def expect_not_found (self, node, value): + assert_raises (JSONRPCException, node.getrawtransaction, value) def run_test (self): self.nodes[0].setgenerate (True, 30) @@ -46,13 +46,17 @@ def run_test (self): tx = self.nodes[0].createrawtransaction (inputs, {addr: value - 1}) signed = self.nodes[0].signrawtransaction (tx) assert_equal (signed["complete"], True) - txid = self.nodes[0].sendrawtransaction (signed["hex"]) + self.nodes[0].sendrawtransaction (signed["hex"]) + data = self.nodes[0].decoderawtransaction (signed["hex"]) + txid = data["txid"] + bareTxid = data["baretxid"] sync_mempools (self.nodes) print ("Lookup through mempool...") for n in self.nodes: assert_equal (n.getrawmempool (), [txid]) - self.expect_found (n, txid) + self.expect_found (n, "txid", txid) + self.expect_found (n, "baretxid", bareTxid) print ("Lookup through UTXO set...") self.nodes[0].setgenerate (True, 1) @@ -60,7 +64,7 @@ def run_test (self): for n in self.nodes: assert_equal (n.getrawmempool (), []) assert n.gettxout (txid, 0) is not None - self.expect_found (n, txid) + self.expect_found (n, "txid", txid) print ("Spending test output...") tx = self.nodes[0].createrawtransaction ([{"txid": txid, "vout": 0}], {addr: value - 2}) @@ -74,8 +78,10 @@ def run_test (self): for n in self.nodes: assert_equal (n.getrawmempool (), []) assert_equal (n.gettxout (txid, 0), None) - self.expect_found (self.nodes[0], txid) + self.expect_found (self.nodes[0], "txid", txid) + self.expect_found (self.nodes[0], "baretxid", bareTxid) self.expect_not_found (self.nodes[1], txid) + self.expect_not_found (self.nodes[1], bareTxid) if __name__ == '__main__': From 1dd1e4ef1a7d130f9d12de117f0c3fa524e8c622 Mon Sep 17 00:00:00 2001 From: Daniel Kraft Date: Thu, 10 Dec 2020 14:57:58 +0100 Subject: [PATCH 16/16] Regtest for segwit light. This adds the new regtest segwit_light.py, which verifies that sending of transactions and staking still works after the fork, and that the new rules work as expected. It also explicitly checks that transaction malleability is not an issue anymore for chains of pre-signed transactions. --- divi/qa/rpc-tests/around_segwit_light.py | 7 +- divi/qa/rpc-tests/segwit_light.py | 116 +++++++++++++++++++++++ divi/qa/rpc-tests/test_runner.py | 1 + 3 files changed, 121 insertions(+), 3 deletions(-) create mode 100755 divi/qa/rpc-tests/segwit_light.py diff --git a/divi/qa/rpc-tests/around_segwit_light.py b/divi/qa/rpc-tests/around_segwit_light.py index 8eb831c74..f42f902ad 100755 --- a/divi/qa/rpc-tests/around_segwit_light.py +++ b/divi/qa/rpc-tests/around_segwit_light.py @@ -11,7 +11,7 @@ from authproxy import JSONRPCException from util import * -ACTIVATION_TIME = 2_100_000_000 +from segwit_light import ACTIVATION_TIME # Offset around the activation time where no restrictions are in place. NO_RESTRICTIONS = 24 * 3_600 @@ -123,12 +123,13 @@ def run_test (self): set_node_times (self.nodes, ACTIVATION_TIME - MEMPOOL_RESTRICTED) self.expect_mempool_restricted ("txid") set_node_times (self.nodes, ACTIVATION_TIME + MEMPOOL_RESTRICTED) - self.expect_mempool_restricted ("txid") + self.node.setgenerate (True, 1) + self.expect_mempool_restricted ("baretxid") # Finally, we should run into mempool-only or no restrictions # at all if we go further into the future, away from the fork. set_node_times (self.nodes, ACTIVATION_TIME + WALLET_RESTRICTED) - self.expect_wallet_restricted ("txid") + self.expect_wallet_restricted ("baretxid") set_node_times (self.nodes, ACTIVATION_TIME + NO_RESTRICTIONS) self.expect_unrestricted () diff --git a/divi/qa/rpc-tests/segwit_light.py b/divi/qa/rpc-tests/segwit_light.py new file mode 100755 index 000000000..9bcb50910 --- /dev/null +++ b/divi/qa/rpc-tests/segwit_light.py @@ -0,0 +1,116 @@ +#!/usr/bin/env python3 +# Copyright (c) 2020 The DIVI developers +# Distributed under the MIT/X11 software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +# Tests the policy changes in wallet and mempool around the +# segwit-light activation time that prevent spending of +# unconfirmed outputs. + +from test_framework import BitcoinTestFramework +from util import * + +from PowToPosTransition import createPoSStacks, generatePoSBlocks + +ACTIVATION_TIME = 2_000_000_000 + + +class SegwitLightTest (BitcoinTestFramework): + + def setup_network (self, split=False): + args = ["-debug", "-spendzeroconfchange"] + self.nodes = start_nodes (2, self.options.tmpdir, extra_args=[args]*2) + connect_nodes (self.nodes[0], 1) + self.is_network_split = False + + def run_test (self): + # Activate the fork and PoS. We go beyond the fork to ensure + # the mempool/wallet limitations are lifted already. + set_node_times (self.nodes, ACTIVATION_TIME + 3_600 * 24 * 7) + reconnect_all (self.nodes) + self.nodes[0].setgenerate (True, 1) + sync_blocks (self.nodes) + createPoSStacks (self.nodes[:1], self.nodes) + generatePoSBlocks (self.nodes, 0, 100) + blk = self.nodes[1].getblockheader (self.nodes[1].getbestblockhash ()) + assert_greater_than (blk["time"], ACTIVATION_TIME) + + # Send some normal transactions from the wallet (but in a chain). + self.nodes[0].sendtoaddress (self.nodes[1].getnewaddress (), 1_000) + generatePoSBlocks (self.nodes, 0, 1) + assert_equal (self.nodes[1].getbalance (), 1_000) + addr = self.nodes[0].getnewaddress () + id1 = self.nodes[1].sendtoaddress (addr, 900) + id2 = self.nodes[1].sendtoaddress (addr, 90) + id3 = self.nodes[1].sendtoaddress (addr, 9) + assert_equal (set (self.nodes[1].getrawmempool ()), set ([id1, id2, id3])) + sync_mempools (self.nodes) + generatePoSBlocks (self.nodes, 0, 1) + assert_equal (self.nodes[1].getrawmempool (), []) + assert_greater_than (1, self.nodes[1].getbalance ()) + + # Build a transaction on top of an unconfirmed one, that we will malleate. + # The prepared transaction should still be valid. For malleating, we use + # funds on a 1-of-2 multisig address, and then change which wallet + # is signing. + keys = [ + n.validateaddress (n.getnewaddress ())["pubkey"] + for n in self.nodes + ] + multisig = self.nodes[0].addmultisigaddress (1, keys) + assert_equal (self.nodes[1].addmultisigaddress (1, keys), multisig) + txid0 = self.nodes[0].sendtoaddress (multisig, 1_000) + data0 = self.nodes[0].getrawtransaction (txid0, 1) + btxid = data0["baretxid"] + outputIndex = None + for i in range (len (data0["vout"])): + if data0["vout"][i]["scriptPubKey"]["addresses"] == [multisig]: + assert outputIndex is None + outputIndex = i + assert outputIndex is not None + generatePoSBlocks (self.nodes, 0, 1) + out = self.nodes[0].gettxout (btxid, outputIndex) + assert_equal (out["confirmations"], 1) + assert_equal (out["value"], 1_000) + assert_equal (out["scriptPubKey"]["addresses"], [multisig]) + + inputs = [{"txid": btxid, "vout": outputIndex}] + tempAddr = self.nodes[0].getnewaddress ("temp") + outputs = {tempAddr: 999} + unsigned1 = self.nodes[0].createrawtransaction (inputs, outputs) + signed1 = self.nodes[0].signrawtransaction (unsigned1) + assert_equal (signed1["complete"], True) + signed1 = signed1["hex"] + data1 = self.nodes[0].decoderawtransaction (signed1) + + prevtx = [ + { + "txid": data1["baretxid"], + "vout": 0, + "scriptPubKey": self.nodes[0].validateaddress (tempAddr)["scriptPubKey"], + } + ] + inputs = [{"txid": data1["baretxid"], "vout": 0}] + finalAddr = self.nodes[1].getnewaddress ("final") + outputs = {finalAddr: 998} + unsigned2 = self.nodes[0].createrawtransaction (inputs, outputs) + signed2 = self.nodes[0].signrawtransaction (unsigned2, prevtx) + assert_equal (signed2["complete"], True) + signed2 = signed2["hex"] + data2 = self.nodes[0].decoderawtransaction (signed2) + + signed1p = self.nodes[1].signrawtransaction (unsigned1) + assert_equal (signed1p["complete"], True) + signed1p = signed1p["hex"] + data1p = self.nodes[0].decoderawtransaction (signed1p) + assert_equal (data1["baretxid"], data1p["baretxid"]) + assert data1["txid"] != data1p["txid"] + + self.nodes[0].sendrawtransaction (signed1p) + self.nodes[0].sendrawtransaction (signed2) + generatePoSBlocks (self.nodes, 0, 1) + sync_blocks (self.nodes) + assert_equal (self.nodes[1].getbalance ("final"), 998) + +if __name__ == '__main__': + SegwitLightTest ().main () diff --git a/divi/qa/rpc-tests/test_runner.py b/divi/qa/rpc-tests/test_runner.py index f5033bb0c..3e9e2498a 100755 --- a/divi/qa/rpc-tests/test_runner.py +++ b/divi/qa/rpc-tests/test_runner.py @@ -100,6 +100,7 @@ 'rpcbind_test.py', 'remotestart.py', 'remotestart.py --outdated_ping', + 'segwit_light.py', 'smartfees.py', 'sync.py', 'txindex.py',