-
Notifications
You must be signed in to change notification settings - Fork 22
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
Implement the peer simulator for Genesis tests #434
Conversation
283545a
to
5340e69
Compare
...oros-consensus-diffusion/test/consensus-test/Test/Consensus/Genesis/Tests/LongRangeAttack.hs
Outdated
Show resolved
Hide resolved
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.
Thanks. Some overall comments:
- Do we want to address all the comments marked as
REVIEW
in this PR? - Do we want to address all the comments marked as
FIXME
in this PR? - We need to make sure no record wildcards are used.
- It'd be great if we could use the same formatting style as the rest of the codebase. Also, at the moment the coding styles within this PR are mixed, eg leading vs trailing commas.
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/Genesis/Setup.hs
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/Genesis/Setup/Classifiers.hs
Outdated
Show resolved
Hide resolved
...os-consensus-diffusion/test/consensus-test/Test/Consensus/Network/AnchoredFragment/Extras.hs
Outdated
Show resolved
Hide resolved
...os-consensus-diffusion/test/consensus-test/Test/Consensus/Network/AnchoredFragment/Extras.hs
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/BlockFetch.hs
Show resolved
Hide resolved
traceWith tracer "Schedule is:" | ||
for_ ps $ \tick -> traceWith tracer $ " " ++ condense tick | ||
traceWith tracer "--------------------------------------------------------------------------------" | ||
traceWith tracer "» Time says “Let there be”" |
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.
Perhaps we should replace this with something that describes what these messages are delimiting.
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.
@Niols do you have any preferences of text to use here? "Running point schedule ..."?
traceWith tracer "--------------------------------------------------------------------------------" | ||
for_ ps (dispatchTick tracer peers) | ||
traceWith tracer "--------------------------------------------------------------------------------" | ||
traceWith tracer "» A Clock stopped -" |
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.
Same here.
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/Trace.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PointSchedule.hs
Show resolved
Hide resolved
@@ -0,0 +1,16 @@ | |||
module Test.QuickCheck.Extras ( |
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.
Perhaps these functions could be upstreamed to QC eventually ...
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.
Indeed.
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.
Do we want to address all the comments marked as REVIEW in this PR?
At this point, the REVIEW comments are notes for the Genesis team. Should we use a different tag for these?
Do we want to address all the comments marked as FIXME in this PR?
Likely the same as the previous item, but I'll ask.
We need to make sure no record wildcards are used.
It'd be great if we could use the same formatting style as the rest of the codebase.
Yes, I think at least I should be paying some more attention to the style guide than I've been doing so far.
...os-consensus-diffusion/test/consensus-test/Test/Consensus/Network/AnchoredFragment/Extras.hs
Show resolved
Hide resolved
...os-consensus-diffusion/test/consensus-test/Test/Consensus/Network/AnchoredFragment/Extras.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/BlockFetch.hs
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PointSchedule.hs
Show resolved
Hide resolved
@@ -0,0 +1,16 @@ | |||
module Test.QuickCheck.Extras ( |
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.
Indeed.
blockFetchCfg = BlockFetchConfiguration | ||
{ bfcMaxConcurrencyBulkSync = 2 | ||
, bfcMaxConcurrencyDeadline = 2 | ||
, bfcMaxRequestsInflight = 4 | ||
, bfcDecisionLoopInterval = 0 | ||
, bfcSalt = 0 | ||
} |
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.
The threadnet tests use hardcoded values as well.
One option would be to factor the hardcoded configuration and use it in both places.
Another alternative would be to make blockFetchCfg a function argument, and generate it with some randomness for all fields except bfcDecisionLoopInterval
which could interfere with the scheduling during tests and should remain set to 0.
Please, let us know if you have any preferences.
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/BlockFetch.hs
Show resolved
Hide resolved
…nt as suggested in #434
…nt as suggested in #434
f4ab0d0
to
3bd3be5
Compare
Co-authored-by: Nicolas “Niols” Jeannerod <nicolas.jeannerod@tweag.io> Co-authored-by: Torsten Schmits <torsten.schmits@tweag.io> Co-authored-by: Facundo Domínguez <facundo.dominguez@tweag.io>
…nt as suggested in #434
2f37f76
to
2076bac
Compare
I brought almost all of the changes in response to comments to this PR. The only exception is the documentation of point schedules which I left in #501. Also merged the latest changes from If there are no more requests, this is good to merge. |
This PR introduces an initial implementation of a Peer Simulator for Genesis tests.
A simulated peer is told how to behave by a list of scheduled events that is generated for every test. In essence, it is an implementation of a ChainSync server and a BlockFetch server that behave as prescribed by the schedule.
The rest of the PR provides support infrastructure in the form of:
This is a squash of the many commits in the integration branch of #367.