Merge pull request #181 from Concordium/fix-dead-block-access

Fix dead block access

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased changes
 
+## concordium-node 1.1.3
+
+- Fix a number of bugs that led to node crashes due to failed block lookup in some situations.
 - Support custom path for genesis data via `CONCORDIUM_NODE_CONSENSUS_GENESIS_DATA_FILE`.
 
 ## concordium-node 1.1.2

concordium-consensus/src/Concordium/Skov/Update.hs

@@ -159,21 +159,27 @@ processFinalization newFinBlock finRec@FinalizationRecord{..} = do
         let pruneHeight = fromIntegral (bpHeight newFinBlock - lastFinHeight)
         -- First, prune the trunk: the section of the branches beyond the
         -- last finalized block up to and including the new finalized block.
-        -- We proceed backwards from the new finalized block, marking it and
-        -- its ancestors as finalized, while other blocks at the same height
-        -- are marked dead.
+        -- We proceed backwards from the new finalized block, collecting blocks
+        -- to mark as dead. When we exhaust the branches we then mark blocks as finalized
+        -- by increasing height.
+        -- Instead of marking blocks dead immediately we accumulate them
+        -- and a return a list. The reason for doing this is that we never
+        -- have to look up a parent block that is already marked dead.
         let
-            pruneTrunk :: BlockPointerType m -> Branches m -> m ()
-            pruneTrunk _ Seq.Empty = return ()
-            pruneTrunk keeper (brs Seq.:|> l) = do
-                forM_ l $ \bp -> if bp == keeper then do
-                                    markFinalized (getHash bp) finRec
-                                    logEvent Skov LLDebug $ "Block " ++ show bp ++ " marked finalized"
-                                else do
-                                    markLiveBlockDead bp
-                                    logEvent Skov LLDebug $ "Block " ++ show bp ++ " marked dead"
+            pruneTrunk :: [BlockPointerType m] -- ^List of blocks to remove.
+                       -> BlockPointerType m -- ^The block that was finalized.
+                       -> Branches m
+                       -> m [BlockPointerType m]
+                       -- ^ The return value is a list of blocks to mark dead, ordered
+                       -- by increasing height.
+            pruneTrunk toRemove _ Seq.Empty = return toRemove
+            pruneTrunk toRemove keeper (brs Seq.:|> l) = do
                 parent <- bpParent keeper
-                pruneTrunk parent brs
+                let toRemove1 = filter (/= keeper) l ++ toRemove
+                toRemove2 <- pruneTrunk toRemove1 parent brs
+                -- mark blocks as finalized now, so that blocks are marked finalized by increasing height
+                markFinalized (getHash keeper) finRec
+                logEvent Skov LLDebug $ "Block " ++ show keeper ++ " marked finalized"
                 -- Finalize the transactions of the surviving block.
                 -- (This is handled in order of finalization.)
                 finalizeTransactions (getHash keeper) (blockSlot keeper) (blockTransactions keeper)
@@ -184,8 +190,9 @@ processFinalization newFinBlock finRec@FinalizationRecord{..} = do
                         bcHeight = bpHeight keeper,
                         ..}
                 flushBlockSummaries ctx ati =<< getSpecialOutcomes =<< blockState keeper
+                return toRemove2
 
-        pruneTrunk newFinBlock (Seq.take pruneHeight oldBranches)
+        toRemoveFromTrunk <- pruneTrunk [] newFinBlock (Seq.take pruneHeight oldBranches)
         -- Archive the states of blocks up to but not including the new finalized block
         let doArchive b = case compare (bpHeight b) lastFinHeight of
                 LT -> return ()
@@ -196,21 +203,29 @@ processFinalization newFinBlock finRec@FinalizationRecord{..} = do
         doArchive =<< bpParent newFinBlock
         -- Prune the branches: mark dead any block that doesn't descend from
         -- the newly-finalized block.
+        -- Instead of marking blocks dead immediately we accumulate them
+        -- and a return a list. The reason for doing this is that we never
+        -- have to look up a parent block that is already marked dead.
         let
