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

Etcm 8983 fix get block author test #361

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

rsporny
Copy link
Collaborator

@rsporny rsporny commented Jan 9, 2025

Description

All in commit msg.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages.
  • New tests are added if needed and existing tests are updated.
  • Relevant logging and metrics added
  • CI passes. See note on CI.
  • Any changes are noted in the changelog.md for affected crate
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG Partner Chains developers to do this
for you.

@rsporny rsporny added the E2E-tests E2E Tests for SDET review/approval label Jan 9, 2025
@rsporny rsporny force-pushed the ETCM-8983-fix-get-block-author-test branch from 7f82a2f to 637d0ed Compare January 9, 2025 12:57
Copy link
Contributor

@chrispalaskas chrispalaskas left a comment

Choose a reason for hiding this comment

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

Looks much nicer!! Please make sure it covers all corner cases though, epoch boundaries, first validator of an epoch missing, last validator of an epoch missing etc

Copy link
Contributor

@mpskowron mpskowron left a comment

Choose a reason for hiding this comment

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

Unfortunately, without signature verification, you can never be sure if the block was authored by the claimed author. I cannot find that part in the code anymore.

@rsporny rsporny force-pushed the ETCM-8983-fix-get-block-author-test branch from d45ae48 to 793b17b Compare January 9, 2025 16:04
@rsporny
Copy link
Collaborator Author

rsporny commented Jan 9, 2025

@chrispalaskas I've run it for multiple mc epochs, we should be good.

@mpskowron the test verifies if block author is in the committee. Extracting block signature was just a way to verify it, but your point is good, we lost that additional check now. @chrispalaskas, since we now have simpler method to get block author, we may utilize it in a new test that would validate if the claimed author really signed a block. Although, that might be slightly excessive... we would be testing aura's round robin essentially.

@mpskowron
Copy link
Contributor

@mpskowron the test verifies if block author is in the committee. Extracting block signature was just a way to verify it, but your point is good, we lost that additional check now. @chrispalaskas, since we now have simpler method to get block author, we may utilize it in a new test that would validate if the claimed author really signed a block. Although, that might be slightly excessive... we would be testing aura's round robin essentially.

Not only aura's round robin but also the signing process and if slots are claimed correctly, etc. Anyway, the solution looks correct.

fixes:
- test_block_authors_match_committee_seats was failing in some cases which wasn't handled properly, e.g. updated DParam or runtime upgrade. Now it's getting the author based on slot number and modulo operation.

Refs: ETCM-8983
@rsporny rsporny force-pushed the ETCM-8983-fix-get-block-author-test branch from 793b17b to 2b1ab26 Compare January 10, 2025 16:38
@rsporny rsporny merged commit 20decaf into master Jan 10, 2025
14 checks passed
@rsporny rsporny deleted the ETCM-8983-fix-get-block-author-test branch January 10, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2E-tests E2E Tests for SDET review/approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants