From 2230ba912f7d178ae2e634b2d3ad5208340b522e Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@z.cash>
Date: Wed, 2 Mar 2022 22:51:09 +0000
Subject: [PATCH 1/2] Ensure the view's best Orchard anchor matches the
 previous block

`ConnectBlock` was already checking that the given view's "best block"
was the previous block. However, it then assumed the view was correct on
its claimed best anchors.

For Orchard, we know that `hashFinalOrchardRoot` field of `CBlockIndex`
will only ever be set when a block is (attempted to be) connected to the
main chain, and so we can instead add assertions around its value and
ensure the view is consistent with the previous block.
---
 src/main.cpp | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/main.cpp b/src/main.cpp
index 244624bb5..8179b6b2a 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -3136,7 +3136,19 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
     assert(view.GetSaplingAnchorAt(view.GetBestAnchor(SAPLING), sapling_tree));
 
     OrchardMerkleFrontier orchard_tree;
-    assert(view.GetOrchardAnchorAt(view.GetBestAnchor(ORCHARD), orchard_tree));
+    if (pindex->pprev && chainparams.GetConsensus().NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_NU5)) {
+        // Verify that the view's current state corresponds to the previous block.
+        assert(pindex->pprev->hashFinalOrchardRoot == view.GetBestAnchor(ORCHARD));
+        // We only call ConnectBlock() on top of the active chain's tip.
+        assert(!pindex->pprev->hashFinalOrchardRoot.IsNull());
+
+        assert(view.GetOrchardAnchorAt(pindex->pprev->hashFinalOrchardRoot, orchard_tree));
+    } else {
+        if (pindex->pprev) {
+            assert(pindex->pprev->hashFinalOrchardRoot.IsNull());
+        }
+        assert(view.GetOrchardAnchorAt(OrchardMerkleFrontier::empty_root(), orchard_tree));
+    }
 
     // Grab the consensus branch ID for this block and its parent
     auto consensusBranchId = CurrentEpochBranchId(pindex->nHeight, chainparams.GetConsensus());

From ae3d2a3525687011a1627c534da8599d461bc0dc Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@z.cash>
Date: Thu, 3 Mar 2022 00:16:40 +0000
Subject: [PATCH 2/2] Add missing `view.PopAnchor(_, ORCHARD)` in
 `DisconnectBlock`

By forgetting to pop the tree, we were leaving the view in an
inconsistent state, where it believed the best Orchard anchor was for a
tree that didn't correspond to the rest of the view. This would then
propagate in subsequent block connections, and the chain history
commitments to Orchard tree roots eventually result in inconsistent
`blockcommitments` values in subsequent blocks.
---
 src/coins.cpp |  8 ++++++++
 src/main.cpp  | 11 +++++++++++
 2 files changed, 19 insertions(+)

diff --git a/src/coins.cpp b/src/coins.cpp
index 2a62e549d..4ed2907af 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -683,6 +683,14 @@ void CCoinsViewCache::PopAnchor(const uint256 &newrt, ShieldedType type) {
                 hashSaplingAnchor
             );
             break;
+        case ORCHARD:
+            AbstractPopAnchor<OrchardMerkleFrontier, CAnchorsOrchardMap, CAnchorsOrchardCacheEntry>(
+                newrt,
+                ORCHARD,
+                cacheOrchardAnchors,
+                hashOrchardAnchor
+            );
+            break;
         default:
             throw std::runtime_error("Unknown shielded type");
     }
diff --git a/src/main.cpp b/src/main.cpp
index 8179b6b2a..5e646a734 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -2909,6 +2909,17 @@ static DisconnectResult DisconnectBlock(const CBlock& block, CValidationState& s
         view.PopAnchor(SaplingMerkleTree::empty_root(), SAPLING);
     }
 
+    // Set the old best Orchard anchor back. We can get this from the
+    // `hashFinalOrchardRoot` of the last block. However, if the last
+    // block was not on or after the Orchard activation height, this
+    // will be set to `null`. For logical consistency, in this case we
+    // set the last anchor to the empty root.
+    if (chainparams.GetConsensus().NetworkUpgradeActive(pindex->pprev->nHeight, Consensus::UPGRADE_NU5)) {
+        view.PopAnchor(pindex->pprev->hashFinalOrchardRoot, ORCHARD);
+    } else {
+        view.PopAnchor(OrchardMerkleFrontier::empty_root(), ORCHARD);
+    }
+
     // This is guaranteed to be filled by LoadBlockIndex.
     assert(pindex->nCachedBranchId);
     auto consensusBranchId = pindex->nCachedBranchId.value();