-            pruneBranches _ Seq.Empty = return Seq.empty
-            pruneBranches parents (brs Seq.:<| rest) = do
-                survivors <- foldrM (\bp l -> do
+            pruneBranches :: [BlockPointerType m] -- ^Accumulator of blocks to mark dead.
+                          -> [BlockPointerType m] -- ^Parents that remain alive.
+                          -> Branches m -- ^Branches to prune
+                          -> m (Branches m, [BlockPointerType m])
+                          -- ^The return value is a pair of new branches and the
+                          -- list of blocks to mark dead. The blocks are ordered
+                          -- by decreasing height.
+            pruneBranches toRemove _ Seq.Empty = return (Seq.empty, toRemove)
+            pruneBranches toRemove parents (brs Seq.:<| rest) = do
+                (survivors, removals) <- foldrM (\bp ~(keep, remove) -> do
                     parent <- bpParent bp
                     if parent `elem` parents then
-                        return (bp:l)
-                    else do
-                        markLiveBlockDead bp
-                        logEvent Skov LLDebug $ "Block " ++ show (bpHash bp) ++ " marked dead"
-                        return l)
-                    [] brs
-                rest' <- pruneBranches survivors rest
-                return (survivors Seq.<| rest')
-        unTrimmedBranches <- pruneBranches [newFinBlock] (Seq.drop pruneHeight oldBranches)
+                        return (bp:keep, remove)
+                    else
+                        return (keep, bp:remove))
+                    ([], toRemove) brs
+                (rest', toRemove') <- pruneBranches removals survivors rest
+                return (survivors Seq.<| rest', toRemove')
+        (unTrimmedBranches, toRemoveFromBranches) <- pruneBranches [] [newFinBlock] (Seq.drop pruneHeight oldBranches)
         -- This removes empty lists at the end of branches which can result in finalizing on a
         -- block not in the current best local branch
         let
@@ -221,6 +236,10 @@ processFinalization newFinBlock finRec@FinalizationRecord{..} = do
                     _ -> return prunedbrs
         newBranches <- trimBranches unTrimmedBranches
         putBranches newBranches
+        -- mark dead blocks by decreasing height
+        forM_ (toRemoveFromBranches ++ reverse toRemoveFromTrunk) $ \bp -> do
+          markLiveBlockDead bp
+          logEvent Skov LLDebug $ "Block " ++ show (bpHash bp) ++ " marked dead"
         -- purge pending blocks with slot numbers predating the last finalized slot
         purgePending
         onFinalize finRec newFinBlock

concordium-node/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "concordium_node"
-version = "1.1.2" # must be kept in sync with 'is_compatible_version' in 'src/configuration.rs'
+version = "1.1.3" # must be kept in sync with 'is_compatible_version' in 'src/configuration.rs'
 description = "Concordium Node"
 authors = ["Concordium <[email protected]>"]
 exclude = [".gitignore", ".gitlab-ci.yml", "test/**/*","**/**/.gitignore","**/**/.gitlab-ci.yml"]

service/windows/installer/Node.wxs

@@ -2,9 +2,9 @@
 <!-- When updating the product version, the Product Id should be refreshed, otherwise it will not be
     possible to upgrade an existing installation.
 -->
-<?define VersionNumber="1.1.2" ?>
+<?define VersionNumber="1.1.3" ?>
 <Wix xmlns="http://schemas.microsoft.com/wix/2006/wi" xmlns:Util="http://schemas.microsoft.com/wix/UtilExtension">
-    <Product Name="Concordium Node" Manufacturer="Concordium Software" Id="b06fb385-0f3d-4322-800d-d619ac9ff9f9" UpgradeCode="297295b4-c716-4d33-8170-0f6136663bfd" Language="1033" Codepage="1252" Version="$(var.VersionNumber)">
+    <Product Name="Concordium Node" Manufacturer="Concordium Software" Id="8c92955b-383a-4f86-ba06-b39aa117493f" UpgradeCode="297295b4-c716-4d33-8170-0f6136663bfd" Language="1033" Codepage="1252" Version="$(var.VersionNumber)">
         <Package Id="*" Keywords="Concordium Installer" Description="Concordium Node $(var.VersionNumber) Installer" Manufacturer="Concordium Software" InstallerVersion="500" Languages="1033" Compressed="yes" InstallScope="perMachine" />
         <MajorUpgrade Schedule="afterInstallValidate" DowngradeErrorMessage="The currently-installed version of [ProductName] is newer than the version you are trying to install. The installation cannot continue. If you wish to install this version, please remove the newer version first." />
         <Media Id="1" Cabinet="Node.cab" EmbedCab="yes" />
@@ -320,4 +320,4 @@
         <WixVariable Id="WixUIBannerBmp" Value="!(bindpath.res)WixUiBanner.png" />
         <WixVariable Id="WixUILicenseRtf" Value="!(bindpath.res)license.rtf" />
     </Product>
-</Wix>
+</Wix>