Skip to content
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.

Sign up for GitHub

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

Jump to bottom

consensus-testlib: add chain generators #240

Merged
merged 1 commit into from
Sep 29, 2023
Merged

consensus-testlib: add chain generators #240

merged 1 commit into from
Sep 29, 2023

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Jul 18, 2023

These generators are the first step towards a new set of tests, that will target the upcoming Ouroboros Genesis implementation.
One of the earlier milestones, however, will be some tests that apply even before Ouroboros Genesis.

The tip of the iceberg is the Test.Ouroboros.Consensus.ChainGenerator.Tests module. You can chase dependencies from there.

This PR was migrated from IntersectMBO/ouroboros-network#4438

@amesgen amesgen mentioned this pull request Jul 18, 2023
Closed
11 tasks
nfrisby
nfrisby reviewed Jul 18, 2023
View reviewed changes
ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Some.hs Outdated
@@ -0,0 +1,131 @@
{-# LANGUAGE DataKinds #-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module is a utility, much more general than ChainGenerators, but relied upon heavily by it.

nfrisby
nfrisby reviewed Jul 18, 2023
View reviewed changes
ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Counting.hs Outdated
@@ -0,0 +1,341 @@
{-# LANGUAGE ExistentialQuantification #-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module is a utility, much more general than ChainGenerators, but relied upon heavily by it.

facundominguez
facundominguez reviewed Aug 1, 2023
View reviewed changes
Copy link
Contributor

@facundominguez facundominguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sharing some comments. I have plenty to read yet.

I started looking at the honest tests and dived into some of the data type definitions.

ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Slot.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Slot.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Slot.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Slot.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Slot.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/BitVector.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Honest.hs Outdated
-----

data HonestRecipe = HonestRecipe !Kcp !Scg !Delta !Len
deriving (Eq, Read, Show)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this really a recipe? I'm finding it rather difficult to interpret. Maybe it is better described as parameters for leader schedule generation (?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be called something like HonestSchemaSpec.

Copy link
Contributor

@nfrisby nfrisby Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used Recipe as a synonym of Spec. I don't have a strong preference, though "spec(ification)" does have connotations around actual documents/broader writing/terms in a modelling language/etc, as opposed to mere data like this.

ouroboros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Honest.hs Outdated Show resolved Hide resolved
facundominguez
facundominguez reviewed Aug 15, 2023
View reviewed changes
...boros-consensus/src/consensus-testlib/Test/Ouroboros/Consensus/ChainGenerator/Adversarial.hs Outdated
Comment on lines 171 to 173
-- TODO optimization: how to skip all the honest Race Windows that
-- don't reach beyond the intersection? Perhaps skip to i - s + d?
go iterH iterA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing the TODO would require counting blocks backwards from the intersection. Is that ideal?

@nfrisby
Copy link
Contributor

nfrisby commented Aug 31, 2023

I pushed up several commits. Most are small. Most of them are rewordings of comments. But there is at least one big comment rewrite, on uniformTheHonestChain. cc @amesgen @facundominguez

@nfrisby nfrisby force-pushed the nfrisby/chain-generators branch 3 times, most recently from df454b3 to 9f17e66 Compare August 31, 2023 21:00
@nfrisby @facundominguez @amesgen
consensus-testlib: add ChainGenerators
ecfbdd6 
For those involved in this PR's development, this commit is a manual full
"squash" of 8f3b50b.

Co-authored-by: Facundo Domínguez <facundominguez@gmail.com>
Co-authored-by: amesgen <amesgen@amesgen.de>
@nfrisby nfrisby marked this pull request as ready for review August 31, 2023 21:04
@nfrisby nfrisby requested a review from a team as a code owner August 31, 2023 21:04
@nfrisby
Copy link
Contributor

nfrisby commented Aug 31, 2023 •
edited
Loading

@dnadales @jasagredo @fraser-iohk

Our Tweag team working on the Genesis Statement of Work is making two claims about this PR.

  • It is worthwhile.
  • It is completely additive: the only way it could disrupt the Consensus codebase is if the tests we're adding here end up being flaky.

Please inspect this PR enough to feel comfortable with those claims and Approve it if so.

In particular, if you doubt the worthwhileness but not the harmlessness, please consider Approving anyway, and we can discuss whether or not to back it out after the fact. Thank you!

@dnadales
Copy link
Member

dnadales commented Sep 1, 2023

I can confirm:

  • It is worthwhile.
  • It is completely additive: the only way it could disrupt the Consensus codebase is if the tests we're adding here end up being flaky.

@amesgen amesgen added this pull request to the merge queue Sep 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 28, 2023
@amesgen amesgen added this pull request to the merge queue Sep 29, 2023
Merged via the queue into main with commit c2a9735 Sep 29, 2023
12 checks passed
@amesgen amesgen deleted the nfrisby/chain-generators branch September 29, 2023 09:05
@facundominguez facundominguez mentioned this pull request Oct 13, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2023
@facundominguez
Implement the peer simulator for Genesis tests (#434)
d866659 
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:
 * a Praos client node that plays the role of subject under test,
 * an example test for long range attacks, and
* minimal definitions to generate a schedule from the chain schemas
introduced in #240.

This is a squash of the many commits in the integration branch of #367.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tek tek tek left review comments

@nfrisby nfrisby nfrisby left review comments

@facundominguez facundominguez facundominguez left review comments

@dnadales dnadales dnadales approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amesgen @nfrisby @dnadales @tek @facundominguez