add sanity checks for weird (i.e. likely-broken) SnapshotPolicyArgs configurations

Author: fraser-iohk

ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node.hs

@@ -525,6 +525,10 @@ runWith RunNodeArgs{..} encAddrNtN decAddrNtN LowLevelRunNodeArgs{..} =
 
           forM_ (sanityCheckConfig cfg) $ \issue ->
             traceWith (consensusSanityCheckTracer rnTraceConsensus) issue
+          let snapshotPolicyArgs =
+                lgrSnapshotPolicyArgs $ ChainDB.cdbLgrDbArgs llrnChainDbArgsDefaults
+          forM_ (sanityCheckSnapshotPolicyArgs snapshotPolicyArgs) $ \issue ->
+            traceWith (consensusSanityCheckTracer rnTraceConsensus) issue
 
           (chainDB, finalArgs) <-
             openChainDB

ouroboros-consensus.cabal

@@ -810,6 +810,7 @@ test-suite storage-test
     Test.Ouroboros.Storage.ImmutableDB.StateMachine
     Test.Ouroboros.Storage.LedgerDB
     Test.Ouroboros.Storage.LedgerDB.Serialisation
+    Test.Ouroboros.Storage.LedgerDB.SnapshotPolicySanityCheck
     Test.Ouroboros.Storage.LedgerDB.Snapshots
     Test.Ouroboros.Storage.LedgerDB.StateMachine
     Test.Ouroboros.Storage.LedgerDB.StateMachine.TestBlock

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Block/SupportsSanityCheck.hs

@@ -21,16 +21,52 @@ import Control.Exception
 import Data.List.NonEmpty (NonEmpty (..))
 import qualified Data.List.NonEmpty as NonEmpty
 import Data.Maybe (catMaybes)
+import Data.Time.Clock (DiffTime)
+import Data.Word (Word64)
 import Ouroboros.Consensus.Config (TopLevelConfig)
 import Ouroboros.Consensus.Config.SecurityParam
 
--- | An issue found in the 'TopLevelConfig' for a block. See 'displayException'
+-- | An issue found in the consensus configuration. See 'displayException'
 --   for human-readable descriptions of each of these cases, especially when
 --   presenting these to users.
 data SanityCheckIssue
   = -- | Configuration contains multiple security parameters. This may cause
     --   strange behaviour around era boundaries.
     InconsistentSecurityParam (NonEmpty SecurityParam)
+  | -- | The configured 'minimumDelay' in 'SnapshotDelayRange' is greater than
+    --   'maximumDelay'. The random snapshot delay will be sampled from an
+    --   inverted range, which is almost certainly a misconfiguration.
+    SnapshotDelayRangeInverted
+      -- | The configured minimumDelay (the larger value)
+      !DiffTime
+      -- | The configured maximumDelay (the smaller value)
+      !DiffTime
+  | -- | The configured 'minimumDelay' in 'SnapshotDelayRange' is negative.
+    --   A negative delay has no meaningful interpretation.
+    SnapshotDelayRangeNegativeMinimum
+      -- | The negative minimumDelay
+      !DiffTime
+  | -- | The configured 'sfaRateLimit' is non-positive, which disables snapshot
+    --   rate limiting entirely. Without a rate limit, snapshots may be taken
+    --   very frequently during bulk sync, causing excessive disk I/O.
+    SnapshotRateLimitDisabled
+  | -- | The configured 'sfaRateLimit' exceeds 24 hours. At steady state, the
+    --   node may go more than a day between snapshots, significantly increasing
+    --   replay time after an unclean restart.
+    SnapshotRateLimitSuspiciouslyLarge
+      -- | The configured rate limit
+      !DiffTime
+  | -- | The configured number of on-disk snapshots to keep is zero. Snapshots
+    --   will be written to disk and then immediately deleted, leaving nothing
+    --   for crash recovery. The node will have to replay from genesis on every
+    --   unclean restart.
+    SnapshotNumZero
+  | -- | The configured snapshot interval does not divide 432000 (the Cardano
+    --   mainnet epoch length in slots). Snapshots will not land on epoch
+    --   boundaries, breaking Mithril compatibility.
+    SnapshotIntervalNotDivisorOfEpoch
+      -- | The configured interval in slots
+      !Word64
   deriving (Show, Eq)
 
 instance Exception SanityCheckIssue where
