Skip to content

Comments

Churn test#5303

Open
the-headless-ghost wants to merge 1 commit intomainfrom
edgr/churn-test
Open

Churn test#5303
the-headless-ghost wants to merge 1 commit intomainfrom
edgr/churn-test

Conversation

@the-headless-ghost
Copy link
Member

@the-headless-ghost the-headless-ghost commented Feb 5, 2026

Description

Closes #5291

Checklist

Quality

  • Commit sequence makes sense and have useful messages, see ref.
  • New tests are added and existing tests are updated.
  • Self-reviewed the PR.

Maintenance

  • Linked an issue or added the PR to the current sprint of ouroboros-network project.
  • Added labels.
  • Updated changelog files.
  • The documentation has been properly updated, see ref.

@github-project-automation github-project-automation bot moved this to In Progress in Ouroboros Network Feb 5, 2026
@the-headless-ghost the-headless-ghost added the churn Issues / PRs related to churn label Feb 5, 2026
@the-headless-ghost the-headless-ghost self-assigned this Feb 5, 2026
@the-headless-ghost the-headless-ghost marked this pull request as ready for review February 6, 2026 08:38
@the-headless-ghost the-headless-ghost requested a review from a team as a code owner February 6, 2026 08:38
@crocodile-dentist
Copy link
Contributor

I wonder if we can make it more uniform and like some other tests because I don't think we need an eventMap or targetsChangeEventMap and do all this extra processing which makes it perhaps more difficult to follow that it is. So the events binding should have the type [Events (WithName NtNAddr DiffusionTestTrace)] (see eg. prop_diffusion_cm_valid_transitions) and we should have a value which is a Signal TraceTargetsChanged where it is just the target change for the node you happen to be checking. Then the test is just conjoin (\ev -> ... signalProperty ...) events. Take a look at how this is done in prop_only_bootstrap_peers_in_fallback_state and maybe adapt it here slightly.

Copy link
Collaborator

@coot coot left a comment

Choose a reason for hiding this comment

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

Very nice! LGTM

@coot
Copy link
Collaborator

coot commented Feb 11, 2026

@crocodile-dentist is right, we don't need to build the maps. It's much better to analyse the trace as a stream if possible.

@coot coot self-requested a review February 11, 2026 14:44
.&&. counterexample "known big ledger peers upper bound violation" (knownBigLedgers' <= knownBigLedgers)
.&&. counterexample "known big ledger peers lower bound violation" (knownBigLedgers' >= decrease knownBigLedgers)
.&&. counterexample "established upper bound violation" (establisheds' <= establisheds)
.&&. counterexample "established lower bound violation" (establisheds' >= decrease establisheds)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add a check that we decrease established peers by at least as how many active peers we have decreased, and analogously for big ledger peers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean this: establisheds - establisheds' >= actives - actives'?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the logic we are trying to test is this in Churn:

    decreaseWithMin :: Int -> Int -> Int
    decreaseWithMin u v =  max 0 $ v - max u (max 1 (v `div` 5))

So we want to take the difference between established and active, and lower it by 20% or 1, whichever is greater, but also at least as many as active peers we have churned.
@karknu @coot should this be min v $ max u (max 1 (v div 5)) - this should catch some corner cases where we churn a few active peers, but the node is configured to use very few established peers in excess of the active target. This change will churn out all excess established peers even if the number of them is less than how many active peers we have churned. Or should this be an invariant violation that we check in the sane targets function - ie. it should be a misconfiguration where the number of excess established peers is less than the fraction of active peers churned at each cycle?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess you mean decreaseWithMin u v = max 0 $ v - min v $ max u (max 1 (v div 5)), isn't it?

Copy link
Contributor

@crocodile-dentist crocodile-dentist Feb 19, 2026

Choose a reason for hiding this comment

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

It's starting to get unwieldy, but what about v - min v (max u (max 1 (v div 5)))?

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

Labels

churn Issues / PRs related to churn

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Simulation tests for churn

3 participants