Apply suggestions by @dnadales

ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/Genesis/Setup/GenChains.hs

@@ -122,7 +122,10 @@ genChains genNumForks = do
     gtDelay = delta,
     gtSlotLength,
     gtChainSyncTimeouts = chainSyncTimeouts gtSlotLength asc,
-    gtLoPBucketParams = LoPBucketParams { lbpCapacity = 10_000, lbpRate = 1_000 }, -- REVIEW: Do we want to generate those randomly?
+    gtLoPBucketParams = LoPBucketParams { lbpCapacity = 10_000, lbpRate = 1_000 },
+    -- ^ REVIEW: Do we want to generate those randomly? For now, the chosen
+    -- values carry no special meaning. Someone needs to think about what values
+    -- would make for interesting tests.
     gtBlockTree = foldl' (flip BT.addBranch') (BT.mkTrunk goodChain) $ zipWith (genAdversarialFragment goodBlocks) [1..] alternativeChainSchemas,
     gtSchedule = ()
     }

ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/Genesis/Tests/LoE.hs

@@ -27,7 +27,6 @@ import           Test.Consensus.PointSchedule.SinglePeer (scheduleBlockPoint,
 import           Test.Tasty
 import           Test.Tasty.QuickCheck
 import           Test.Util.Orphans.IOLike ()
-import           Test.Util.TersePrinting (tersePoint)
 import           Test.Util.TestEnv (adjustQuickCheckTests)
 
 tests :: TestTree
@@ -62,11 +61,15 @@ prop_adversaryHitsTimeouts timeoutsEnabled =
       )
       shrinkPeerSchedules
       ( \GenesisTest {gtBlockTree} StateView {svSelectedChain, svChainSyncExceptions} ->
-        tabulate "binary log buckets of the length of the honest chain" [show (round @_ @Int (2 ** (realToFrac @_ @Double (round @_ @Int (logBase 2 (realToFrac @_ @Double (1 + AF.length (btTrunk gtBlockTree))))))))] $
-        counterexample ("Selection is not the honest tip: " ++ tersePoint (AF.headPoint (btTrunk gtBlockTree)) ++ " / " ++ tersePoint (AF.castPoint (AF.headPoint svSelectedChain))) $
-          let treeTipPoint = AF.headPoint $ btTrunk gtBlockTree
+          let -- The tip of the blocktree trunk.
+              treeTipPoint = AF.headPoint $ btTrunk gtBlockTree
+              -- The tip of the selection.
               selectedTipPoint = AF.castPoint $ AF.headPoint svSelectedChain
+              -- If timeouts are enabled, then the adversary should have been
+              -- killed and the selection should be the whole trunk.
               selectedCorrect = timeoutsEnabled == (treeTipPoint == selectedTipPoint)
+              -- If timeouts are enabled, then we expect exactly one
+              -- `ExceededTimeLimit` exception in the adversary's ChainSync.
               exceptionsCorrect = case svChainSyncExceptions of
                 [] -> not timeoutsEnabled
                 [ChainSyncException (PeerId _) exn] ->

ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/Genesis/Tests/LoP.hs

@@ -202,9 +202,15 @@ prop_delayAttack lopEnabled =
       )
       shrinkPeerSchedules
       ( \GenesisTest {gtBlockTree} StateView {svSelectedChain, svChainSyncExceptions} ->
-          let treeTipPoint = AF.headPoint $ btTrunk gtBlockTree
+          let -- The tip of the blocktree trunk.
+              treeTipPoint = AF.headPoint $ btTrunk gtBlockTree
+              -- The tip of the selection.
               selectedTipPoint = AF.castPoint $ AF.headPoint svSelectedChain
+              -- If LoP is enabled, then the adversary should have been killed
+              -- and the selection should be the whole trunk.
               selectedCorrect = lopEnabled == (treeTipPoint == selectedTipPoint)
+              -- If LoP is enabled, then we expect exactly one `EmptyBucket`
+              -- exception in the adversary's ChainSync.
               exceptionsCorrect = case svChainSyncExceptions of
                 [] -> not lopEnabled
                 [ChainSyncException (PeerId _) exn] ->

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Util/LeakyBucket.hs

@@ -82,15 +82,15 @@ data State cfg = State
 type Bucket m = StrictTVar m (State (Config m))
 
 -- | Whether filling the bucket overflew.
-newtype Overflew = Overflew Bool
+data FillResult = Overflew | DidNotOverflow
 
 -- | The handlers to a bucket: contains the API to interact with a running
 -- bucket.
 data Handlers m = Handlers
   { -- | Refill the bucket by the given amount and returns whether the bucket
     -- overflew. The bucket may silently get filled to full capacity or not get
     -- filled depending on 'fillOnOverflow'.
-    fill         :: !(Rational -> m Overflew),
+    fill         :: !(Rational -> m FillResult),
     -- | Pause or resume the bucket. Pausing stops the bucket from leaking until
     -- it is resumed. It is still possible to fill it during that time. @setPaused
     -- True@ and @setPaused False@ are idempotent.
@@ -146,7 +146,11 @@ runAgainstBucket config action = do
     )
     (stopThread thread)
   where
-    startThread :: StrictTVar m (Maybe (Async m ())) -> Bucket m -> ThreadId m -> m ()
+    startThread ::
+      StrictTVar m (Maybe (Async m ())) ->
+      Bucket m ->
+      ThreadId m ->
+      m ()
     startThread thread bucket tid =
       readTVarIO thread >>= \case
         Just _ -> error "LeakyBucket: startThread called when a thread is already running"
@@ -163,9 +167,13 @@ runAgainstBucket config action = do
       newState <- snapshot bucket
       atomically $ writeTVar bucket newState {paused}
 
-    updateConfig :: StrictTVar m (Maybe (Async m ())) -> Bucket m -> ThreadId m -> (Config m -> Config m) -> m ()
+    updateConfig ::
+      StrictTVar m (Maybe (Async m ())) ->
+      Bucket m ->
+      ThreadId m ->
+      (Config m -> Config m) ->
+      m ()
     updateConfig thread bucket tid = \f -> do
-      -- FIXME: All of that should be in one STM transaction.
       State {level, time, paused, config = oldConfig} <- snapshot bucket
       let newConfig@Config {capacity = newCapacity, rate = newRate} = f oldConfig
           newLevel = min newCapacity level
@@ -177,15 +185,22 @@ runAgainstBucket config action = do
 
 -- | Initialise a bucket given a configuration. The bucket starts full at the
 -- time where one calls 'init'.
-init :: (MonadMonotonicTime m, MonadSTM m) => Config m -> m (Bucket m)
+init ::
+  (MonadMonotonicTime m, MonadSTM m) =>
+  Config m ->
+  m (Bucket m)
 init config@Config {capacity} = do
   time <- getMonotonicTime
   uncheckedNewTVarM $ State {time, level = capacity, paused = False, config}
 
 -- | Monadic action that calls 'threadDelay' until the bucket is empty, then
 -- returns @()@. It receives the 'ThreadId' argument of the action's thread,
 -- which it uses to throw exceptions at it.
-leak :: (MonadDelay m, MonadCatch m, MonadFork m, MonadAsync m) => Bucket m -> ThreadId m -> m (Maybe (Async m ()))
+leak ::
+  (MonadDelay m, MonadCatch m, MonadFork m, MonadAsync m) =>
+  Bucket m ->
+  ThreadId m ->
+  m (Maybe (Async m ()))
 leak bucket actionThreadId = do
   State {config = Config {rate}} <- snapshot bucket
   if rate <= 0
@@ -204,7 +219,10 @@ leak bucket actionThreadId = do
 
 -- | Take a snapshot of the bucket, that is compute its state at the current
 -- time.
-snapshot :: (MonadSTM m, MonadMonotonicTime m) => Bucket m -> m (State (Config m))
+snapshot ::
+  (MonadSTM m, MonadMonotonicTime m) =>
+  Bucket m ->
+  m (State (Config m))
 snapshot bucket = fst <$> snapshotFill bucket 0
 
 -- | Same as 'snapshot' but also adds the given quantity to the resulting
@@ -220,7 +238,11 @@ snapshot bucket = fst <$> snapshotFill bucket 0
 -- something else. It cannot easily be an STM transaction, though, because we
 -- need to measure the time, and @io-classes@'s STM does not allow running IO in
 -- an STM.
-snapshotFill :: (MonadSTM m, MonadMonotonicTime m) => Bucket m -> Rational -> m (State (Config m), Overflew)
+snapshotFill ::
+  (MonadSTM m, MonadMonotonicTime m) =>
+  Bucket m ->
+  Rational ->
+  m (State (Config m), FillResult)
 snapshotFill bucket toAdd = do
   newTime <- getMonotonicTime
   atomically $ do
@@ -234,7 +256,7 @@ snapshotFill bucket toAdd = do
         newLevel = if not overflew || fillOnOverflow then levelFilled else levelLeaked
         newState = State {time = newTime, level = newLevel, paused, config}
     writeTVar bucket newState
-    pure (newState, Overflew overflew)
+    pure (newState, if overflew then Overflew else DidNotOverflow)
 
 -- | Convert a 'DiffTime' to a 'Rational' number of seconds. This is similar to
 -- 'diffTimeToSeconds' but with picoseconds precision.

ouroboros-consensus/test/infra-test/Test/Ouroboros/Consensus/Util/LeakyBucket/Tests.hs

@@ -5,6 +5,10 @@
 {-# LANGUAGE RankNTypes          #-}
 {-# LANGUAGE ScopedTypeVariables #-}
 
+-- | This module contains various tests for the leaky bucket. Some, prefixed by
+-- “play”, are simple, manual tests; two concern (non-)propagation of exceptions
+-- between the bucket thread and the action's thread; the last one compares a
+-- run of the actual bucket implementation against a model.
 module Test.Ouroboros.Consensus.Util.LeakyBucket.Tests (tests) where
 
 import           Control.Monad (foldM, void)
@@ -30,10 +34,10 @@ tests = testGroup "Ouroboros.Consensus.Util.LeakyBucket" [
   testProperty "play a bit" prop_playABit,
   testProperty "play too long" prop_playTooLong,
   testProperty "play too long harmless" prop_playTooLongHarmless,
+  testProperty "play with pause" prop_playWithPause,
+  testProperty "play with pause too long" prop_playWithPauseTooLong,
   testProperty "wait almost too long" (prop_noRefill (-1)),
   testProperty "wait just too long" (prop_noRefill 1),
-  testProperty "pause for a time" prop_playWithPause,
-  testProperty "resume too quickly" prop_playWithPauseTooLong,
   testProperty "propagates exceptions" prop_propagateExceptions,
   testProperty "propagates exceptions (IO)" prop_propagateExceptionsIO,
   testProperty "catch exception" prop_catchException,