@@ -42,6 +78,61 @@ instance Exception SanityCheckIssue where
         , "eras of a HardForkBlock: "
         , show (NonEmpty.toList ks)
         ]
+    SnapshotDelayRangeInverted mn mx ->
+      mconcat
+        [ "SnapshotDelayRangeInverted: "
+        , "The configured snapshot delay range has minimumDelay ("
+        , show mn
+        , ") greater than maximumDelay ("
+        , show mx
+        , "). The random snapshot delay will be sampled from an inverted range. "
+        , "Please ensure minimumDelay <= maximumDelay in sfaDelaySnapshotRange."
+        ]
+    SnapshotDelayRangeNegativeMinimum mn ->
+      mconcat
+        [ "SnapshotDelayRangeNegativeMinimum: "
+        , "The configured snapshot delay range has a negative minimumDelay: "
+        , show mn
+        , ". A negative delay has no meaningful interpretation. "
+        , "Please set minimumDelay to a non-negative value in sfaDelaySnapshotRange."
+        ]
+    SnapshotRateLimitDisabled ->
+      mconcat
+        [ "SnapshotRateLimitDisabled: "
+        , "The configured sfaRateLimit is non-positive, which disables snapshot "
+        , "rate limiting entirely. Without a rate limit, snapshots may be taken "
+        , "very frequently during bulk sync, causing excessive disk I/O. "
+        , "The default rate limit is 10 minutes."
+        ]
+    SnapshotRateLimitSuspiciouslyLarge rl ->
+      mconcat
+        [ "SnapshotRateLimitSuspiciouslyLarge: "
+        , "The configured sfaRateLimit ("
+        , show rl
+        , ") exceeds 24 hours. At steady state, the node may go more than a day "
+        , "between snapshots, significantly increasing replay time after an "
+        , "unclean restart. The default rate limit is 10 minutes."
+        ]
+    SnapshotNumZero ->
+      mconcat
+        [ "SnapshotNumZero: "
+        , "The configured number of on-disk snapshots to keep (spaNum) is 0. "
+        , "Snapshots will be written to disk and immediately deleted, leaving "
+        , "nothing for crash recovery. The node will have to replay the chain "
+        , "from genesis on every unclean restart. "
+        , "Consider setting spaNum to at least 2 (the default)."
+        ]
+    SnapshotIntervalNotDivisorOfEpoch interval ->
+      mconcat
+        [ "SnapshotIntervalNotDivisorOfEpoch: "
+        , "The configured sfaInterval ("
+        , show interval
+        , " slots) does not evenly divide the Cardano mainnet epoch length "
+        , "(432000 slots). Snapshots will not consistently land on epoch "
+        , "boundaries, which breaks Mithril compatibility. "
+        , "Consider using an interval that divides 432000 evenly, "
+        , "such as 4320 (the default, = 2k for k=2160)."
+        ]
 
 -- | 'BlockSupportsSanityCheck' provides evidence that a block can be sanity
 --   checked for common issues on node startup. 'sanityCheckConfig', which runs

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/LedgerDB/Snapshots.hs

@@ -82,6 +82,8 @@ module Ouroboros.Consensus.Storage.LedgerDB.Snapshots
   , SnapshotFrequency (..)
   , SnapshotFrequencyArgs (..)
   , defaultSnapshotPolicy
+  , mithrilEpochSize
+  , sanityCheckSnapshotPolicyArgs
   , pattern DoDiskSnapshotChecksum
   , pattern NoDoDiskSnapshotChecksum
 
@@ -706,6 +708,59 @@ defaultSnapshotPolicy (SecurityParam k) args =
   -- Most relevant during syncing.
   defRateLimit = secondsToDiffTime $ 10 * 60
 
