Edit documentation of LoE and GDD

Also:
* Remove ill-defined LoELimit
* Remove unused updateLoEFragStall
* Rename runGdd to runGDDGovernor
* Print GDD in traces instead of GDG
* Remove the UpdateLoEFrag callback
* Reorder functions in the Governor module

ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/Run.hs

@@ -21,8 +21,7 @@ import           Data.Map.Strict (Map)
 import qualified Data.Map.Strict as Map
 import           Ouroboros.Consensus.Block
 import           Ouroboros.Consensus.Config (TopLevelConfig (..))
-import           Ouroboros.Consensus.Genesis.Governor (runGdd,
-                     updateLoEFragGenesis)
+import           Ouroboros.Consensus.Genesis.Governor (runGDDGovernor)
 import           Ouroboros.Consensus.Ledger.SupportsProtocol
                      (LedgerSupportsProtocol)
 import           Ouroboros.Consensus.MiniProtocol.ChainSync.Client
@@ -365,11 +364,16 @@ startNode schedulerConfig genesisTest interval = do
   -- peer fragments than registered clients.
   BlockFetch.startBlockFetchLogic lrRegistry lrTracer lnChainDb fetchClientRegistry getCandidates
 
-  let gdd = updateLoEFragGenesis lrConfig (mkGDDTracerTestBlock lrTracer) (readTVar handles)
-      gddTrigger = viewChainSyncState handles (\ s -> (csLatestSlot s, csIdling s))
+  let gddTrigger = viewChainSyncState handles (\ s -> (csLatestSlot s, csIdling s))
   for_ lrLoEVar $ \ var -> do
       forkLinkedThread lrRegistry "LoE updater background" $
-        void $ runGdd gdd var lnChainDb gddTrigger
+        void $ runGDDGovernor
+          lrConfig
+          (mkGDDTracerTestBlock lrTracer)
+          (readTVar handles)
+          var
+          lnChainDb
+          gddTrigger
   where
     LiveResources {lrRegistry, lrTracer, lrConfig, lrPeerSim, lrLoEVar} = resources
 

ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/Trace.hs

@@ -483,7 +483,7 @@ terseGDDEvent :: TraceGDDEvent PeerId TestBlock -> String
 terseGDDEvent = \case
   TraceGDDEvent {sgen = GenesisWindow sgen, curChain, bounds, candidates, candidateSuffixes, losingPeers, loeHead} ->
     unlines $ [
-      "GDG | Window: " ++ window sgen loeHead,
+      "GDD | Window: " ++ window sgen loeHead,
       "      Selection: " ++ terseHFragment curChain,
       "      Candidates:"
       ] ++

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Genesis/Governor.hs

@@ -13,25 +13,23 @@
 -- The GDD governor is the component responsible for identifying and
 -- disconnecting peers offering sparser chains than the best. This has the
 -- effect of unblocking the Limit on Eagerness, since removing disagreeing
--- peers allows the current selection to advance.
+-- peers allows the current selection to advance. See
+-- 'Ouroboros.Consensus.Storage.ChainDB.API.LoE' for more details.
 --
--- The GDD governor, invoked with 'runGdd', is supposed to run in a background
+-- The GDD governor, invoked with 'runGDDGovernor', is supposed to run in a background
 -- thread. It evaluates candidate chains whenever they change, or whenever a
 -- peer claims to have no more headers, or whenever a peer starts sending
 -- headers beyond the forecast horizon.
 --
--- Whenever GDD disconnects peers, the chain selection is updated.
+-- Whenever GDD disconnects peers, and as a result the youngest header present
+-- in all candidate fragments changes, the chain selection is updated.
 --
 module Ouroboros.Consensus.Genesis.Governor (
     DensityBounds (..)
   , TraceGDDEvent (..)
-  , UpdateLoEFrag (..)
   , densityDisconnect
-  , runGdd
+  , runGDDGovernor
   , sharedCandidatePrefix
-  , updateLoEFragGenesis
-  , updateLoEFragStall
-  , updateLoEFragUnconditional
   ) where
 
 import           Control.Monad (guard, when)
