-
Notifications
You must be signed in to change notification settings - Fork 992
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 below-threshold validator set #1576
Conversation
396d5ce
to
06b5ee5
Compare
Several unit tests break because The more rigorous solution is to modify |
); | ||
return None; | ||
} | ||
// TODO: maybe debug_assert that the new stake is >= threshold? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to be careful with removing this check. We can do it, but we should probably enforce that the validator_stake_threshold
and tm_votes_per_token * u64::from(validator_stake_threshold)
are non-zero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with if prev_tm_voting_power == 0 ...
for below capacity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enforce with debug_asserts or by returning None
values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe smth like
if params.validator_stake_threshold == token::Amount::default() && *prev_tm_voting_power == 0 && *new_tm_voting_power == 0
{
tracing::info!(
"skipping validator update, {address} is in consensus \
set but without voting power"
);
return None;
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALSO, do we still need this Lazy
type wrapped around prev_tm_voting_power
and new_tm_voting_power
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could enforce in the paramaters PosParams::validate
that validator_stake_threshold
and tm_votes_per_token * u64::from(validator_stake_threshold)
are non-zero - only thing is that we're currently not using this function but we should probably fix that anyway. We could use it for genesis parameters validation and for on-chain parameters change, which is only allowed via governance.
If we enforce these invariants on the params then we can remove the check in here as we couldn't have a validator with 0 TM voting to enter the consensus set. That being said, what you've done LGTM and I don't think we need to change this immediately.
I think we should keep Lazy
as the first condition if matches!(prev_state, Some(ValidatorState::Consensus))
might short-circuit in which case we don't need to compute these.
77f8a5f
to
865d7ce
Compare
865d7ce
to
0df7872
Compare
pls update wasm |
I'll squash the fixup commits and add this to draft |
3100bfc
to
6926d60
Compare
* brent/add-below-threshold-validators: [ci] wasm checksums update changelog: add #1576 fix pos unit tests pos/tests: improve `arb_genesis_validators` to consider a stake threshold pos/lib: refactor `validator_set_update_tendermint` removing below-threshold validators from storage WIP fixing other pos unit tests fix wasm tx tests pos/tests: fix broken unit tests pos/lib: update `validator_set_update_tendermint` pos/state_machine: update test with new validator set pos/lib: fix `init_genesis` remove outdated comments pos/lib: update logic in `update_validator_set` beginning changes
9c488fb
to
6926d60
Compare
* origin/brent/add-below-threshold-validators: [ci] wasm checksums update changelog: add #1576 fix pos unit tests pos/tests: improve `arb_genesis_validators` to consider a stake threshold pos/lib: refactor `validator_set_update_tendermint` removing below-threshold validators from storage WIP fixing other pos unit tests fix wasm tx tests pos/tests: fix broken unit tests pos/lib: update `validator_set_update_tendermint` pos/state_machine: update test with new validator set pos/lib: fix `init_genesis` remove outdated comments pos/lib: update logic in `update_validator_set` beginning changes
Merged in v0.18.0. |
Based on v0.17.4.
Closes #1128.
This PR implements a third validator set in addition to the
consensus
andbelow-capacity
sets: thebelow-threshold
set. All validators with stake below thevalidator_stake_threshold
parameter are placed into this set, which is unordered, unlike the other two validator sets. Smaller amounts of data are needed to be kept in storage for validators in this set.