+-- | The Cardano mainnet epoch length in slots, used as the reference for the
+-- Mithril snapshot compatibility check in 'sanityCheckSnapshotPolicyArgs'.
+-- Mithril requires a ledger snapshot at each epoch boundary; for snapshots to
+-- land on every epoch boundary, 'sfaInterval' must divide this value evenly.
+mithrilEpochSize :: Word64
+mithrilEpochSize = 432000
+
+-- | Check a 'SnapshotPolicyArgs' for suspicious configurations and return a
+-- (possibly empty) list of 'SanityCheckIssue's describing any problems found.
+--
+-- Only 'Override' values are checked — 'UseDefault' values are known-good and
+-- are never flagged. Checks that are specific to 'SnapshotFrequency' are
+-- skipped entirely when 'spaFrequency' is 'DisableSnapshots'.
+sanityCheckSnapshotPolicyArgs :: SnapshotPolicyArgs -> [SanityCheckIssue]
+sanityCheckSnapshotPolicyArgs SnapshotPolicyArgs{spaFrequency, spaNum} =
+  catMaybes $
+    checkNumZero spaNum
+      : case spaFrequency of
+        DisableSnapshots -> []
+        SnapshotFrequency sfa -> checkFrequencyArgs sfa
+ where
+  checkNumZero (Override 0) = Just SnapshotNumZero
+  checkNumZero _ = Nothing
+
+  checkFrequencyArgs SnapshotFrequencyArgs{sfaDelaySnapshotRange, sfaRateLimit, sfaInterval} =
+    [ checkDelayRange sfaDelaySnapshotRange
+    , checkRateLimitDisabled sfaRateLimit
+    , checkRateLimitLarge sfaRateLimit
+    , checkMithrilDivisibility sfaInterval
+    ]
+
+  checkDelayRange UseDefault = Nothing
+  checkDelayRange (Override (SnapshotDelayRange mn mx))
+    | mn < 0 = Just (SnapshotDelayRangeNegativeMinimum mn)
+    | mn > mx = Just (SnapshotDelayRangeInverted mn mx)
+    | otherwise = Nothing
+
+  checkRateLimitDisabled UseDefault = Nothing
+  checkRateLimitDisabled (Override rl)
+    | rl <= 0 = Just SnapshotRateLimitDisabled
+    | otherwise = Nothing
+
+  checkRateLimitLarge UseDefault = Nothing
+  checkRateLimitLarge (Override rl)
+    | rl > 86400 = Just (SnapshotRateLimitSuspiciouslyLarge rl)
+    | otherwise = Nothing
+
+  checkMithrilDivisibility UseDefault = Nothing
+  checkMithrilDivisibility (Override interval)
+    | mithrilEpochSize `mod` unNonZero interval /= 0 =
+        Just (SnapshotIntervalNotDivisorOfEpoch (unNonZero interval))
+    | otherwise = Nothing
+
 {-------------------------------------------------------------------------------
   Tracing snapshot events
 -------------------------------------------------------------------------------}

ouroboros-consensus/test/storage-test/Test/Ouroboros/Storage/LedgerDB.hs

@@ -6,6 +6,7 @@
 module Test.Ouroboros.Storage.LedgerDB (tests) where
 
 import qualified Test.Ouroboros.Storage.LedgerDB.Serialisation as Serialisation
+import qualified Test.Ouroboros.Storage.LedgerDB.SnapshotPolicySanityCheck as SnapshotPolicySanityCheck
 import qualified Test.Ouroboros.Storage.LedgerDB.Snapshots as Snapshots
 import qualified Test.Ouroboros.Storage.LedgerDB.StateMachine as StateMachine
 import qualified Test.Ouroboros.Storage.LedgerDB.V1.BackingStore as BackingStore
@@ -24,6 +25,7 @@ tests =
     , -- Independent of the LedgerDB implementation
       Serialisation.tests
     , Snapshots.tests
+    , SnapshotPolicySanityCheck.tests
     , -- Tests both V1 and V2
       StateMachine.tests
     ]