Genesis milestone 13: documentation / performance optimization #1113

nbacquey · 2024-05-21T13:09:55Z

This PR improves the code documentation of Genesis, and implements some fixes and optimizations.

Documentation, refactoring, and testing

The code documentation of GDD, CSJ, and LoE is better connected in cb5b69a.

Additionally, cd032b1 brings small changes to the LoE, motivated by issues found while integrating the LoE into the ChainDB q-s-m tests (see #1119).

2f7152b enables CSJ in all of the uniform tests and tests run more tries now.

Shrinking of peer schedules has been simplified in 627634f, and 6b44bfb adds an extra field to the PointSchedule type to specify the time at the end of a test, allowing to clean up a lot of schedules.

Profiling and optimizations

We consider our baseline, the fastest possible syncing, to be the scenario in which the syncing peer connects to a caught up peer and syncs using Praos.

From the profiling of Genesis on mainnet data, the GDD governor and chain selection was revealed as one of the major sinks of CPU time when enabling Genesis and syncing from multiple peers (between 2 and 30 peers).

Chain selection has been made idempotent again, to save on useless recomputations in 82376e6. Idempotency had been lost in #1015. 64f3da3 also calls to change selection less often, by avoiding calls when the LoE fragment doesn't change in a significant way.

The overhead of GDD is eliminated in a592c65 by replacing a frequent call to AnchoredFragment.takeWhileOldest with a faster variant.

With these changes, the only significant overhead of syncing from multiple peers comes from the BlockFetch logic, which is the topic of ongoing work.

CSJ tests

The testing code has been adapted in a969b37 to allow to specify multiple honest peers in CSJ tests. CSJ tests are the first time we care of running multiple honest peers to check that header are downloaded from only one of them.

We also added a test in 1d7fc95 to check that headers are downloaded from at most one peer even in the presence of adversarial peers.

GSM integration

In b525959 Limit on Eagerness is enabled only while GSM is in syncing state, and 83b12ee does similarly for LoP.

f69fd1f makes possible to toggle genesis components with environment variables. This was helpful to measure the overheads of the individual components.

jasagredo

Some minor comments here and there. Also:

there are hlint warnings, please fix them before merge
There are commits which are Unverified, I doubt branch protection rules will allow you to merge this.

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

jasagredo · 2024-07-02T14:00:03Z

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

I wonder if this file will result in the same as the ResourceRegistry: we have an isolated component that does not depend on consensus, then it becomes difficult (in terms of bureaucracy) extracting it. I wonder if this should be a separate library.

I think all of our abstractions initially started in Consensus (eg the Fuse you added most recently); but we could change that and put it at least into a different library. Is it fine to do that later in a separate PR?

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

jasagredo · 2024-07-03T13:29:40Z

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

Couldn't the GSM and GsmState live under a Node/Genesis/. directory?

The GSM is also used for pre-Genesis bootstrap peers, but yeah, we could move it in that subdirectory.

The GsmState can't be moved there as it is used in the ChainSync client:

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

Lines 4 to 6 in 6b44bfb

-- | This module contains the definition of a state in the Genesis State Machine

-- (GSM). The GSM itself is defined in 'ouroboros-consensus-diffusion', but the

-- ChainSync client relies on its state.

jasagredo · 2024-07-03T13:30:08Z

...oros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/NodeKernel.hs

@@ -247,7 +253,7 @@ initNodeKernel args@NodeKernelArgs { registry, cfg, tracers
              , GSM.setCaughtUpPersistentMark = \upd ->
                  (if upd then GSM.touchMarkerFile else GSM.removeMarkerFile)
                    gsmMarkerFileView
-              , GSM.writeGsmState = \gsmState ->
+              , GSM.writeGsmState = \gsmState -> do


Is this about the extraneous do at the end of the line?

Suggested change

, GSM.writeGsmState = \gsmState -> do

, GSM.writeGsmState = \gsmState ->

jasagredo · 2024-07-03T13:31:27Z

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

I'm in general a bit concerned that Genesis seems to permeate into ouroboros-consensus and ouroboros-consensus-diffusion with subtle subtleties on which concepts go into each package

Yeah, partly, I think this is because we don't really have a great distinction already in main (eg why does the forging loop live in ouroboros-consensus-diffusion?). We might think about this and shuffle some things around, but probably in a later PR in case that is fine.

dnadales

Co-authored with @jasagredo

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

dnadales · 2024-07-02T13:18:03Z

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

@@ -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


Same here, variable loeFrag is not visible in the resulting haddock.

It is introduced in the first line of this comment.

-- | @densityDisconnect genWin k states candidateSuffixes loeFrag@ -- yields the list of peers which are known to lose the density comparison with -- any other peer, when looking at the genesis window after @loeFrag@. -- ...

Let me know if that doesn't work still.

dnadales · 2024-07-02T13:21:16Z

ouroboros-consensus/src/ouroboros-consensus/Ouroboros/Consensus/Storage/ChainDB/API.hs

 --
-- This type indicates whether the feature is enabled, and contains a value
-- if it is.
+-- This type indicates whether LoE is enabled, and contains a value if it is.


Can we explain what this value represents and give some examples?

The meaning of a depends on the context on which the LoE is used. There is no intended use at the definition.

LoE a is isomorphic to Maybe a, with the added meaning that Just is only used if LoE is enabled. I elaborated this point in 20d86b3.

dnadales · 2024-07-02T13:22:33Z

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

@@ -649,7 +648,7 @@ chainSelectionForBlock cdb@CDB{..} blockCache hdr punish = electric $ do
         -- ^ The current chain and ledger
      -> LoE (AnchoredFragment (Header blk))
         -- ^ LoE fragment
-      -> LoELimit
+      -> LoE Word64


In which cases will LoE be parametrized with other types?

For example here:

type GetLoEFragment m blk = m (LoE (AnchoredFragment (Header blk)))

dnadales · 2024-07-02T13:36:19Z

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

Why is this file not part of ouroboros-consensus like the GSM state?

I don't have a good answer (note that the GSM was introduced in #808, not in this PR), also see #1113 (comment)

dnadales · 2024-07-03T16:02:17Z

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

+-- 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


Same for the reference to LoE.

This bit of the comment has been removed, but there is also a reference for LoE in the module header.

dnadales · 2024-07-03T16:16:53Z

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

+import           Ouroboros.Network.AnchoredFragment (AnchoredFragment)
+import qualified Ouroboros.Network.AnchoredFragment as AF
+
+-- We have multiple other Genesis-related types of a similar shape ('LoE', LoP


Add a REVIEW or TODO keyword maybe?

This comment has been removed.

dnadales · 2024-07-03T16:17:17Z

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

+  , gcsCSJConfig                :: !CSJConfig
+  }
+
+-- TODO justification/derivation from other parameters


When do we want to address this?

Probably after "real world" performance benchmarks on resource-limited machines. For now, the values are conservative; tuning them could be one part of a phased rollout of Genesis.

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

dnadales · 2024-07-03T16:36:30Z

ouroboros-consensus/changelog.d/20240531_155125_alexander.esgen_milestone_13.md

@@ -0,0 +1,4 @@
+### Breaking
+
+- Internal refactorings in the CSJ and GDD.


We should use the same grammatical form across all changelogs (at least within the same PR :))

Thanks, addressed 👍

This also involves some fairly important changes to the leaky bucket and to the leaky bucket API. These changes make the leaky bucket significantly more robust. Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io>

Also: * Simplify chainDiffs computation in chainSelectionForBlock * Revert "Don't allow to extend the selection by more than k blocks" This reverts commit c8468c2. * Revert "Retrigger GDD when the selection changes" This reverts commit f990d41.

…t change

…f them

* Rewrite `Peers` to accept arbitrary number of peers * Actually generate honest peers in CSJ happy path * Support a field for extra honest peers in `GenesisTest` * Allow `uniformPoints` to generate schedules with multiple honest peers * Adapt CSJ test to use native multiple honest peers generation * Share partial accessor functions used in tests * Use partial accessor to retrieve the only honest peer

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

…' and disable timeouts as they are supposed to be

Also: * LoEEnabled: make payload strict to avoid NoThunks failures * LoE: allow to dynamically en-/disable * Depending on the state of the GSM, we want to either en- or disable the LoE. * Refactor `runGdd` to use `Watcher`, share trigger logic Previously, we would duplicate the logic for when to trigger the GDD between the NodeKernel and the peer simulator. * Add GDD tracing The `Show` instances are probably way too large ATM, but there are currently unused, so that isn't a pressing concern. * LoP rate: fix typo 500/s = 1/2ms, not 2/ms 🤦

Previously, it wasn't possible to eg run *just* CSJ.

Modify the honest shrinking function by no longer speeding up the other schedules when an honest tick is deleted. This simplifies a lot of code in the `Shrinking` module, at the cost of no longer ensuring that shrunk schedules preserve the overall order of events. Additionally, we re-enable shrinking in CSJ tests Also: * Extend adversarial schedules when shrinking an honest one * Document the cases when we don't use shrinking

facundominguez · 2024-07-10T16:02:25Z

I rebased main.

Co-authored-by: Javier Sagredo <javier.sagredo@iohk.io>

Co-authored-by: Damian Nadales <damian.only@gmail.com> Co-authored-by: Javier Sagredo <javier.sagredo@iohk.io>

Fixup for commit 9011106 Rework `followsLoEFrag` into `trimToLoE`

facundominguez · 2024-07-11T14:45:27Z

I'm labeling "no changelog" since I don't think the changes to ouroboros-consensus-cardano deserve a changelog entry in this PR. But let me know if something else should be done about it.

nbacquey added documentation Improvements or additions to documentation do-not-merge Genesis PRs related to Genesis testing and implementation labels May 21, 2024

nbacquey changed the base branch from main to genesis/milestone-12-chain-sync-jumping May 21, 2024 13:12

nbacquey force-pushed the genesis/milestone-12-chain-sync-jumping branch from 980c35a to ce3b2c6 Compare May 29, 2024 15:37

Base automatically changed from genesis/milestone-12-chain-sync-jumping to main May 29, 2024 18:14

nbacquey force-pushed the genesis/milestone-13 branch from 241f00d to bbcd6e3 Compare May 30, 2024 14:45

facundominguez force-pushed the genesis/milestone-13 branch 5 times, most recently from db11bc1 to 2acc4b3 Compare May 30, 2024 19:23

Niols marked this pull request as ready for review June 10, 2024 15:42

Niols requested a review from a team as a code owner June 10, 2024 15:42

dnadales self-assigned this Jun 14, 2024

jasagredo reviewed Jul 3, 2024

View reviewed changes

dnadales approved these changes Jul 3, 2024

View reviewed changes

Niols and others added 13 commits July 10, 2024 09:32

Make the LoP follow the GSM states

30da324

This also involves some fairly important changes to the leaky bucket and to the leaky bucket API. These changes make the leaky bucket significantly more robust. Co-authored-by: Alexander Esgen <alexander.esgen@iohk.io>

Don't retrigger chain selection if the tip of the loe fragment doesn'…

1708b34

…t change

Increase amount of tries in the Uniform tests and enable CSJ in all o…

d6d84f3

…f them

Add some adversaries to the “happy path” test

3ec40c2

Edit documentation of LoE and GDD

6191884

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

Explain more the shrinking choice of test 'serve adversarial branches…

94b89f3

…' and disable timeouts as they are supposed to be

Automated watcher of CSJ state invariants

3f952fe

Allow to disable *all* Genesis components individually

1cf7129

Previously, it wasn't possible to eg run *just* CSJ.

Avoid reconstructing the candidateSuffixes in densityDisconnect

e6d057b

facundominguez requested review from fraser-iohk and RenateEilers as code owners July 10, 2024 16:01

facundominguez and others added 17 commits July 10, 2024 13:07

Fix grammar in comments

461f165

Co-authored-by: Javier Sagredo <javier.sagredo@iohk.io>

Fix comments in LeakyBucket.hs

f2763da

Co-authored-by: Damian Nadales <damian.only@gmail.com> Co-authored-by: Javier Sagredo <javier.sagredo@iohk.io>

Expand HAA acronym

47cc624

Remove redundant do keyword

1fe1b10

Replace Bool with isomorphic and more telling datatypes in CSJ test

4db1b7a

Fix double quotes and make Note visible in haddock

a50026e

Update haddock comments in the Governor module

f6105ba

Elaborate on the meaning of the type parameter of LoE

20d86b3

Remove redundant parenthesis

f133483

Fix formatting in haddock comment

302b9d8

Please hlint

bb3f5a0

Adapt to change of type in maximalCandidates.

39d7d4d

Fixup for commit 9011106 Rework `followsLoEFrag` into `trimToLoE`

LeakyBucket: assert timeToWaitMicroseconds > 0

32a2fd0

LeakyBucket: wrap leak in forever for clarity

d4cbcb8

fixup! LeakyBucket: assert timeToWaitMicroseconds > 0

1af233e

ChainSync client state: cschGsmCallback -> cschOnGsmStateChanged

7d02b4c

changelog: use consistent grammar

01d4594

facundominguez added the no changelog label Jul 11, 2024

facundominguez force-pushed the genesis/milestone-13 branch from 6bcfdc9 to 5680c0b Compare July 11, 2024 15:20

facundominguez enabled auto-merge July 11, 2024 16:45

Make types and comments a bit more precise for the leak thread

53ea76c

facundominguez force-pushed the genesis/milestone-13 branch from 5680c0b to 53ea76c Compare July 11, 2024 17:28

facundominguez added this pull request to the merge queue Jul 11, 2024

Merged via the queue into main with commit 1fb0c16 Jul 11, 2024
15 checks passed

facundominguez deleted the genesis/milestone-13 branch July 11, 2024 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Genesis milestone 13: documentation / performance optimization #1113

Genesis milestone 13: documentation / performance optimization #1113

nbacquey commented May 21, 2024 •

edited by Niols

Loading

jasagredo left a comment

jasagredo Jul 2, 2024

amesgen Jul 4, 2024

jasagredo Jul 3, 2024

amesgen Jul 4, 2024 •

edited

Loading

jasagredo Jul 3, 2024

amesgen Jul 10, 2024

facundominguez Jul 10, 2024

jasagredo Jul 3, 2024

amesgen Jul 4, 2024

dnadales left a comment

dnadales Jul 2, 2024

facundominguez Jul 10, 2024

dnadales Jul 2, 2024

facundominguez Jul 10, 2024

dnadales Jul 2, 2024

amesgen Jul 4, 2024

dnadales Jul 2, 2024

amesgen Jul 4, 2024

dnadales Jul 3, 2024

facundominguez Jul 10, 2024

dnadales Jul 3, 2024

facundominguez Jul 10, 2024

dnadales Jul 3, 2024

amesgen Jul 4, 2024

dnadales Jul 3, 2024

amesgen Jul 11, 2024

facundominguez commented Jul 10, 2024

facundominguez commented Jul 11, 2024

	-- \| This module contains the definition of a state in the Genesis State Machine
	-- (GSM). The GSM itself is defined in 'ouroboros-consensus-diffusion', but the
	-- ChainSync client relies on its state.

	, GSM.writeGsmState = \gsmState -> do
	, GSM.writeGsmState = \gsmState ->

		@@ -0,0 +1,4 @@
		### Breaking

		- Internal refactorings in the CSJ and GDD.

Genesis milestone 13: documentation / performance optimization #1113

Genesis milestone 13: documentation / performance optimization #1113

Conversation

nbacquey commented May 21, 2024 • edited by Niols Loading

Documentation, refactoring, and testing

Profiling and optimizations

CSJ tests

GSM integration

jasagredo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amesgen Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadales left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facundominguez commented Jul 10, 2024

facundominguez commented Jul 11, 2024

nbacquey commented May 21, 2024 •

edited by Niols

Loading

amesgen Jul 4, 2024 •

edited

Loading