Incorporate review comments

ouroboros-consensus-test/src/Test/ThreadNet/Network.hs

@@ -109,22 +109,18 @@ import           Ouroboros.Consensus.Util.Time
 import qualified Ouroboros.Consensus.Storage.ChainDB as ChainDB
 import qualified Ouroboros.Consensus.Storage.ChainDB.API.Types.InvalidBlockPunishment as InvalidBlockPunishment
 import           Ouroboros.Consensus.Storage.ChainDB.Impl (ChainDbArgs (..))
-import           Ouroboros.Consensus.Storage.FS.API (SomeHasFS (..))
 import qualified Ouroboros.Consensus.Storage.ImmutableDB as ImmutableDB
-import qualified Ouroboros.Consensus.Storage.ImmutableDB.Impl.Index as Index
-import qualified Ouroboros.Consensus.Storage.LedgerDB.DiskPolicy as LgrDB
-import qualified Ouroboros.Consensus.Storage.VolatileDB as VolatileDB
 import           Ouroboros.Consensus.Util.Enclose (pattern FallingEdge)
 
 import           Test.ThreadNet.TxGen
 import           Test.ThreadNet.Util.NodeJoinPlan
 import           Test.ThreadNet.Util.NodeRestarts
 import           Test.ThreadNet.Util.NodeTopology
 import           Test.ThreadNet.Util.Seed
+import           Test.Util.ChainDB
 
 import           Test.Util.FS.Sim.MockFS (MockFS)
 import qualified Test.Util.FS.Sim.MockFS as Mock
-import           Test.Util.FS.Sim.STM (simHasFS)
 import           Test.Util.HardFork.Future (Future)
 import qualified Test.Util.HardFork.Future as HFF
 import           Test.Util.HardFork.OracularClock (OracularClock (..))
@@ -704,33 +700,18 @@ runThreadNetwork systemTime ThreadNetworkArgs
       clock registry
       cfg initLedger
       invalidTracer addTracer selTracer updatesTracer pipeliningTracer
-      nodeDBs _coreNodeId = ChainDbArgs {
-          -- HasFS instances
-          cdbHasFSImmutableDB       = SomeHasFS $ simHasFS (nodeDBsImm nodeDBs)
-        , cdbHasFSVolatileDB        = SomeHasFS $ simHasFS (nodeDBsVol nodeDBs)
-        , cdbHasFSLgrDB             = SomeHasFS $ simHasFS (nodeDBsLgr nodeDBs)
-          -- Policy
-        , cdbImmutableDbValidation  = ImmutableDB.ValidateAllChunks
-        , cdbVolatileDbValidation   = VolatileDB.ValidateAll
-        , cdbMaxBlocksPerFile       = VolatileDB.mkBlocksPerFile 4
-        , cdbDiskPolicy             = LgrDB.defaultDiskPolicy (configSecurityParam cfg) LgrDB.DefaultSnapshotInterval
-          -- Integration
-        , cdbTopLevelConfig         = cfg
-        , cdbChunkInfo              = ImmutableDB.simpleChunkInfo epochSize0
-        , cdbCheckIntegrity         = nodeCheckIntegrity (configStorage cfg)
-        , cdbGenesis                = return initLedger
-        , cdbCheckInFuture          = InFuture.reference (configLedger cfg) InFuture.defaultClockSkew
+      nodeDBs _coreNodeId =
+        let args = mkTestChainDbArgs cfg
+                                     (ImmutableDB.simpleChunkInfo epochSize0)
+                                     initLedger
+                                     registry
+                                     nodeDBs
+                                     (instrumentationTracer <> nullDebugTracer)
+                                     nullDebugTracer
+        in args { cdbCheckIntegrity = nodeCheckIntegrity (configStorage cfg)
+                , cdbCheckInFuture = InFuture.reference (configLedger cfg) InFuture.defaultClockSkew
                                       (OracularClock.finiteSystemTime clock)
-        , cdbImmutableDbCacheConfig = Index.CacheConfig 2 60
-        -- Misc
-        , cdbTracer                 = instrumentationTracer <> nullDebugTracer
-        , cdbTraceLedger            = nullDebugTracer
-        , cdbRegistry               = registry
-          -- TODO vary these
-        , cdbGcDelay                = 0
-        , cdbGcInterval             = 1
-        , cdbBlocksToAddSize        = 2
-        }
+                }
       where
         prj af = case AF.headBlockNo af of
             At bno -> bno
