Improved error coverage and added default behaviour for null merkle tree

src/rpc/blockchain.cpp

@@ -6,6 +6,7 @@
 // file COPYING or https://www.opensource.org/licenses/mit-license.php.
 
 #include "budget/budgetmanager.h"
+#include "chainparams.h"
 #include "checkpoints.h"
 #include "clientversion.h"
 #include "consensus/upgrades.h"
@@ -1212,7 +1213,7 @@ UniValue invalidateblock(const JSONRPCRequest& request)
     if (request.fHelp || request.params.size() != 1)
         throw std::runtime_error(
             "invalidateblock \"blockhash\"\n"
-            "\nPermanently marks a block as invalid, as if it violated a consensus rule. Note: Your oldest sapling note must not be more than 43200 blocks behind the chain tip\n"
+            "\nPermanently marks a block as invalid, as if it violated a consensus rule. Note: it might take up to some minutes and after calling it's reccomended to run recover transactions. \n"
 
             "\nArguments:\n"
             "1. blockhash   (string, required) the hash of the block to mark as invalid\n"
@@ -1229,13 +1230,13 @@ UniValue invalidateblock(const JSONRPCRequest& request)
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
 
         CBlockIndex* pblockindex = mapBlockIndex[hash];
-        //For each walllet in your wallet list
+        // For each wallet in your wallet list
         std::string errString = "";
         for (auto* pwallet : vpwallets) {
             //Do we need to recreate the witnesscache or is the current one enough?
             if (pwallet->GetSaplingScriptPubKeyMan()->nWitnessCacheSize <= (chainActive.Height() - pblockindex->nHeight + 1)) {
-                if (!pwallet->GetSaplingScriptPubKeyMan()->BuildWitnessChain(pblockindex)) {
-                    throw JSONRPCError(RPC_DATABASE_ERROR, "Sapling notes are too old!");
+                if (!pwallet->GetSaplingScriptPubKeyMan()->BuildWitnessChain(pblockindex, Params().GetConsensus(), errString)) {
+                    throw JSONRPCError(RPC_DATABASE_ERROR, errString);
                 }
             }
         }

src/sapling/saplingscriptpubkeyman.cpp

@@ -6,13 +6,15 @@
 #include "sapling/saplingscriptpubkeyman.h"
 
 #include "chain.h" // for CBlockIndex
+#include "consensus/params.h"
 #include "primitives/block.h"
 #include "sapling/incrementalmerkletree.h"
 #include "uint256.h"
 #include "validation.h" // for ReadBlockFromDisk()
 #include "wallet/wallet.h"
 #include <algorithm>
 #include <map>
+#include <string>
 #include <vector>
 
 void SaplingScriptPubKeyMan::AddToSaplingSpends(const uint256& nullifier, const uint256& wtxid)
@@ -189,7 +191,7 @@ void WitnessNoteIfMine(SaplingNoteData* nd,
     assert(nWitnessCacheSize >= (int64_t) nd->witnesses.size());
 }
 
-template<typename NoteDataMap>
+template <typename NoteDataMap>
 void UpdateWitnessHeights(NoteDataMap& noteDataMap, int indexHeight, int64_t nWitnessCacheSize)
 {
     for (auto& item : noteDataMap) {
@@ -205,8 +207,13 @@ void UpdateWitnessHeights(NoteDataMap& noteDataMap, int indexHeight, int64_t nWi
     }
 }
 
-bool SaplingScriptPubKeyMan::BuildWitnessChain(const CBlockIndex* pTargetBlock)
+bool SaplingScriptPubKeyMan::BuildWitnessChain(const CBlockIndex* pTargetBlock, const Consensus::Params& params, std::string& errorStr)
 {
+    // If V5 is not enforced building the witness cache is useless
+    if (!params.NetworkUpgradeActive(chainActive.Height(), Consensus::UPGRADE_V5_0)) {
+        return true;
+    }
+
     LOCK2(cs_main, wallet->cs_wallet);
     // Target is the last block we want to invalidate
     rollbackTargetHeight = pTargetBlock->nHeight;
@@ -222,13 +229,6 @@ bool SaplingScriptPubKeyMan::BuildWitnessChain(const CBlockIndex* pTargetBlock)
         minHeight = std::min(wtx.m_confirm.block_height, minHeight);
     }
 
-    // For the moment as a maximum rollback span we use 1 month or 43200 blocks
-    if ((chainActive.Height() - minHeight) > 43200) {
-        cachedWitnessMap.clear();
-        rollbackTargetHeight = -1;
-        return false;
-    }
-
     // Read blocks from the disk from chaintip to the minimum found height
     std::vector<CBlock> cblocks;
     const CBlockIndex* pIndex = GetChainTip();
@@ -241,12 +241,14 @@ bool SaplingScriptPubKeyMan::BuildWitnessChain(const CBlockIndex* pTargetBlock)
         currentHeight = pIndex->nHeight;
     }
 
-    // Load the SaplingMerkleTree for the block before the oldest note
-    SaplingMerkleTree initialSaplingTree;
-    if (!pcoinsTip->GetSaplingAnchorAt(pIndex->hashFinalSaplingRoot, initialSaplingTree)) {
+    SaplingMerkleTree initialSaplingTree = SaplingMerkleTree();
+    // Load the SaplingMerkleTree for the block before the oldest note (if the hash is zero then continue with an empty merkle tree)
+    if (!(pIndex->hashFinalSaplingRoot == UINT256_ZERO) && !pcoinsTip->GetSaplingAnchorAt(pIndex->hashFinalSaplingRoot, initialSaplingTree)) {
+        errorStr = "Cannot fetch the sapling anchor!";
         return false;
     }
     // Finally build the witness cache for each sapling note of your wallet
+    int height = minHeight;
     for (CBlock& block : cblocks) {
         // Finally build the witness cache for each sapling note
         std::vector<uint256> noteCommitments;
@@ -276,7 +278,10 @@ bool SaplingScriptPubKeyMan::BuildWitnessChain(const CBlockIndex* pTargetBlock)
             }
         }
         for (auto& it2 : cachedWitnessMap) {
-            it2.second.emplace_front(it2.second.front());
+            // Don't duplicate if the block is too old
+            if (height >= rollbackTargetHeight) {
+                it2.second.emplace_front(it2.second.front());
+            }
             for (auto& noteComm : noteCommitments) {
                 it2.second.front().append(noteComm);
             }
@@ -288,6 +293,7 @@ bool SaplingScriptPubKeyMan::BuildWitnessChain(const CBlockIndex* pTargetBlock)
                 cachedWitnessMap.emplace(*(nd->nullifier), witnesses);
             }
         }
+        height++;
     }
     return true;
 }

src/sapling/saplingscriptpubkeyman.h

@@ -160,7 +160,7 @@ class SaplingScriptPubKeyMan {
     /**
      * Build the old witness chain.
      */
-    bool BuildWitnessChain(const CBlockIndex* pTargetBlock);
+    bool BuildWitnessChain(const CBlockIndex* pTargetBlock, const Consensus::Params& params, std::string& errorStr);
 
     /**
      * pindex is the new tip being connected.