BulkSync BlockFetch implementation for Genesis #4919

facundominguez · 2024-07-29T14:30:18Z

This pull request implements a BlockFetch logic to use when synchronizing peers with Genesis.

BlockFetch needs to penalize peers that are slow enough to delay syncing in order to fend from attacks targeting the synchronizing nodes.

The penalization consists in switching the serving peer when blocks are in flight long enough that the syncing node is idle while waiting for more blocks to validate.

Switching peers, in turn, requires organizing peers in a queue, where blocks are retrieved from the first peer in the queue that can serve them, and any time a peer is penalized, it is moved to the end of the queue.

Most of the implementation is in the first commit where the implementation of BulkSync mode is replaced. Probably the best file to approach the implementation is ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs. There are supporting commits to split large modules into smaller ones, and to update the tests related to the BulkSync mode.

ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs

ouroboros-network/src/Ouroboros/Network/BlockFetch/ClientState.hs

ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs

facundominguez · 2024-08-21T17:17:06Z

I force-pushed a rehashing of the changes that preserves the old BulkSync mode, which the network team suggested in the last review call.

coot

Thank you @facundominguez for the PR. My main concern is using FetchMode while LedgerStateJudgement might be more suitable for the Genesis block-fetch client. I wonder what you guys think about it.

coot · 2024-09-11T07:39:06Z

ouroboros-network-api/src/Ouroboros/Network/BlockFetch/ConsensusInterface.hs

+data GenesisFetchMode = FetchModeGenesis | PraosFetchMode FetchMode
+  deriving (Eq, Show)


Is this the right type to use? There's also LedgerStateJudgement. Unlike the original FetchMode it is governed by the genesis state machine.