@@ -1416,16 +1397,6 @@ data NodeEvents blk ev = NodeEvents
     -- ^ Pipelining events tracking the tentative header
   }
 
--- | A vector with an element for each database of a node
---
--- The @db@ type parameter is instantiated by this module at types for mock
--- filesystems; either the 'MockFS' type or reference cells thereof.
-data NodeDBs db = NodeDBs
-  { nodeDBsImm :: db
-  , nodeDBsVol :: db
-  , nodeDBsLgr :: db
-  }
-
 newNodeInfo ::
   forall blk m.
      IOLike m

ouroboros-consensus-test/src/Test/Util/ChainDB.hs

@@ -1,5 +1,9 @@
 {-# LANGUAGE FlexibleContexts #-}
-module Test.Util.ChainDB where
+module Test.Util.ChainDB (
+    NodeDBs (..)
+  , emptyNodeDBs
+  , mkTestChainDbArgs
+  ) where
 
 
 import           Data.Functor.Identity (Identity)
@@ -52,7 +56,7 @@ emptyNodeDBs = NodeDBs
   <*> uncheckedNewTVarM Mock.empty
 
 
--- | 1. No integrity checking of blocks
+-- | Creates a default set of of arguments for ChainDB tests.
 mkTestChainDbArgs :: (MonadThrow m, MonadSTM m, ConsensusProtocol (BlockProtocol blk))
                   => TopLevelConfig blk
                   -> ImmutableDB.ChunkInfo
@@ -71,109 +75,34 @@ mkTestChainDbArgs cfg chunkInfo initLedger registry nodeDBs tracer traceLedger =
     cdbHasFSImmutableDB = SomeHasFS $ simHasFS (nodeDBsImm nodeDBs)
   , cdbHasFSVolatileDB = SomeHasFS $ simHasFS (nodeDBsVol nodeDBs)
   , cdbHasFSLgrDB = SomeHasFS $ simHasFS (nodeDBsLgr nodeDBs)
-
   , cdbImmutableDbValidation = ImmutableDB.ValidateAllChunks
-  -- ^ Which chunks of the ImmutableDB to validate on opening: all chunks, or only the most recent
-  -- chunk?
-
-  , cdbVolatileDbValidation = VolatileDB.ValidateAll -- VolatileDB
-  -- ^ Should the parser for the VolatileDB fail when it encounters a corrupt/invalid block.
-
-  , cdbMaxBlocksPerFile = VolatileDB.mkBlocksPerFile 4 -- VolatileDB
-  -- ^ The maximum number of blocks to store per file in the volatile DB.
-
+  , cdbVolatileDbValidation = VolatileDB.ValidateAll
+  , cdbMaxBlocksPerFile = VolatileDB.mkBlocksPerFile 4
   , cdbDiskPolicy = LedgerDB.defaultDiskPolicy (configSecurityParam cfg)
                     LedgerDB.DefaultSnapshotInterval
-  -- ^ When to snapshot the LedgerDB, and how many snapshots to keep around.
+  -- ^ Keep 2 ledger snapshots, and take a new snapshot at least every 2 * k seconds, where k is the
+  -- security parameter.
   , cdbTopLevelConfig = cfg
   , cdbChunkInfo = chunkInfo
-
-  -- The size of the chunk in the ImmutableDB, since now the chunk size is uniform. Conceptually,
-  -- all on-disk layout functions depend on this.
-
-  -- The top-level config might have information on the epoch size that could be used to construct a
-  -- sensible default for @cdbChunkInfo@ but that depends on the specific instantiations for @blk@
-  -- of the type families in @TopLevelConfig@.
-
-  -- StateMachine: passed via argument, eventually as an parameter of the QuickCheck property
-  -- ThreadNet: ImmutableDB.simpleChunkInfo epochSize0
-  -- Client: ImmutableDB.simpleChunkInfo epochSize
-  -- ImmutableDB.simpleChunkInfo epochSize
-
-  -- where epochSize      = eraEpochSize $ topLevelConfigLedger topLevelConfig
-
-  , cdbCheckIntegrity = const True -- ImmutableDB, VolatileDB.
-  -- ^ Integrity checking of blocks for both the ImmutableDB and the VolatileDB.
-  , cdbGenesis = return initLedger -- LgrDB
-    -- StateMachine: return initLedger
-    -- ThreadNet: return initLedger (passed in as an argument to mkArgs)
-    -- BlockFetch: pure testInitExtLedger --> ChainDbArgs f m TestBlock
+  , cdbCheckIntegrity = const True
+  -- ^ Getting a verified block component does not do any integrity checking, both for the
+  -- ImmutableDB, as the VolatileDB. This is done in @extractBlockComponent@ in the iterator for the
+  -- ImmutableDB, and in @getBlockComponent@ for the VolatileDB.
+  , cdbGenesis = return initLedger
   , cdbCheckInFuture = CheckInFuture $ \vf -> pure (VF.validatedFragment vf, [])
-    -- ChainDB <--- TODO
-    -- StateMachine: InFuture.miracle (readTVar varCurSlot) maxClockSkew
-    -- ThreadNet: InFuture.reference (configLedger cfg)
-    --                               InFuture.defaultClockSkew
-    --                               (OracularClock.finiteSystemTime clock)
-    -- BlockFetch: CheckInFuture $ \vf -> pure (validatedFragment vf, [])
-    -- FollowerPromptness: CheckInFuture $ \vf -> pure (validatedFragment vf, [])
-
-    -- It seems that this allows for splitting the past from future: the miracle function assumes an
-    -- oracle that can always give us the 'correct' slot. The FollowerPromptness tests and the
-    -- BlockFetch tests assume that everything is in the past, so no behaviour based on the future.
-
+  -- ^ Blocks are never in the future.
   , cdbImmutableDbCacheConfig = ImmutableDB.CacheConfig 2 60 -- ImmutableDB
-
-  -- Cache configuration for the immutable DB. How many past chunks to cache (keep in memory,
-  -- excluding the current), and when to expire past chunks.
-
-  -- StateMachine: Index.CacheConfig 2 60
-  -- ThreadNet: Index.CacheConfig 2 60
-  -- BlockFetch.Client: ImmutableDB.CacheConfig 2 60
-  -- FollowerPromptness: ImmutableDB.CacheConfig 2 60
-
-  -- Misc
-  , cdbTracer = tracer -- ChainDB, ImmutableDB, VolatileDB, LgrDB
-  , cdbTraceLedger = traceLedger -- LgrDB
-  -- StateMachine
-  --   cdbTracer                 = tracer
-  --   cdbTraceLedger            = nullTracer
-  -- ThreadNet more complicated, since we use tracers for testing
-  -- BlockFetch
-  --   cdbTraceLedger            = nullTracer
-  --   where tracer is passed to the constructor
-  --   cdbTracer = Tracer \case
-  --      ChainDBImpl.TraceAddBlockEvent ev ->
-  --         traceWith tracer $ "ChainDB: " <> show ev
-  --      _ -> pure ()
-  -- FollowerPromptness uses BlockFetch tracers
-
-  , cdbRegistry = registry -- ChainDB, ImmutableDB
-
-  , cdbGcDelay = 1 -- ChainDB, size of the bounded queue
-  , cdbGcInterval = 1 -- ChainDB
-  , cdbBlocksToAddSize = 1 -- ChainDB
-  -- FollowerPromptness:
-  --    cdbBlocksToAddSize        = 1
-  --    cdbGcDelay                = 1
-  --    cdbGcInterval             = 1
-  -- BlockFetch
-  --    cdbBlocksToAddSize        = 1
-        -- not relevant for this test
-  --    cdbGcDelay                = 1
-  --    cdbGcInterval             = 1
-  -- StateMachine
-  --  , cdbBlocksToAddSize        = 2
-      -- We don't run the background threads, so these are not used
-  --  , cdbGcDelay                = 1
-  --  , cdbGcInterval             = 1
+  -- ^ Cache at most 2 chunks and expire each chunk after 60 seconds of being unused.
+  , cdbTracer = tracer
+  , cdbTraceLedger = traceLedger
+  , cdbRegistry = registry
+  , cdbGcDelay = 1
+  , cdbGcInterval = 1
+  , cdbBlocksToAddSize = 1
   }
 
 
--- background threads:
---   addBlockThread -- moves blocks synchronously from the queue to the "ChainDB"
---   gcThread -- volatile to immutable (?)
---   copyAndSnapshotThread --
---
---
-    -- epochSize = eraEpochSize $ topLevelConfigLedger cfg
-    -- Only when LedgerConfig blk ~ EraParams, which is not likely to hold for all cases.
+-- There are a number of fields that deal with validation of blocks:
+-- cdbCheckIntegrity
+-- cdbVolatileDbValidation
+-- cdbImmutableDbValidation

ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/ChainDB/StateMachine.hs

@@ -1485,8 +1485,8 @@ prop_sequential maxClockSkew (SmallChunkInfo chunkInfo) = withMaxSuccess 100000
       varCurSlot         <- uncheckedNewTVarM 0
       varNextId          <- uncheckedNewTVarM 0
       nodeDBs             <- emptyNodeDBs
-      let args = mkArgs testCfg maxClockSkew chunkInfo testInitExtLedger tracer
-            threadRegistry varCurSlot nodeDBs
+      let args = mkArgs testCfg chunkInfo testInitExtLedger threadRegistry nodeDBs tracer
+            maxClockSkew varCurSlot
 
       (hist, model, res, trace) <- bracket
         (open args >>= newMVar)
