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

Update Staker requirements and BatchHeader::MAX_CERTIFICATES #2386

Merged
merged 16 commits into from Mar 16, 2024

Conversation

raychu86
Copy link
Collaborator

@raychu86 raychu86 commented Mar 9, 2024

Motivation

This PR updates the minimum validator/delegator stakes and also the BatchHeader::MAX_CERTIFICATES:

MIN_VALIDATOR_STAKE: 1_000_000 credits -> 10_000_000 credits
MIN_DELEGATOR_STAKE: 10 credits -> 10_000 credits
MAX_CERTIFICATES: 200 -> 10

Note that Committee::MAX_COMMITTEE_SIZE and BatchCertificate::MAX_SIGNATURES will also be changed because they are equivalent to BatchHeader::MAX_CERTIFICATES.

@raychu86 raychu86 requested a review from d0cd March 9, 2024 01:25
@raychu86
Copy link
Collaborator Author

raychu86 commented Mar 9, 2024

@ljedrz @niklaslong @joske There seems to be an issue with how Committee::MAX_COMMITTEE_SIZE and BatchCertificate::MAX_SIGNATURES are being set in testing. They do not properly use the cfg(test) value of BatchHeader::MAX_CERTIFICATES when running the tests (they use 10 instead of 100).

Would you happen to have a cleaner way to do this rather than adding the cfg flags to both MAX_COMMITTEE_SIZE and MAX_SIGNATURES?

@raychu86 raychu86 marked this pull request as draft March 9, 2024 02:54
@ljedrz
Copy link
Contributor

ljedrz commented Mar 9, 2024

@raychu86 I usually prefer to introduce a test feature and then apply a #[cfg(feature = "test")] attribute where applicable; that way, as long as the crate is consistently imported as a dev-dependency with that feature enabled, it doesn't matter how the test is executed - the attribute applies.

edit: since I see that this is also in place, I would check if the crate in question is consistently imported as a dev-depencency with the test feature in the crates where the tests reside.

@raychu86 raychu86 marked this pull request as ready for review March 11, 2024 21:44
Copy link
Contributor

@d0cd d0cd left a comment

Choose a reason for hiding this comment

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

LGTM! Left a minor nit.

synthesizer/src/vm/helpers/committee.rs Outdated Show resolved Hide resolved
raychu86 and others added 6 commits March 15, 2024 13:29
Co-authored-by: d0cd <23022326+d0cd@users.noreply.github.com>
Signed-off-by: Raymond Chu <14917648+raychu86@users.noreply.github.com>
Signed-off-by: Raymond Chu <14917648+raychu86@users.noreply.github.com>
Signed-off-by: Raymond Chu <14917648+raychu86@users.noreply.github.com>
@howardwu howardwu merged commit 01c8b09 into mainnet-staging Mar 16, 2024
78 checks passed
@howardwu howardwu deleted the update/requirements branch March 16, 2024 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants