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).

Author: domob1812

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

divi/src/BlockMemoryPoolTransactionCollector.cpp

@@ -204,7 +204,7 @@ std::vector<TxPriority> 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.
@@ -335,10 +335,10 @@ std::vector<PrioritizedTransactionData> BlockMemoryPoolTransactionCollector::Pri
         nBlockSigOps += nTxSigOps;
 
         CTxUndo txundo;
-        UpdateCoinsWithTransaction(tx, view, txundo, nHeight);
+        UpdateCoinsWithTransaction(tx, view, txundo, mempool_.GetUtxoHasher(), nHeight);
 
         // 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);

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

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
+#endif// BLOCK_TRANSACTION_CHECKER_H

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

divi/src/IndexDatabaseUpdates.cpp

@@ -1,5 +1,8 @@
 #include <IndexDatabaseUpdates.h>
 
+#include <primitives/transaction.h>
+#include <UtxoCheckingAndUpdating.h>
+
 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)
 {
-}
+}

divi/src/IndexDatabaseUpdates.h

@@ -6,6 +6,9 @@
 #include <spentindex.h>
 #include <uint256.h>
 
+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
@@ -32,11 +35,13 @@ 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);
 };

divi/src/UtxoCheckingAndUpdating.cpp

@@ -10,28 +10,35 @@
 #include <undo.h>
 #include <chainparams.h>
 #include <defaultValues.h>
+#include <IndexDatabaseUpdates.h>
 
 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);
         }
     }
 
     // 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())
     {

divi/src/UtxoCheckingAndUpdating.h

@@ -3,20 +3,64 @@
 #include <vector>
 #include <scriptCheck.h>
 #include <amount.h>
+#include <uint256.h>
 
 class CTransaction;
 class CValidationState;
 class CCoinsViewCache;
 class CTxUndo;
+class TransactionLocationReference;
+
 enum class TxReversalStatus
 {
     ABORT_NO_OTHER_ERRORS,
     ABORT_WITH_OTHER_ERRORS,
     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<CScriptCheck>* pvChecks = nullptr,
     bool connectBlockDoS = false);
-#endif// UTXO_CHECKING_AND_UPDATING_H
+#endif// UTXO_CHECKING_AND_UPDATING_H

divi/src/main.cpp

@@ -1207,7 +1207,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__),
@@ -1219,7 +1219,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");
@@ -1339,14 +1339,16 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
     LogWalletBalance();
     static const CChainParams& chainParameters = Params();
 
+    const BlockUtxoHasher utxoHasher;
+
     VerifyBestBlockIsAtPreviousBlock(pindex,view);
     if (block.GetHash() == Params().HashGenesisBlock())
     {
         if(!fJustCheck)
             view.SetBestBlock(pindex->GetBlockHash());
         return true;
     }
-    if(!CheckEnforcedPoSBlocksAndBIP30(chainParameters,block,state,pindex,view))
+    if(!CheckEnforcedPoSBlocksAndBIP30(chainParameters,utxoHasher,block,state,pindex,view))
     {
         return false;
     }
@@ -1363,7 +1365,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))
     {
@@ -2636,6 +2638,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<CTxUndo> vtxundo;
                 vtxundo.reserve(block.vtx.size() - 1);
@@ -2645,7 +2648,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);
                 }