@@ -43,7 +41,6 @@ import qualified Data.Map.Strict as Map
 import           Data.Maybe (maybeToList)
 import           Data.Void
 import           Data.Word (Word64)
-import           GHC.Generics (Generic)
 import           Ouroboros.Consensus.Block
 import           Ouroboros.Consensus.Config (TopLevelConfig, configLedger,
                      configSecurityParam)
@@ -68,29 +65,124 @@ import           Ouroboros.Consensus.Util.STM (blockUntilChanged)
 import           Ouroboros.Network.AnchoredFragment (AnchoredFragment)
 import qualified Ouroboros.Network.AnchoredFragment as AF
 
--- | An action representing an update to the LoE fragment, that determines which
--- blocks can be selected in the ChainDB. With Ouroboros Genesis, this is
--- implemented via the GDD governor, see 'updateLoEFragGenesis'.
+-- | A never ending computation that evaluates the GDD rule whenever
+-- the STM action @getTrigger@ yields a different result, writing the LoE
+-- fragment to @varLoEFrag@, and then triggering ChainSel to reprocess all
+-- blocks that had previously been postponed by the LoE.
 --
--- The callback is applied to the current chain and the current ledger state,
--- and yields the new LoE fragment, which should be anchored in the immutable
--- tip.
-data UpdateLoEFrag m blk = UpdateLoEFrag {
-    updateLoEFrag ::
-         AnchoredFragment (Header blk)
-      -> ExtLedgerState blk
-      -> m (AnchoredFragment (Header blk))
-  }
-  deriving stock (Generic)
-  deriving anyclass (NoThunks)
+-- Evaluating the GDD rule might cause peers to be disconnected if they have
+-- sparser chains than the best chain.
+--
+-- The LoE fragment is the fragment anchored at the immutable tip and ending at
+-- the LoE tip.
+--
+-- @getHandles@ is the callback to get the handles that allow to disconnect
+-- from peers.
+--
+runGDDGovernor ::
+  ( Monoid a,
+    Eq a,
+    HasHardForkHistory blk,
+    IOLike m,
+    LedgerSupportsProtocol blk,
+    Ord peer
+  ) =>
+  TopLevelConfig blk ->
+  Tracer m (TraceGDDEvent peer blk) ->
+  STM m (Map peer (ChainSyncClientHandle m blk)) ->
+  StrictTVar m (AnchoredFragment (Header blk)) ->
+  ChainDB m blk ->
+  STM m a ->
+  m Void
+runGDDGovernor cfg tracer getHandles varLoEFrag chainDb getTrigger =
+  spin mempty
+  where
+    spin oldTrigger = do
+      (newTrigger, curChain, curLedger) <- atomically $ do
+        (_, newTrigger) <- blockUntilChanged id oldTrigger getTrigger
+        curChain <- ChainDB.getCurrentChain chainDb
+        curLedger <- ChainDB.getCurrentLedger chainDb
+        pure (newTrigger, curChain, curLedger)
+      loeFrag <- evaluateGDD cfg tracer getHandles curChain curLedger
+      oldLoEFrag <- atomically $ swapTVar varLoEFrag loeFrag
+      -- The chain selection only depends on the LoE tip, so there
+      -- is no point in retriggering it if the LoE tip hasn't changed.
+      when (AF.headHash oldLoEFrag /= AF.headHash loeFrag) $
+        triggerChainSelectionAsync chainDb
+      spin newTrigger
 