Consensus switches between FetchModeBulkSync and FetchModeDeadline (I'm ignoring bootstrap peers) whether the node is more than 1000 slots behind or not. ref

I suspect this will also simplify integration on the ouroboros-cosensus side since you'll not conflict Praos and Genesis notions.

FTR we talked about this in the Networking call yesterday:

When we integrate this into Consensus, we will make sure that when Genesis is enabled, readFetchModeDefault will uses the current LedgerStateJudgement to decide which FetchMode to use (concretely, FetchModeGenesis when TooOld, and PraosFetchMode FetchModeDeadline when YoungEnough). (cc @crocodile-dentist)

However, when Genesis is disabled, we want to keep the current behavior (based on slot distance to the tip), so it makes sense to keep FetchMode/GenesisFetchMode for now.

Does that make sense?

I think this will be confusing to use FetchMode and LedgerStateJudgement in this way. Why not just directly use LedgerStateJudgement? Can we use FetchMode in the legacy block-fetch implementation, but LedgerStateJudgment in the new one? There's ConsensusMode available from ouroboros-network-api (which is passed through Ouroboros.Network.Diffusion.P2P.ArugmentsExtra from consensus) - this is the switch that is required.

We could nicely abstract this using something like this:

data EffectiveFetchMode = BulkSyncMode | DeadlineMode effectiveFetchMode :: ConsensusMode -> m FetchMode -> m LedgerStateJudgement -> m EffectiveFetchMode effectiveFetchMode GenesisMode _ lsj = _f <$> lsj effectiveFetchMode PraosMode fm _ = _g <$> fm

So I think we are in agreement of what the semantics should be, i.e. when to use old/new BulkSync vs Deadline logic in Praos/Genesis mode:

In Praos mode: Use the existing readFetchModeDefault logic

In Genesis mode: Use the current LedgerStateJudgement to decide, ie FetchModeGenesis when TooOld, and FetchModeDeadline when YoungEnough.

The question now is where this logic should live.

It could live entirely in Consensus, such that the BlockFetch logic in Network still is only aware of the FetchMode.

It could be spread across Network and Consensus, such that Network makes the decision whether to use the FetchMode or the LedgerStateJudgement, basically your effectiveFetchMode.

I have no strong opinion here; the second option would require passing down more stuff to Network for BlockFetch, but we already pass down these things in other places, so it doesn't really increase the API surface between Network and Consensus.

So I think we are in agreement of what the semantics should be
Yes we are

The question now is where this logic should live.

It would be nice to design something that's consistent, so it's easy which parts belong to genesis and which to praos. If it's just about the genesis block-fetch implementation it also could just directly use LedgerStateJudgement (it will only run if GenesisMode is on).

ouroboros-network/src/Ouroboros/Network/PeerSelection/Churn.hs

ouroboros-network/src/Ouroboros/Network/BlockFetch.hs

ouroboros-network/src/Ouroboros/Network/BlockFetch/State.hs

coot · 2024-09-11T07:02:31Z

ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/Genesis.hs

+
+          -- Trim the fragments to the peer's candidate, keeping only blocks that
+          -- they may actually serve.
+          trimmedFragments <- trimFragmentsToCandidate thePeerCandidate (snd fragments)


Haven't we picked a peer which can serve these blocks in selectThePeer?

ouroboros-network-api/src/Ouroboros/Network/BlockFetch/ConsensusInterface.hs

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/Diffusion/Node.hs

coot · 2024-09-11T10:42:10Z

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/BlockFetch.hs

-  [ testProperty "static chains without overlap"
-                 prop_blockFetchStaticNoOverlap
-
-  , testProperty "static chains with overlap"
-                 prop_blockFetchStaticWithOverlap
-
+  [ testGroup "BulkSync"
+    [ testProperty "static chains without overlap" $
+                   prop_blockFetchStaticNoOverlap (PraosFetchMode FetchModeBulkSync)
+
+    , testProperty "static chains with overlap" $
+                   prop_blockFetchStaticWithOverlap (PraosFetchMode FetchModeBulkSync)
+
+    --TODO: test where for any given delta-Q, check that we do achieve full
+    -- pipelining to keep the server busy and get decent enough batching of
+    -- requests (testing the high/low watermark mechanism).
+    , testProperty "termination" $
+                   prop_terminate (PraosFetchMode FetchModeBulkSync)
+    ]
+  , testGroup "Genesis"
+    [ testProperty "static chains without overlap" $
+                   prop_blockFetchStaticNoOverlap FetchModeGenesis
+
+    , testProperty "static chains with overlap" $
+                   prop_blockFetchStaticWithOverlap FetchModeGenesis
+
+    , testProperty "termination" $
+                   prop_terminate FetchModeGenesis
+    ]
  , testCaseSteps "bracketSyncWithFetchClient"
                  unit_bracketSyncWithFetchClient
-
-  --TODO: test where for any given delta-Q, check that we do achieve full
-  -- pipelining to keep the server busy and get decent enough batching of
-  -- requests (testing the high/low watermark mechanism).
-  , testProperty "termination"
-                 prop_terminate
  , testProperty "compare comparePeerGSV" prop_comparePeerGSV
  , testProperty "eq comparePeerGSV" prop_comparePeerGSVEq
  ]


In such case we were usualy more verbose and defined things like prop_blockFetchStaticWithOverlapGenesis and prop_blockFetchStaticWithOverlapPraos.

This makes it easier to work with a failing test since the fetch mode is already defined in the closure.

Just to make sure: So copy-pasting the tests for Praos and Genesis, respectively?

ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/BlockFetch.hs

The new logic aims to guarantee progress of synchronization via genesis. See the comments in ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/Genesis.hs Co-authored-by: Nicolas BACQUEY <nicolas.bacquey@tweag.io> Co-authored-by: Nicolas “Niols” Jeannerod <nicolas.jeannerod@tweag.io>

facundominguez added the Genesis label Jul 29, 2024

facundominguez commented Jul 29, 2024

View reviewed changes

ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs Outdated Show resolved Hide resolved

facundominguez force-pushed the genesis/bulksync branch from f45bf35 to 5e4515e Compare July 29, 2024 15:40

amesgen reviewed Jul 29, 2024

View reviewed changes

ouroboros-network/src/Ouroboros/Network/BlockFetch/ClientState.hs Outdated Show resolved Hide resolved

ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs Outdated Show resolved Hide resolved

facundominguez changed the title ~~BulkSYnc BlockFetch implementation for Genesis~~ BulkSync BlockFetch implementation for Genesis Jul 29, 2024

facundominguez commented Jul 30, 2024

View reviewed changes

ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs Outdated Show resolved Hide resolved

amesgen reviewed Aug 1, 2024

View reviewed changes

ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs Outdated Show resolved Hide resolved

nbacquey force-pushed the genesis/bulksync branch from b171be1 to 0ef2f19 Compare August 1, 2024 13:03

facundominguez force-pushed the genesis/bulksync branch from 2a163ab to 440def5 Compare August 2, 2024 20:06

nbacquey force-pushed the genesis/bulksync branch from 440def5 to abf35e0 Compare August 5, 2024 11:52

facundominguez marked this pull request as ready for review August 5, 2024 12:11

facundominguez requested a review from a team as a code owner August 5, 2024 12:11

amesgen mentioned this pull request Aug 5, 2024

Full rework of the BlockFetch logic for bulk sync mode IntersectMBO/ouroboros-consensus#1179

Open

amesgen force-pushed the genesis/bulksync branch from abf35e0 to ebf401b Compare August 5, 2024 17:38

facundominguez force-pushed the genesis/bulksync branch from f16a4d6 to 0d08166 Compare August 21, 2024 17:13

coot added the block-fetch Issues related to block fetch component. label Sep 2, 2024

coot reviewed Sep 12, 2024

View reviewed changes

amesgen self-assigned this Sep 16, 2024

facundominguez and others added 7 commits September 17, 2024 18:34

Fix typo in error message

39a22e6

Ensure BulkSync tests terminate even if BulkSync misbehaves

7fd2cd6

Rename BlockFetch tests that only test BulkSync mode

bc20394

Parameterize BlockFetch tests by fetch mode

1a4eae3

Test FetchModeGenesis in BlockFetch tests

b3cb7e9

Please stylish-haskell

944bcca

amesgen force-pushed the genesis/bulksync branch from 0d08166 to 5441dc8 Compare September 18, 2024 14:24

facundominguez added 3 commits September 18, 2024 16:27

Allow to set the Genesis fetch mode in FetchDeclineConcurrencyLimit

8acf57f

GracePeriod rename

8bf522b

Change daBlockFetchMode to use GenesisBlockFetch

f217de0

amesgen force-pushed the genesis/bulksync branch from 5441dc8 to f217de0 Compare September 18, 2024 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BulkSync BlockFetch implementation for Genesis #4919

BulkSync BlockFetch implementation for Genesis #4919

facundominguez commented Jul 29, 2024 •

edited by amesgen

Loading

facundominguez commented Aug 21, 2024

coot left a comment

coot Sep 11, 2024

amesgen Sep 12, 2024

coot Sep 13, 2024

coot Sep 13, 2024 •

edited

Loading

amesgen Sep 13, 2024

coot Sep 16, 2024

coot Sep 11, 2024

coot Sep 11, 2024

amesgen Sep 13, 2024

		data GenesisFetchMode = FetchModeGenesis \| PraosFetchMode FetchMode
		deriving (Eq, Show)

BulkSync BlockFetch implementation for Genesis #4919

Are you sure you want to change the base?

BulkSync BlockFetch implementation for Genesis #4919

Conversation

facundominguez commented Jul 29, 2024 • edited by amesgen Loading

facundominguez commented Aug 21, 2024

coot 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

coot Sep 13, 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

facundominguez commented Jul 29, 2024 •

edited by amesgen

Loading

coot Sep 13, 2024 •

edited

Loading