@@ -1611,15 +1611,15 @@ traceEventName = \case
 
 mkArgs :: IOLike m
        => TopLevelConfig Blk
-       -> MaxClockSkew
        -> ImmutableDB.ChunkInfo
        -> ExtLedgerState Blk
-       -> Tracer m (TraceEvent Blk)
        -> ResourceRegistry m
-       -> StrictTVar m SlotNo
        -> NodeDBs (StrictTVar m MockFS)
+       -> Tracer m (TraceEvent Blk)
+       -> MaxClockSkew
+       -> StrictTVar m SlotNo
        -> ChainDbArgs Identity m Blk
-mkArgs cfg (MaxClockSkew maxClockSkew) chunkInfo initLedger tracer registry varCurSlot nodeDBs =
+mkArgs cfg chunkInfo initLedger registry nodeDBs tracer (MaxClockSkew maxClockSkew) varCurSlot =
   let args = mkTestChainDbArgs cfg chunkInfo initLedger registry nodeDBs tracer nullTracer
   in args { cdbCheckInFuture = InFuture.miracle (readTVar varCurSlot) maxClockSkew
           , cdbCheckIntegrity = testBlockIsValid

ouroboros-consensus/src/Ouroboros/Consensus/Storage/ChainDB/Impl/Args.hs

@@ -49,14 +49,20 @@ data ChainDbArgs f m blk = ChainDbArgs {
 
       -- Policy
     , cdbImmutableDbValidation  :: ImmutableDB.ValidationPolicy
+    -- ^ Which chunks of the ImmutableDB to validate on opening: all chunks, or
+    -- only the most recent chunk?
     , cdbVolatileDbValidation   :: VolatileDB.BlockValidationPolicy
+    -- ^ Should the parser for the VolatileDB fail when it encounters a
+    -- corrupt/invalid block?
     , cdbMaxBlocksPerFile       :: VolatileDB.BlocksPerFile
     , cdbDiskPolicy             :: LgrDB.DiskPolicy
 
       -- Integration
     , cdbTopLevelConfig         :: HKD f (TopLevelConfig blk)
     , cdbChunkInfo              :: HKD f ChunkInfo
     , cdbCheckIntegrity         :: HKD f (blk -> Bool)
+    -- ^ Predicate to check for integrity of @GetVerifiedBlock@ components when
+    -- extracting them from both the VolatileDB and the ImmutableDB.
     , cdbGenesis                :: HKD f (m (ExtLedgerState blk))
     , cdbCheckInFuture          :: HKD f (CheckInFuture m blk)
     , cdbImmutableDbCacheConfig :: ImmutableDB.CacheConfig