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

ChainSync client test: decrease number of tests without any sent headers #529

Closed
amesgen opened this issue Nov 29, 2023 · 0 comments · Fixed by #1201
Closed

ChainSync client test: decrease number of tests without any sent headers #529

amesgen opened this issue Nov 29, 2023 · 0 comments · Fixed by #1201
Assignees
@fraser-iohk

Comments

@amesgen
Copy link
Member

amesgen commented Nov 29, 2023 •
edited
Loading

Upon inspection, I was surpsised that ~70% of the cases had SomeNotEarly_SomeEarly Zero Zero, which implies the test never involved downloading a header!

My initial guess is that the client and server chain are diverging before they ever reach startTick.

I suspect we want to tune the generator to visit that less often. But that can be a separate Issue.

Originally posted by @nfrisby in #525 (comment)

Yeah, that seems be the case, when always setting startTick to 1, the Zero Zero case shrinks to ~9% of all cases.
@amesgen amesgen moved this to 🔖 Ready in Consensus Team Backlog Nov 29, 2023
@amesgen amesgen mentioned this issue Nov 29, 2023
Merged
@fraser-iohk fraser-iohk self-assigned this Jul 29, 2024
@fraser-iohk fraser-iohk moved this to 🏗 In progress in Consensus Team Backlog Jul 29, 2024
@amesgen amesgen mentioned this issue Aug 2, 2024
Merged
@fraser-iohk fraser-iohk moved this from 🏗 In progress to 👀 In review in Consensus Team Backlog Aug 8, 2024
@fraser-iohk fraser-iohk moved this from 👀 In review to ✅ Done in Consensus Team Backlog Aug 19, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 19, 2024
@fraser-iohk
reduce the frequency of client and server diverging before they reach…
a718db0 
… startTick (#1201)

This PR changes the generator for `ChainSyncClientSetup` such that
`startTick` is now heavily weighted to pick a lower value (closer to 1)
rather than a linear range between 1 and `maxStartTick`. This causes
fewer randomly-generated `ChainSyncClientSetup`s to produce test runs in
which no headers are sent due to immediate divergence.

Before:

```
ouroboros-consensus
  ChainSyncClient
    chainSync: OK (5.69s)
      +++ OK, passed 10000 tests:
      67.35% ForkTooDeep
      27.89% NoMoreIntersection
       0.25% RolledBackPastIntersection

      TickArrivalTimeStats (10000 in total):
      67.40% OnlyNotEarly_SomeEarly Zero Zero
      10.76% OnlyNotEarly_SomeEarly Zero Many
       6.72% OnlyNotEarly_SomeEarly One Many
       5.95% OnlyNotEarly_SomeEarly One One
       3.75% OnlyNotEarly_SomeEarly Many Zero
       2.89% OnlyNotEarly_SomeEarly One Zero
       1.38% OnlyNotEarly_SomeEarly Many Many
       1.07% OnlyNotEarly_SomeEarly Many One
       0.08% OnlyNotEarly_SomeEarly Zero One
```

After:
```
ouroboros-consensus
  ChainSyncClient
    chainSync: OK (8.51s)
      +++ OK, passed 10000 tests:
      84.58% NoMoreIntersection
      10.51% ForkTooDeep
       0.62% RolledBackPastIntersection

      TickArrivalTimeStats (10000 in total):
      19.50% OnlyNotEarly_SomeEarly One Many
      16.81% OnlyNotEarly_SomeEarly One Zero
      15.85% OnlyNotEarly_SomeEarly Zero Many
      13.83% OnlyNotEarly_SomeEarly One One
      13.25% OnlyNotEarly_SomeEarly Many Zero
      10.58% OnlyNotEarly_SomeEarly Zero Zero
       5.39% OnlyNotEarly_SomeEarly Many Many
       4.79% OnlyNotEarly_SomeEarly Many One

```

Note that the `NoMoreIntersection` case is now significantly more common
than the `ForkTooDeep` case, rather than the inverse.

Closes #529
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fraser-iohk fraser-iohk

Labels
None yet
Projects
Consensus Team Backlog
Status: ✅ Done
Milestone
No milestone
2 participants
@amesgen @fraser-iohk