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

Add orbiter notify_inactive_collator tests #2683

Merged
merged 6 commits into from
Feb 28, 2024
Merged

Conversation

elfedy
Copy link
Contributor

@elfedy elfedy commented Feb 26, 2024

What does it do?

Adds a test flow that creates an orbiter pool and tests three things:

  1. An orbiter cannot be notified as offline no matter if it produced blocks or not.
  2. Marking an orbiter pool with an active orbiter (that is assigned to the current round) as inactive results in a noop when the pool has not produced blocks for MaxOfflineRounds
  3. Marking an orbiter pool without an active orbiter as inactive is successful when the pool has not produced blocks for MaxOfflineRounds

What important points reviewers should know?

Had to add MoonbeamOrbitersConfig to Moonbase's genesis config to set MinOrbiterDeposit. The only way to set this value is via genesis or a migration.

Is there something left for follow-up PRs?

  • Maybe a rust integration test where the paths on which the pool produces blocks are tested. There is no simple way to do this on typescript tests as alith will always be the block producer on dev mode.
  • I am investigating an edge case that affected this test (solved it by using a different collator for the pool) where a collator cannot be marked as offline even if it didn't produce blocks. Because it's AtStake entry gets deleted on pay_one_collator_reward.

@elfedy elfedy added B0-silent Changes should not be mentioned in any release notes D2-notlive PR doesn't change runtime code (so can't be audited) labels Feb 26, 2024
Copy link
Contributor

github-actions bot commented Feb 26, 2024

Coverage Report

@@                  Coverage Diff                   @@
##           master   elfedy-test-orbiter     +/-   ##
======================================================
  Coverage   73.68%                73.68%   0.00%     
  Files         232                   232             
  Lines       72753                 72753             
======================================================
  Hits        53603                 53603             
  Misses      19150                 19150             
Files Changed Coverage

Coverage generated Wed Feb 28 15:26:01 UTC 2024

@elfedy elfedy requested a review from timbrinded February 26, 2024 12:52
@elfedy elfedy force-pushed the elfedy-test-orbiter branch from a83920a to a8e3c25 Compare February 26, 2024 13:53
@RomarQ
Copy link
Contributor

RomarQ commented Feb 26, 2024

@elfedy can you also commit the script you used to regenerate the test identifiers?
It will give smaller diffs if someone needs to regenerate them in the future.

@elfedy
Copy link
Contributor Author

elfedy commented Feb 26, 2024

@elfedy can you also commit the script you used to regenerate the test identifiers? It will give smaller diffs if someone needs to regenerate them in the future.

@RomarQ it's the pnpm moonwall derive <suite_root_dir> command that was added to moonwall

@elfedy elfedy force-pushed the elfedy-test-orbiter branch from e89f7e0 to d03b5d5 Compare February 27, 2024 13:04
Copy link
Contributor

@Agusrodri Agusrodri left a comment

Choose a reason for hiding this comment

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

Looks good! Some small suggestions :)

@elfedy elfedy merged commit c0246d8 into master Feb 28, 2024
27 checks passed
@elfedy elfedy deleted the elfedy-test-orbiter branch February 28, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B0-silent Changes should not be mentioned in any release notes D2-notlive PR doesn't change runtime code (so can't be audited)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants