[Refactoring] Prune invalid UTXOs from the coins cache

src/Makefile.am

@@ -195,7 +195,6 @@ BITCOIN_CORE_H = \
   interfaces/wallet.h \
   invalid.h \
   invalid_outpoints.json.h \
-  invalid_serials.json.h \
   legacy/stakemodifier.h \
   kernel.h \
   key.h \

src/chainparams.cpp

@@ -161,11 +161,9 @@ class CMainParams : public CChainParams
         consensus.nTime_RejectOldSporkKey = 1614560400;     //!> March 1, 2021 01:00:00 AM GMT
 
         // height-based activations
+        consensus.height_last_invalid_UTXO = 894538;
         consensus.height_last_ZC_AccumCheckpoint = 1686240;
         consensus.height_last_ZC_WrappedSerials = 1686229;
-        consensus.height_start_InvalidUTXOsCheck = 902850;
-        consensus.height_start_ZC_InvalidSerials = 891737;
-        consensus.height_start_ZC_SerialRangeCheck = 895400;
         consensus.height_ZC_RecalcAccumulators = 908000;
 
         // validation by-pass
@@ -298,11 +296,9 @@ class CTestNetParams : public CChainParams
         consensus.nTime_RejectOldSporkKey = 1614560400;     //!> March 1, 2021 01:00:00 AM GMT
 
         // height based activations
+        consensus.height_last_invalid_UTXO = -1;
         consensus.height_last_ZC_AccumCheckpoint = -1;
         consensus.height_last_ZC_WrappedSerials = -1;
-        consensus.height_start_InvalidUTXOsCheck = 999999999;
-        consensus.height_start_ZC_InvalidSerials = 999999999;
-        consensus.height_start_ZC_SerialRangeCheck = 1;
         consensus.height_ZC_RecalcAccumulators = 999999999;
 
         // Zerocoin-related params
@@ -423,11 +419,9 @@ class CRegTestParams : public CChainParams
         consensus.nTime_RejectOldSporkKey = 0;
 
         // height based activations
+        consensus.height_last_invalid_UTXO = -1;
         consensus.height_last_ZC_AccumCheckpoint = 310;     // no checkpoints on regtest
         consensus.height_last_ZC_WrappedSerials = -1;
-        consensus.height_start_InvalidUTXOsCheck = 999999999;
-        consensus.height_start_ZC_InvalidSerials = 999999999;
-        consensus.height_start_ZC_SerialRangeCheck = 300;
         consensus.height_ZC_RecalcAccumulators = 999999999;
 
         // Zerocoin-related params

src/coins.cpp

@@ -8,6 +8,7 @@
 #include "consensus/consensus.h"
 #include "policy/fees.h"
 #include "invalid.h"
+#include "logging.h"
 #include "memusage.h"
 #include "random.h"
 
@@ -113,14 +114,20 @@ void CCoinsViewCache::AddCoin(const COutPoint& outpoint, Coin&& coin, bool possi
     cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
 }
 
-void AddCoins(CCoinsViewCache& cache, const CTransaction& tx, int nHeight, bool check)
+void AddCoins(CCoinsViewCache& cache, const CTransaction& tx, int nHeight, bool check, bool fSkipInvalid)
 {
     bool fCoinbase = tx.IsCoinBase();
     bool fCoinstake = tx.IsCoinStake();
     const uint256& txid = tx.GetHash();
     for (size_t i = 0; i < tx.vout.size(); ++i) {
-        bool overwrite = check && cache.HaveCoin(COutPoint(txid, i));
-        cache.AddCoin(COutPoint(txid, i), Coin(tx.vout[i], nHeight, fCoinbase, fCoinstake), overwrite);
+        const COutPoint out(txid, i);
+        // Don't add fraudulent/banned outputs
+        if (fSkipInvalid && invalid_out::ContainsOutPoint(out)) {
+            cache.SpendCoin(out);   // no-op if the coin is not in the cache
+            continue;
+        }
+        bool overwrite = check && cache.HaveCoin(out);
+        cache.AddCoin(out, Coin(tx.vout[i], nHeight, fCoinbase, fCoinstake), overwrite);
     }
 }
 
@@ -408,19 +415,18 @@ CAmount CCoinsViewCache::GetTotalAmount() const
     return nTotal;
 }
 