--- | A dummy version of the LoE that sets the LoE fragment to the current
--- selection. This can be seen as emulating Praos behavior.
-updateLoEFragUnconditional ::
-  MonadSTM m =>
-  UpdateLoEFrag m blk
-updateLoEFragUnconditional =
-  UpdateLoEFrag $ \ curChain _ -> pure curChain
+-- | Disconnect peers that lose density comparisons and recompute the LoE fragment.
+--
+-- Disconnecting peers causes candidate fragments to be removed, which causes
+-- the GDD governor to reevaluate GDD over and over until no more peers are
+-- disconnected.
+--
+-- @getHandles@ is the callback to get the handles that allow to disconnect
+-- from peers.
+--
+-- @curChain@ is the current chain selection.
+--
+-- @immutableLedgerSt@ is the current ledger state.
+--
+-- Yields the new LoE fragment.
+--
+evaluateGDD ::
+     forall m blk peer.
+     ( IOLike m
+     , Ord peer
+     , LedgerSupportsProtocol blk
+     , HasHardForkHistory blk
+     )
+  => TopLevelConfig blk
+  -> Tracer m (TraceGDDEvent peer blk)
+  -> STM m (Map peer (ChainSyncClientHandle m blk))
+  -> AnchoredFragment (Header blk)
+  -> ExtLedgerState blk
+  -> m (AnchoredFragment (Header blk))
+evaluateGDD cfg tracer getHandles curChain immutableLedgerSt = do
+    (states, candidates, candidateSuffixes, handles, loeFrag) <- atomically $ do
+      handles <- getHandles
+      states <- traverse (readTVar . cschState) handles
+      let
+        candidates = csCandidate <$> states
+        (loeFrag, candidateSuffixes) =
+          sharedCandidatePrefix curChain candidates
+      pure (states, candidates, candidateSuffixes, handles, loeFrag)
+
+    let msgen :: Maybe GenesisWindow
+        -- This could also use 'runWithCachedSummary' if deemed desirable.
+        msgen = eitherToMaybe $ runQuery qry summary
+          where
+            -- We use the Genesis window for the first slot /after/ the common
+            -- intersection. In particular, when the intersection is the last
+            -- slot of an era, we will use the Genesis window of the next era,
+            -- as all slots in the Genesis window reside in that next era.
+            slot    = succWithOrigin $ AF.headSlot loeFrag
+            qry     = qryFromExpr $ slotToGenesisWindow slot
+            summary =
+              hardForkSummary
+                (configLedger cfg)
+                -- Due to the cross-chain lemma (Property 17.3 in the Consensus
+                -- report) one could also use the ledger state at the tip of our
+                -- selection here (in which case this should never return
+                -- 'Nothing'), but this is subtle and maybe not desirable.
+                --
+                -- In any case, the immutable ledger state will also
+                -- /eventually/ catch up to the LoE tip, so @msgen@ won't be
+                -- 'Nothing' forever.
+                (ledgerState immutableLedgerSt)
+
+    whenJust msgen $ \sgen -> do
+      let
+        (losingPeers, bounds) =
+          densityDisconnect sgen (configSecurityParam cfg) states candidateSuffixes loeFrag
+        loeHead = AF.headAnchor loeFrag
+
+      traceWith tracer TraceGDDEvent {sgen, curChain, bounds, candidates, candidateSuffixes, losingPeers, loeHead}
+
+      for_ losingPeers $ \peer -> cschGDDKill (handles Map.! peer)
+
+    pure loeFrag
 
 -- | Compute the fragment @loeFrag@ between the immutable tip and the
 -- earliest intersection between @curChain@ and any of the @candidates@.
@@ -121,55 +213,6 @@ sharedCandidatePrefix curChain candidates =
       -- 'Map' via 'mapMaybe'.
       Map.mapMaybe splitAfterImmutableTip candidates
 
