QA: Transaction based indexing fixes

General robustness fixes
Introduce new class "CoinUndo" to work around a bug in undo behaviour that was 'corrupting' the undo state
Fix an issue with coin selection and prematurely selecting 'index based outpoints' prior to activation
Improve/correct/enhance the 'validateInsert' function to improve robustness

src/coins.h

@@ -121,6 +121,44 @@ class Coin
     }
 };
 
+class CoinUndo : public Coin
+{
+    public:
+    uint256 prevhash;
+    CoinUndo(Coin&& fromCoin, uint256 hashIn)
+    : Coin(fromCoin)
+    , prevhash(hashIn)
+    {
+    }
+
+    //! empty constructor
+    CoinUndo() : Coin() {}
+
+    template<typename Stream>
+    void Serialize(Stream &s) const
+    {
+        assert(!IsSpent());
+        uint32_t code = (nHeight<<2) + (fSegSig << 1) + fCoinBase;
+        ::Serialize(s, VARINT(code));
+        ::Serialize(s, VARINT(nTxIndex));
+        out.WriteToStream(s, (fSegSig ? CTransaction::SEGSIG_ACTIVATION_VERSION : CTransaction::SEGSIG_ACTIVATION_VERSION-1));
+        ::Serialize(s, prevhash);
+    }
+
+    template<typename Stream>
+    void Unserialize(Stream &s)
+    {
+        uint32_t code = 0;
+        ::Unserialize(s, VARINT(code));
+        nHeight = ( ((code  & 0b11111111111111111111111111111100) >> 2) );
+        fSegSig = ( (code   & 0b00000000000000000000000000000010) > 0 );
+        fCoinBase = ( (code & 0b00000000000000000000000000000001) > 0 );
+        ::Unserialize(s, VARINT(nTxIndex));
+        out.ReadFromStream(s, (fSegSig ? CTransaction::SEGSIG_ACTIVATION_VERSION : CTransaction::SEGSIG_ACTIVATION_VERSION-1));
+        ::Unserialize(s, prevhash);
+    }
+};
+
 class SaltedOutpointHasher
 {
 private:
@@ -254,6 +292,7 @@ class CCoinsViewCache : public CCoinsViewBacked
     mutable uint256 hashBlock;
     mutable CCoinsMap cacheCoins;
     mutable CCoinsRefMap cacheCoinRefs;
+    mutable uint64_t cacheMempoolRefs;
 
     /* Cached dynamic memory usage for the inner Coin objects. */
     mutable size_t cachedCoinsUsage;
@@ -297,7 +336,7 @@ class CCoinsViewCache : public CCoinsViewBacked
      * If no unspent output exists for the passed outpoint, this call
      * has no effect.
      */
-    void SpendCoin(const COutPoint &outpoint, Coin* moveto = nullptr, bool nodeletefresh = false);
+    void SpendCoin(const COutPoint &outpoint, CoinUndo* moveto = nullptr, bool nodeletefresh = false);
 
     /**
      * Push the modifications applied to this cache to its base.
@@ -363,13 +402,8 @@ class CCoinsViewCache : public CCoinsViewBacked
 private:
     CCoinsMap::iterator FetchCoin(const COutPoint &outpoint, CCoinsRefMap::iterator* pRefIterReturn=nullptr) const;
 
-#ifdef DEBUG
-    #define DEBUG_COINSCACHE_VALIDATE_INSERTS
+    //#define DEBUG_COINSCACHE_VALIDATE_INSERTS 
     void validateInsert(const COutPoint &outpoint, uint64_t block, uint64_t txIndex, uint32_t voutIndex) const;
-#else
-    // compiler can easily optimise it out in release
-    void validateInsert(const COutPoint &outpoint, uint64_t block, uint64_t txIndex, uint32_t voutIndex) const {}
-#endif
 
     /**
      * By making the copy constructor private, we prevent accidentally using it when one intends to create a cache on top of a base cache.

src/test/coins_tests.cpp

@@ -23,7 +23,7 @@
 
 #include <boost/test/unit_test.hpp>
 
-int ApplyTxInUndo(Coin&& undo, CCoinsViewCache& view, const COutPoint& out);
+int ApplyTxInUndo(CoinUndo&& undo, CCoinsViewCache& view, COutPoint out);
 void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txundo, uint32_t nHeight, uint32_t nTxIndex);
 
 //fixme: (PHASE4) Add additional tests for new segsig related coin features.
@@ -477,7 +477,7 @@ BOOST_AUTO_TEST_CASE(updatecoins_simulation_test)
             if (!tx.IsCoinBase())
             {
                 const COutPoint &out = tx.vin[0].prevout;
-                Coin coin = undo.vprevout[0];
+                CoinUndo coin = undo.vprevout[0];
                 ApplyTxInUndo(std::move(coin), *(stack.back()), out);
             }
             // Store as a candidate for reconnection

src/undo.h

@@ -28,7 +28,7 @@ class CTxUndo
 {
 public:
     // undo information for all txins
-    std::vector<Coin> vprevout;
+    std::vector<CoinUndo> vprevout;
 
     template <typename Stream>
     void Serialize(Stream& s) const {

src/validation/validation.cpp

@@ -675,24 +675,37 @@ bool AbortNode(CValidationState& state, const std::string& strMessage, const std
  * @param out The out point that corresponds to the tx input.
  * @return A DisconnectResult as an int
  */
-int ApplyTxInUndo(Coin&& undo, CCoinsViewCache& view, const COutPoint& out)
+int ApplyTxInUndo(CoinUndo&& undo, CCoinsViewCache& view, COutPoint out)
 {
     bool fClean = true;
 
-    if (view.HaveCoin(out)) fClean = false; // overwriting transaction output
+    // We always want to revert to using the hash based outpoint here to keep the coin database consistent
+    if (!out.isHash)
+    {
+        out.setHash(undo.prevhash);
+    }
+
+    if (view.HaveCoin(out))
+    {
+        fClean = false; // overwriting transaction output
+    }
 
-    if (undo.nHeight == 0) {
+    if (undo.nHeight == 0)
+    {
         // Missing undo metadata (height and coinbase). Older versions included this
         // information only in undo records for the last spend of a transactions'
         // outputs. This implies that it must be present for some other output of the same tx.
         uint256 txHash;
         if (!GetTxHash(out, txHash))
             return DISCONNECT_FAILED; // adding output for transaction without known metadata
         const Coin& alternate = AccessByTxid(view, txHash);
-        if (!alternate.IsSpent()) {
+        if (!alternate.IsSpent()) 
+        {
             undo.nHeight = alternate.nHeight;
             undo.fCoinBase = alternate.fCoinBase;
-        } else {
+        }
+        else
+        {
             return DISCONNECT_FAILED; // adding output for transaction without known metadata
         }
     }
@@ -734,16 +747,17 @@ DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex,
         for (size_t o = 0; o < tx.vout.size(); o++) {
             if (!tx.vout[o].IsUnspendable()) {
                 COutPoint out(hash, o);
-                Coin coin;
+                CoinUndo coin;
                 view.SpendCoin(out, &coin);
                 if (tx.vout[o] != coin.out) {
                     fClean = false; // transaction output mismatch
                 }
             }
         }
 
-        // restore inputs
-        if (i > 0) { // not coinbases
+        // restore inputs (not coinbases)
+        if (i > 0)
+        {
             CTxUndo &txundo = blockUndo.vtxundo[i-1];
             //fixme: (PHASE4) (HIGH) (force only 1 valid input in checkblock as well.)
             if (tx.IsPoW2WitnessCoinBase())
@@ -753,7 +767,8 @@ DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex,
                     error("DisconnectBlock(): transaction and undo data inconsistent");
                     return DISCONNECT_FAILED;
                 }
-                for (unsigned int j = tx.vin.size(); j-- > 0;) {
+                for (unsigned int j = tx.vin.size(); j-- > 0;)
+                {
                     if (!tx.vin[j].prevout.IsNull())
                     {
                         const COutPoint &out = tx.vin[j].prevout;
@@ -766,11 +781,13 @@ DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* pindex,
             }
             else
             {
-                if (txundo.vprevout.size() != tx.vin.size()) {
+                if (txundo.vprevout.size() != tx.vin.size())
+                {
                     error("DisconnectBlock(): transaction and undo data inconsistent");
                     return DISCONNECT_FAILED;
                 }
-                for (unsigned int j = tx.vin.size(); j-- > 0;) {
+                for (unsigned int j = tx.vin.size(); j-- > 0;)
+                {
                     const COutPoint &out = tx.vin[j].prevout;
                     int res = ApplyTxInUndo(std::move(txundo.vprevout[j]), view, out);
                     if (res == DISCONNECT_FAILED)
@@ -4078,7 +4095,7 @@ bool LoadExternalBlockFile(const CChainParams& chainparams, FILE* fileIn, CDiskB
                     //Note: an attacker would still have to meet/break/forge the sha ppev hash checks for an entire chain from the checkpoints
                     // This is enough to ensure that an attacker would have to go to great lengths for what would amount to a minor nuisance (having to refetch some data after detecting wrong chain)
                     // So this is not really a major weakening of security in any way and still more than sufficient.
-                    if ((mapBlockIndex.find(block.hashPrevBlock)->second->nHeight < Checkpoints::LastCheckPointHeight()))
+                    if (mapBlockIndex.find(block.hashPrevBlock)->second->nHeight < Checkpoints::LastCheckPointHeight())
                     {
                         fAssumePOWGood = true;
                     }

src/wallet/feebumper.cpp

@@ -31,7 +31,7 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *pWal
     for (auto& input : tx.vin) {
         const CWalletTx* prev = pWallet->GetWalletTx(input.prevout);
         assert(prev && input.prevout.n < prev->tx->vout.size());
-        vCoins.emplace_back(CInputCoin(prev, input.prevout.n));
+        vCoins.emplace_back(CInputCoin(prev, input.prevout.n, IsOldTransactionVersion(tx.nVersion)));
     }
     std::vector<CKeyStore*> accountsToTry;
     for (const auto& [accountUUID, account] : pWallet->mapAccounts)