-void CCoinsViewCache::PruneInvalidEntries()
+bool CCoinsViewCache::PruneInvalidEntries()
 {
     // Prune zerocoin Mints and fraudulent/frozen outputs
-    std::unique_ptr<CCoinsViewCursor> pcursor(Cursor());
-    while (pcursor->Valid()) {
-        COutPoint key;
-        Coin coin;
-        if (pcursor->GetKey(key) && pcursor->GetValue(coin)) {
-            if (coin.out.IsZerocoinMint() || invalid_out::ContainsOutPoint(key))
-                SpendCoin(key);
+    bool loaded = invalid_out::LoadOutpoints();
+    assert(loaded);
+    for (const COutPoint& out: invalid_out::setInvalidOutPoints) {
+        if (HaveCoin(out)) {
+            LogPrintf("Pruning invalid output %s\n", out.ToString());
+            SpendCoin(out);
         }
-        pcursor->Next();
     }
+    return Flush();
 }
 
 static const size_t MAX_OUTPUTS_PER_BLOCK = MAX_BLOCK_SIZE_CURRENT /  ::GetSerializeSize(CTxOut(), SER_NETWORK, PROTOCOL_VERSION); // TODO: merge with similar definition in undo.h.

src/coins.h

@@ -409,7 +409,7 @@ class CCoinsViewCache : public CCoinsViewBacked
     /*
      * Prune zerocoin mints and frozen outputs - do it once, after initialization
      */
-    void PruneInvalidEntries();
+    bool PruneInvalidEntries();
 
 
 private:
@@ -448,7 +448,8 @@ class CCoinsViewCache : public CCoinsViewBacked
 // PIVX: When check is false, this assumes that overwrites are never possible due to BIP34 always in effect
 // When check is true, the underlying view may be queried to determine whether an addition is
 // an overwrite.
-void AddCoins(CCoinsViewCache& cache, const CTransaction& tx, int nHeight, bool check = false);
+// When fSkipInvalid is true, the invalid_out list is checked before adding the coin.
+void AddCoins(CCoinsViewCache& cache, const CTransaction& tx, int nHeight, bool check = false, bool fSkipInvalid = false);
 
 //! Utility function to find any unspent output with a given txid.
 const Coin& AccessByTxid(const CCoinsViewCache& cache, const uint256& txid);

src/consensus/params.h

@@ -114,11 +114,9 @@ struct Params {
     int64_t nTime_RejectOldSporkKey;
 
     // height-based activations
+    int height_last_invalid_UTXO;
     int height_last_ZC_AccumCheckpoint;
     int height_last_ZC_WrappedSerials;
-    int height_start_InvalidUTXOsCheck;
-    int height_start_ZC_InvalidSerials;
-    int height_start_ZC_SerialRangeCheck;
     int height_ZC_RecalcAccumulators;
 
     // validation by-pass

src/consensus/tx_verify.cpp

@@ -55,7 +55,7 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in
     return nSigOps;
 }
 
-bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, bool fRejectBadUTXO, CValidationState& state, bool fFakeSerialAttack, bool fColdStakingActive, bool fSaplingActive)
+bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationState& state, bool fFakeSerialAttack, bool fColdStakingActive, bool fSaplingActive)
 {
     // Basic checks that don't depend on any context
     // Transactions containing empty `vin` must have non-empty `vShieldedSpend`.

src/consensus/tx_verify.h

@@ -17,7 +17,7 @@ class CValidationState;
 /** Transaction validation functions */
 
 /** Context-independent validity checks */
-bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, bool fRejectBadUTXO, CValidationState& state, bool fFakeSerialAttack = false, bool fColdStakingActive=false, bool fSaplingActive=false);
+bool CheckTransaction(const CTransaction& tx, bool fZerocoinActive, CValidationState& state, bool fFakeSerialAttack = false, bool fColdStakingActive=false, bool fSaplingActive=false);
 
 /**
  * Count ECDSA signature operations the old-fashioned (pre-0.6) way

src/init.cpp

@@ -1563,6 +1563,9 @@ bool AppInitMain()
     LogPrintf("* Using %.1fMiB for chain state database\n", nCoinDBCache * (1.0 / 1024 / 1024));
     LogPrintf("* Using %.1fMiB for in-memory UTXO set\n", nCoinCacheUsage * (1.0 / 1024 / 1024));
 
+    const CChainParams& chainparams = Params();
+    const Consensus::Params& consensus = chainparams.GetConsensus();
+
     bool fLoaded = false;
     while (!fLoaded && !ShutdownRequested()) {
         bool fReset = fReindex;
@@ -1609,9 +1612,6 @@ bool AppInitMain()
                     break;
                 }
 
-                const CChainParams& chainparams = Params();
-                const Consensus::Params& consensus = chainparams.GetConsensus();
-
                 // If the loaded chain has a wrong genesis, bail out immediately
                 // (we're likely using a testnet datadir, or the other way around).
                 if (!mapBlockIndex.empty() && mapBlockIndex.count(consensus.hashGenesisBlock) == 0)
@@ -1666,29 +1666,27 @@ bool AppInitMain()
                     assert(chainActive.Tip() != NULL);
                 }
 
-                // Populate list of invalid/fraudulent outpoints that are banned from the chain
-                invalid_out::LoadOutpoints();
-                invalid_out::LoadSerials();
-
-                int chainHeight;
-                bool fZerocoinActive;
-                {
+                if (Params().NetworkIDString() == CBaseChainParams::MAIN) {
+                    // Prune zerocoin invalid outs if they were improperly stored in the coins database
                     LOCK(cs_main);
-                    chainHeight = chainActive.Height();
-                    fZerocoinActive = consensus.NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_ZC);
-
-                    // Prune zerocoin mints that were improperly stored in the coins database
-                    // Do it only once, when removing money supply (key 'M') from the DB. Can be skipped in future versions.
-                    int64_t nDummySupply;
-                    if (fZerocoinActive && pblocktree->Read('M', nDummySupply)) {
-                        LogPrintf("Pruning zerocoin mints / invalid outs, at height %d\n", chainHeight);
-                        pcoinsTip->PruneInvalidEntries();
-                        if (!pcoinsTip->Flush()) {
+                    int chainHeight = chainActive.Height();
+                    bool fZerocoinActive = chainHeight > 0 && consensus.NetworkUpgradeActive(chainHeight, Consensus::UPGRADE_ZC);
+
+                    uiInterface.InitMessage(_("Loading/Pruning invalid outputs..."));
+                    if (fZerocoinActive) {
+                        if (!pcoinsTip->PruneInvalidEntries()) {
                             strLoadError = _("System error while flushing the chainstate after pruning invalid entries. Possible corrupt database.");
                             break;
                         }
                         MoneySupply.Update(pcoinsTip->GetTotalAmount(), chainHeight);
-                        pblocktree->Erase('M');
+                        // No need to keep the invalid outs in memory. Clear the map 100 blocks after the last invalid UTXO
+                        if (chainHeight > consensus.height_last_invalid_UTXO + 100) {
+                            invalid_out::setInvalidOutPoints.clear();
+                        }
+                    } else {
+                        // Populate list of invalid/fraudulent outpoints that are banned from the chain
+                        // They will not be added to coins view
+                        invalid_out::LoadOutpoints();
                     }
                 }
 
@@ -1702,7 +1700,7 @@ bool AppInitMain()
 
                     {
                         LOCK(cs_main);
-                        CBlockIndex *tip = chainActive[chainHeight];
+                        CBlockIndex *tip = chainActive.Tip();
                         RPCNotifyBlockChange(true, tip);
                         if (tip && tip->nTime > GetAdjustedTime() + 2 * 60 * 60) {
                             strLoadError = _("The block database contains a block which appears to be from the future. "

src/invalid.cpp

@@ -4,11 +4,13 @@
 
 #include "invalid.h"
 #include "invalid_outpoints.json.h"
-#include "invalid_serials.json.h"
+
+#include "primitives/transaction.h"
+
+#include <univalue.h>
 
 namespace invalid_out
 {
-    std::set<CBigNum> setInvalidSerials;
     std::set<COutPoint> setInvalidOutPoints;
 
     UniValue read_json(const std::string& jsondata)
@@ -52,39 +54,9 @@ namespace invalid_out
         return true;
     }
 
-    bool LoadSerials()
-    {
-        UniValue v = read_json(LoadInvalidSerials());
-
-        if (v.empty())
-            return false;
-
-        for (unsigned int idx = 0; idx < v.size(); idx++) {
-            const UniValue &val = v[idx];
-            const UniValue &o = val.get_obj();
-
-            const UniValue &vSerial = find_value(o, "s");
-            if (!vSerial.isStr())
-                return false;
-
-            CBigNum bnSerial = 0;
-            bnSerial.SetHex(vSerial.get_str());
-            if (bnSerial == 0)
-                return false;
-            setInvalidSerials.insert(bnSerial);
-        }
-
-        return true;
-    }
-
     bool ContainsOutPoint(const COutPoint& out)
     {
         return static_cast<bool>(setInvalidOutPoints.count(out));
     }
-
-    bool ContainsSerial(const CBigNum& bnSerial)
-    {
-        return static_cast<bool>(setInvalidSerials.count(bnSerial));
-    }
 }
 

src/invalid.h

@@ -5,21 +5,20 @@
 #ifndef PIVX_INVALID_H
 #define PIVX_INVALID_H
 
-#endif //PIVX_INVALID_H
+#include <set>
+#include <string>
 
-#include <libzerocoin/bignum.h>
-#include <univalue/include/univalue.h>
-#include <primitives/transaction.h>
+class COutPoint;
+class UniValue;
 
 namespace invalid_out
 {
-    extern std::set<CBigNum> setInvalidSerials;
     extern std::set<COutPoint> setInvalidOutPoints;
 
     UniValue read_json(const std::string& jsondata);
 
     bool ContainsOutPoint(const COutPoint& out);
-    bool ContainsSerial(const CBigNum& bnSerial);
     bool LoadOutpoints();
-    bool LoadSerials();
-}
+}
+
+#endif //PIVX_INVALID_H