--- | This version of the LoE implements part of the intended Genesis approach.
--- The fragment is set to the prefix of all candidates, ranging from the
--- immutable tip to the earliest intersection of all peers.
---
--- Using this will cause ChainSel to stall indefinitely, or until a peer
--- disconnects for unrelated reasons.
--- In the future, the Genesis Density Disconnect Governor variant will extend
--- this with an analysis that will always result in disconnections from peers
--- to ensure the selection can advance.
-updateLoEFragStall ::
-  MonadSTM m =>
-  GetHeader blk =>
-  STM m (Map peer (AnchoredFragment (Header blk))) ->
-  UpdateLoEFrag m blk
-updateLoEFragStall getCandidates =
-  UpdateLoEFrag $ \ curChain _ ->
-    atomically $ do
-      candidates <- getCandidates
-      pure (fst (sharedCandidatePrefix curChain candidates))
-
--- | A never ending computation that runs the GDD governor whenever
--- the STM action @getTrigger@ changes, writing the LoE fragment
--- computed by @loEUpdater@ to @varLoEFrag@, and then triggering
--- ChainSel to reprocess all blocks that had previously been
--- postponed by the LoE.
-runGdd ::
-  (Monoid a, Eq a, IOLike m, LedgerSupportsProtocol blk) =>
-  UpdateLoEFrag m blk ->
-  StrictTVar m (AnchoredFragment (Header blk)) ->
-  ChainDB m blk ->
-  STM m a ->
-  m Void
-runGdd loEUpdater varLoEFrag chainDb getTrigger =
-  spin mempty
-  where
-    spin oldTrigger = do
-      (newTrigger, curChain, curLedger) <- atomically $ do
-        (_, newTrigger) <- blockUntilChanged id oldTrigger getTrigger
-        curChain <- ChainDB.getCurrentChain chainDb
-        curLedger <- ChainDB.getCurrentLedger chainDb
-        pure (newTrigger, curChain, curLedger)
-      loeFrag <- updateLoEFrag loEUpdater curChain curLedger
-      oldLoEFrag <- atomically $ swapTVar varLoEFrag loeFrag
-      -- The chain selection only depends on the LoE tip, so there
-      -- is no point in retriggering it if the LoE tip hasn't changed.
-      when (AF.headHash oldLoEFrag /= AF.headHash loeFrag) $
-        triggerChainSelectionAsync chainDb
-      spin newTrigger
-
 data DensityBounds blk =
   DensityBounds {
     clippedFragment :: AnchoredFragment (Header blk),
@@ -194,8 +237,14 @@ data DensityBounds blk =
 -- ChainSync instruction the peer sent, and whether the peer is idling (i.e. it
 -- sent @MsgAwaitReply@).
 --
--- @loeFrag@ is the fragment from the immutable tip to the first intersection
--- with a candidate fragment.
+-- @loeFrag@ is the fragment anchored at the immutable tip and ending in the
+-- LoE tip.
+--
+-- ChainSync jumping depends on this function to disconnect either of any two
+-- peers that offer different chains and provided a header in the last slot of
+-- the genesis window or later. Either of them should be disconnected, even if
+-- both of them are serving adversarial chains. See
+-- "Ouroboros.Consensus.MiniProtocol.ChainSync.Client.Jumping" for more details.
 --
 densityDisconnect ::
      ( Ord peer
@@ -281,6 +330,9 @@ densityDisconnect (GenesisWindow sgen) (SecurityParam k) states candidateSuffixe
       guard $ AF.lastPoint frag0 /= AF.lastPoint frag1
       -- peer1 offers more than k blocks or peer0 has sent all headers in the
       -- genesis window after the intersection (idling or not)
+      --
+      -- Checking for offersMoreThanK is important to avoid disconnecting
+      -- competing honest peers when the syncing node is nearly caught up.
       guard $ offersMoreThanK || lb0 == ub0
       -- peer1 has the same or better density than peer0
       -- If peer0 is idling, we assume no more headers will be sent.
@@ -292,6 +344,15 @@ densityDisconnect (GenesisWindow sgen) (SecurityParam k) states candidateSuffixe
       -- This matters to ChainSync jumping, where adversarial dynamo and
       -- objector could offer chains of equal density.
       guard $ lb1 >= (if idling0 then lb0 else ub0)
+
+      -- We disconnect peer0 if there is at least another peer peer1 with a
+      -- chain which is at least as good, and peer0 is either idling or there is
+      -- no extension to peer0's chain that can make it better than peer1's, and
+      -- peer1's has more than k headers or peer0 has sent all its headers in
+      -- the genesis window anchored at the intersection.
+      --
+      -- A chain is "as good as another" if it has at least as many headers in
+      -- the genesis window anchored at the intersection.
       pure peer0
 
     loeIntersectionSlot = AF.headSlot loeFrag
@@ -338,74 +399,3 @@ data TraceGDDEvent peer blk =
     loeHead           :: AF.Anchor (Header blk),
     sgen              :: GenesisWindow
   }
-
--- | Update the LoE fragment.
---
--- See 'UpdateLoEFrag' for the definition of LoE fragment.
---
--- Additionally, disconnect the peers that lose density comparisons.
---
--- Disconnecting peers causes chain fragments to be removed, which causes
--- the LoE fragment to be updated over and over until no more peers are
--- disconnected.
---
--- @getCandidates@ is the callback to obtain the candidate fragments
---
--- @getHandles@ is the callback to get the handles that allow to disconnect
--- from peers.
-updateLoEFragGenesis ::
-     forall m blk peer.
-     ( IOLike m
-     , Ord peer
-     , LedgerSupportsProtocol blk
-     , HasHardForkHistory blk
-     )
-  => TopLevelConfig blk
-  -> Tracer m (TraceGDDEvent peer blk)
-  -> STM m (Map peer (ChainSyncClientHandle m blk))
-  -> UpdateLoEFrag m blk
-updateLoEFragGenesis cfg tracer getHandles =
-  UpdateLoEFrag $ \ curChain immutableLedgerSt -> do
-    (states, candidates, candidateSuffixes, handles, loeFrag) <- atomically $ do
-      handles <- getHandles
-      states <- traverse (readTVar . cschState) handles
-      let
-        candidates = csCandidate <$> states
-        (loeFrag, candidateSuffixes) =
-          sharedCandidatePrefix curChain candidates
-      pure (states, candidates, candidateSuffixes, handles, loeFrag)
-
-    let msgen :: Maybe GenesisWindow
-        -- This could also use 'runWithCachedSummary' if deemed desirable.
-        msgen = eitherToMaybe $ runQuery qry summary
-          where
-            -- We use the Genesis window for the first slot /after/ the common
-            -- intersection. In particular, when the intersection is the last
-            -- slot of an era, we will use the Genesis window of the next era,
-            -- as all slots in the Genesis window reside in that next era.
-            slot    = succWithOrigin $ AF.headSlot loeFrag
-            qry     = qryFromExpr $ slotToGenesisWindow slot
-            summary =
-              hardForkSummary
-                (configLedger cfg)
-                -- Due to the cross-chain lemma (Property 17.3 in the Consensus
-                -- report) one could also use the ledger state at the tip of our
-                -- selection here (in which case this should never return
-                -- 'Nothing'), but this is subtle and maybe not desirable.
-                --
-                -- In any case, the immutable ledger state will also
-                -- /eventually/ catch up to the LoE tip, so @msgen@ won't be
-                -- 'Nothing' forever.
-                (ledgerState immutableLedgerSt)
-
-    whenJust msgen $ \sgen -> do
-      let
-        (losingPeers, bounds) =
-          densityDisconnect sgen (configSecurityParam cfg) states candidateSuffixes loeFrag
-        loeHead = AF.headAnchor loeFrag
-
-      traceWith tracer TraceGDDEvent {sgen, curChain, bounds, candidates, candidateSuffixes, losingPeers, loeHead}
-
-      for_ losingPeers $ \peer -> cschGDDKill (handles Map.! peer)
-
-    pure loeFrag