Merge pull request #1330 from Concordium/fix-1329

Correctly construct account difference map for certified blocks

Author: td202

CHANGELOG.md

@@ -1,11 +1,13 @@
 # Changelog
 
 ## Unreleased changes
+
 - Replace `BufferedRef` with `HashedBufferedRef` in `PoolRewards`
   `bakerPoolRewardDetails::LFMBTree` field to cache computed hashes.
-
 - Improvements to the loading of modules. This particularly improves the performance of
   `GetModuleSource` in certain cases, and can also reduce start-up time.
+- Fix a bug that affects setting up the account map correctly for non-finalized certified blocks
+  that contain account creations (#1329).
 
 ## 8.0.3
 

concordium-consensus/src/Concordium/GlobalState/BlockState.hs

@@ -1608,6 +1608,8 @@ class (BlockStateOperations m, FixedSizeSerialization (BlockStateRef m)) => Bloc
     --
     --  Preconditions:
     --  * This function MUST only be called on a certified block.
+    --  * The block state MUST already be cached (with 'cacheBlockState'). Otherwise the update to
+    --    the difference map may not be retained.
     --  * This function MUST only be called on a block state that does not already
     --    have a difference map.
     --  * The provided list of accounts MUST correspond to the accounts created in the block,

concordium-consensus/src/Concordium/GlobalState/Persistent/Accounts.hs

@@ -173,7 +173,7 @@ mkNewChildDifferenceMap accts@Accounts{..} = do
     return accts{accountDiffMapRef = newDiffMapRef}
 
 -- | Create and set the 'DiffMap.DifferenceMap' for the provided @Accounts pv@.
---  This function constructs the difference map for the block such that the assoicated difference map
+--  This function constructs the difference map for the block such that the associated difference map
 --  and lmdb backed account map are consistent with the account table.
 --
 --  The function is highly unsafe and can cause state invariant failures if not all of the

concordium-consensus/src/Concordium/KonsensusV1/TreeState/StartUp.hs

@@ -25,7 +25,7 @@ import Concordium.Types.Updates
 import Concordium.Utils
 
 import qualified Concordium.GlobalState.AccountMap.DifferenceMap as DiffMap
-import Concordium.GlobalState.BlockState
+import Concordium.GlobalState.BlockState as BlockState
 import Concordium.GlobalState.Parameters hiding (getChainParameters)
 import qualified Concordium.GlobalState.Persistent.BlockState as PBS
 import qualified Concordium.GlobalState.Statistics as Stats
@@ -436,6 +436,11 @@ loadCertifiedBlocks = do
         _ -> Nothing
     loadCertBlock (storedBlock, qc) loadedBlocks = do
         blockPointer <- mkBlockPointer storedBlock
+        let bh = getHash @BlockHash blockPointer
+        -- Cache the block state first, as this is required for reconstructing the account
+        -- difference map.
+        cacheBlockState (bpState blockPointer)
+
         -- As only finalized accounts are stored in the account map, then
         -- we need to reconstruct the 'DiffMap.DifferenceMap' here for the certified block we're loading.
         let accountsToInsert = mapMaybe getAccountAddressFromDeployment (blockTransactions storedBlock)
@@ -456,7 +461,29 @@ loadCertifiedBlocks = do
         -- append to the accumulator with this new difference map reference
         let loadedBlocks' = HM.insert (getHash storedBlock) newDifferenceMap loadedBlocks
 
-        cacheBlockState (bpState blockPointer)
+        -- Validate that the 'accountsToInsert' are now accessible.
+        -- This should never fail, but it is worth verifying here since there are likely to be
+        -- few such accounts and it is better to catch any bug here than end up with a corrupted
+        -- account map.
+        forM_ accountsToInsert $ \addr -> do
+            BlockState.getAccount (bpState blockPointer) addr >>= \case
+                Nothing ->
+                    throwM . TreeStateInvariantViolation $
+                        "Account "
+                            ++ show addr
+                            ++ " not found after loading certified block "
+                            ++ show bh
+                Just (_, acc) -> do
+                    actualAddr <- getAccountCanonicalAddress acc
+                    unless (actualAddr == addr) $
+                        throwM . TreeStateInvariantViolation $
+                            "Account address mismatch ("
+                                ++ show actualAddr
+                                ++ " != "
+                                ++ show addr
+                                ++ ") after loading certified block "
+                                ++ show bh
+
         blockTable . liveMap . at' (getHash blockPointer) ?=! blockPointer
         addToBranches blockPointer
         forM_ (blockTransactions blockPointer) $ \tr -> do