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.

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

Test promptness of tentative followers #3857

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Jul 4, 2022

Description

Closes CAD-4195

See the module description for motivation/approach.

Checklist

  • Branch
    • Commit sequence broadly makes sense
    • Commits have useful messages
    • New tests are added if needed and existing tests are updated
    • If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
    • If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
  • Pull Request
    • Self-reviewed the diff
    • Useful pull request description at least containing the following information:
      • What does this PR change?
      • Why these changes were needed?
      • How does this affect downstream repositories and/or end-users?
      • Which ticket does this PR close (if any)? If it does, is it linked?
    • Reviewer requested

@amesgen amesgen requested review from nfrisby and dnadales as code owners July 4, 2022 07:51
@amesgen amesgen added consensus issues related to ouroboros-consensus testing labels Jul 4, 2022
@iohk-bors iohk-bors bot deleted the branch master July 6, 2022 15:30
@iohk-bors iohk-bors bot closed this Jul 6, 2022
@amesgen amesgen reopened this Jul 6, 2022
@amesgen amesgen changed the base branch from amesgen/CAD-4193-BlockFetch-client-test to master July 6, 2022 16:01
@amesgen amesgen force-pushed the amesgen/CAD-4195-tentative-promptness branch 2 times, most recently from 3e0dac6 to 3f1b16b Compare July 21, 2022 08:35
@amesgen amesgen force-pushed the amesgen/CAD-4195-tentative-promptness branch from 3f1b16b to 0c6b2b7 Compare October 18, 2022 16:01
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

I found some typos in the comments, and made a few comment/layout suggestions. Approved! Really nice. As you say, there's a good bit of ceremony in the setup, but it seems minimal and tenable.


instance Arbitrary FollowerPromptnessTestSetup where
arbitrary = do
chainUpdates <- genChainUpdates TentativeChainBehavior securityParam 20
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any properties of genChainUpdates that we're assuming here?

I just noticed it doesn't have a comment on it :(

EG, at a glance, it looks like it can generate SwitchFork n bs where length bs == n, but I didn't see anything ensuring that in that case the last bs is preferred to the youngest rolled back block. Thus these ChainUpdates do not necessarily monotonically improve the chain tip 🤔

That might not matter to this test, but perhaps a one line comment here clarifying the extent to which it does or doesn't would be worthwhile.

Copy link
Member Author

@amesgen amesgen Nov 2, 2022

Choose a reason for hiding this comment

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

Yes, it is not guaranteed that every chain update will result in a tentative header, but this is not important for/orthogonal to this test, as we still mainly get pipelined blocks. I am adding a comment stressing this 👍

Copy link
Contributor

@nfrisby nfrisby Nov 2, 2022

Choose a reason for hiding this comment

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

Discussed on a call.

  • Esgen pointed out that this TestBlock(With) uses the Bft protocol, which has no tie-breaker. And so length bs == n will never cause a switch (with no tiebreaker, the behavior is just to keep the chain you already have).

  • We'll open a ticket to enrich other tentative header tests specifically with interesting tiebreaker behaviors. But for this promptness test, it's a bit off-topic: AddBlock will trigger the tentative pathway.

@amesgen amesgen force-pushed the amesgen/CAD-4195-tentative-promptness branch from 0c6b2b7 to 2cb9db2 Compare November 2, 2022 10:55
@amesgen
Copy link
Member Author

amesgen commented Nov 2, 2022

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 2, 2022

@iohk-bors iohk-bors bot merged commit 968183b into master Nov 2, 2022
@iohk-bors iohk-bors bot deleted the amesgen/CAD-4195-tentative-promptness branch November 2, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants