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

reduce the frequency of client and server diverging before they reach startTick #1201

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

fraser-iohk
Copy link
Contributor

@fraser-iohk fraser-iohk commented Aug 1, 2024

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 ChainSyncClientSetups 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

Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

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

Can confirm that this achives the goal of #529:

Before #1201

      TickArrivalTimeStats (10000 in total):
      69.09% OnlyNotEarly_SomeEarly Zero Zero
      10.61% OnlyNotEarly_SomeEarly Zero Many
       5.89% OnlyNotEarly_SomeEarly One Many
       5.52% OnlyNotEarly_SomeEarly One One
       3.82% OnlyNotEarly_SomeEarly Many Zero
       2.70% OnlyNotEarly_SomeEarly One Zero
       1.26% OnlyNotEarly_SomeEarly Many Many
       1.01% OnlyNotEarly_SomeEarly Many One
       0.10% OnlyNotEarly_SomeEarly Zero One

With #1201

      TickArrivalTimeStats (10000 in total):
      18.16% OnlyNotEarly_SomeEarly One Many
      16.79% OnlyNotEarly_SomeEarly Zero Zero
      15.15% OnlyNotEarly_SomeEarly Zero Many
      14.41% OnlyNotEarly_SomeEarly One Zero
      13.38% OnlyNotEarly_SomeEarly One One
      12.87% OnlyNotEarly_SomeEarly Many Zero
       4.72% OnlyNotEarly_SomeEarly Many Many
       4.52% OnlyNotEarly_SomeEarly Many One

While better, the Zero Zero case still occurs ~17% of the time, so I guess startTick could be even smaller on average.

@fraser-iohk fraser-iohk force-pushed the fraser-iohk/reduce-chainsync-gen-divergence branch from f7dce79 to 644f2a3 Compare August 6, 2024 12:35
@fraser-iohk fraser-iohk marked this pull request as ready for review August 8, 2024 11:28
@fraser-iohk fraser-iohk force-pushed the fraser-iohk/reduce-chainsync-gen-divergence branch from 644f2a3 to 9ecb854 Compare August 8, 2024 11:28
Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

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

LGTM

Needs eg an empty force push to make the changelog CI check go green after Javier added the no changelog label (we should at some point try to make this check be triggered by label changes).

… startTick

the generator for startTick is now heavily weighted to pick a lower value
@fraser-iohk fraser-iohk force-pushed the fraser-iohk/reduce-chainsync-gen-divergence branch from 9ecb854 to 365aba2 Compare August 15, 2024 10:58
@fraser-iohk fraser-iohk added this pull request to the merge queue Aug 19, 2024
Merged via the queue into main with commit a718db0 Aug 19, 2024
17 checks passed
@fraser-iohk fraser-iohk deleted the fraser-iohk/reduce-chainsync-gen-divergence branch August 19, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ChainSync client test: decrease number of tests without any sent headers
3 participants