-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BulkSync BlockFetch implementation for Genesis #4919
base: master
Are you sure you want to change the base?
Conversation
ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs
Outdated
Show resolved
Hide resolved
f45bf35
to
5e4515e
Compare
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
ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/src/Ouroboros/Network/BlockFetch/Decision/BulkSync.hs
Outdated
Show resolved
Hide resolved
b171be1
to
0ef2f19
Compare
2a163ab
to
440def5
Compare
440def5
to
abf35e0
Compare
abf35e0
to
ebf401b
Compare
f16a4d6
to
0d08166
Compare
I force-pushed a rehashing of the changes that preserves the old BulkSync mode, which the network team suggested in the last review call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
data GenesisFetchMode = FetchModeGenesis | PraosFetchMode FetchMode | ||
deriving (Eq, Show) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 currentLedgerStateJudgement
to decide whichFetchMode
to use (concretely,FetchModeGenesis
whenTooOld
, andPraosFetchMode FetchModeDeadline
whenYoungEnough
). (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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, ieFetchModeGenesis
whenTooOld
, andFetchModeDeadline
whenYoungEnough
.
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 theLedgerStateJudgement
, basically youreffectiveFetchMode
.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
|
||
-- Trim the fragments to the peer's candidate, keeping only blocks that | ||
-- they may actually serve. | ||
trimmedFragments <- trimFragmentsToCandidate thePeerCandidate (snd fragments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't we picked a peer which can serve these blocks in selectThePeer
?
ouroboros-network-api/src/Ouroboros/Network/BlockFetch/ConsensusInterface.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/Diffusion/Node.hs
Outdated
Show resolved
Hide resolved
[ 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 | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure: So copy-pasting the tests for Praos and Genesis, respectively?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, just defining wrapper functions which instantiate some of the parameters.
ouroboros-network/sim-tests-lib/Test/Ouroboros/Network/BlockFetch.hs
Outdated
Show resolved
Hide resolved
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 <[email protected]> Co-authored-by: Nicolas “Niols” Jeannerod <[email protected]>
0d08166
to
5441dc8
Compare
5441dc8
to
f217de0
Compare
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.