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

Validator - support partitioning of the slot id space #890

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

marcinczenko
Copy link

@marcinczenko marcinczenko commented Sep 2, 2024

This PR, addresses issue #457 (please refer to the issue discussion for more context info so that we do not have to duplicate it all here).

To shortly summarise issue #457: we want to be able to tell the validator what partition of the whole slot id space it should observe and help validating.

To achieve that, we provide two new CLI/config parameters:

  • validator-partition-size - to indicate number equal partitions dividing the whole slot id space,
  • validator-partition-index - to indicate the index of the portion observed by the validator.

The validator will only consider a slot with slot id slotId for validation if it matches the following criteria:

slotId % validatorPartitionSize == validatorPartitionIndex

We decided to keep the --validator-max-slots option to additionally limit the number of slots that will be observed by the validator for the chosen slot id partition.

To be done in this PR:

  • add new CLI/config options
  • add the corresponding partitioning params to the validation object,
  • incorporate portioning logic in the validator,
  • extend the validation tests to cover partitioning
  • refactor validation params into separate type for cleaner validation
  • updated relevant documentation (specifically where the cmd line params are mentioned).

@marcinczenko marcinczenko added the Marketplace See https://miro.com/app/board/uXjVNZ03E-c=/ for details label Sep 2, 2024
@marcinczenko marcinczenko self-assigned this Sep 2, 2024
@marcinczenko marcinczenko marked this pull request as draft September 2, 2024 23:14
@marcinczenko marcinczenko marked this pull request as ready for review September 13, 2024 17:22
Copy link
Contributor

@emizzle emizzle left a comment

Choose a reason for hiding this comment

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

Overall, great job! 👏

I have a few (detailed) comments, mostly because I had already gone through the mental hurdles of this task in the past, so I knew of some nuances.

codex/codex.nim Show resolved Hide resolved
codex/conf.nim Outdated Show resolved Hide resolved
codex/conf.nim Outdated Show resolved Hide resolved
codex/contracts/validation/validationparams.nim Outdated Show resolved Hide resolved
codex/validation.nim Outdated Show resolved Hide resolved
tests/codex/testvalidation.nim Show resolved Hide resolved
tests/codex/testvalidation.nim Show resolved Hide resolved
tests/codex/testvalidation.nim Show resolved Hide resolved
tests/codex/testvalidation.nim Outdated Show resolved Hide resolved
codex/conf.nim Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
codex/validation.nim Outdated Show resolved Hide resolved
Copy link
Contributor

@emizzle emizzle left a comment

Choose a reason for hiding this comment

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

Looking good, Marcin! I'll approve now as there is nothing blocking.

On a formatting note, it would be good if you could limit line length to 80 chars please 🙏

@marcinczenko
Copy link
Author

Looking good, Marcin! I'll approve now as there is nothing blocking.

On a formatting note, it would be good if you could limit line length to 80 chars please 🙏

Will be done.

README.md Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Marketplace See https://miro.com/app/board/uXjVNZ03E-c=/ for details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants