[sui system] implementing SIP-39#19836
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
| } | ||
|
|
||
| /// return (min, low, very low voting power) thresholds | ||
| fun get_voting_power_thresholds(ctx: &TxContext): (u64, u64, u64) { |
There was a problem hiding this comment.
@lxfind, @emmazzz told me it would be ok/preferable to use module constants for these instead of SuiSystemStateV2. So the way I handled this is marking the old stake-based constants in SuiSystemV2 as deprecated, and using this as the source of truth for the new voting-power based constants. Let me know if that doesn't make sense, and if there's anything should change in the Rust version of SuiSystemV2.
There was a problem hiding this comment.
Using module constants is fine however I don't see a way for these to be different across different networks.
Since you are not adding any new parameters, I guess we could technically rename the existing parameter fields and repurpose them.
But in order to update them differently for different networks, we might have to introduce a separate private entry function called update_parameters, which are then called at epoch change based on the protocol configs?
There was a problem hiding this comment.
Do we need different values for these in different networks?
| #[test] | ||
| fun test_low_stake_departure() { | ||
| #[expected_failure(abort_code = validator_set::ENotAValidator)] | ||
| fun test_request_add_then_pull_stake() { |
There was a problem hiding this comment.
@emmazzz I added a test for this (didn't see one previously), and wasn't sure what we would expect the outcome to be. A failure in request_withdraw_stake (what happens) seems like a reasonable answer, but was wondering if we should use a more descriptive error code than ENotAValidator. The specific problem here is (I think) that the validator is pending addition, and the stake of a pending validator cannot be withdrawn until the next epoch (when the validator will have been added)
|
|
||
| /// return (min, low, very low voting power) thresholds | ||
| fun get_voting_power_thresholds(ctx: &TxContext): (u64, u64, u64) { | ||
| // TODO: adjust as needed depending on when this will ship. and how will we deal with the devnet/testnet/mainnet differences? |
There was a problem hiding this comment.
probably best to use protocol version for this maybe?
There was a problem hiding this comment.
I looked into this, but I think it actually doesn't work unless we assume that upgrades happen every 2 weeks and every upgrade is a new protocol version. And since we'd have to hardcode the start protocol version anyway, we might as well encode the start epoch and keep it simple.
The good thing is that start_epoch can be set as conservatively as we want--e.g., if we anticipated this framework upgrade shipping on 10/30, there'd be no problem with setting the start_epoch as 11/30
There was a problem hiding this comment.
I more meant that once this code lands and has been upgraded on a network we can immediately start using it the next epoch. Hard coding an epoch number likely will not work since we have different networks at different epochs
There was a problem hiding this comment.
I am really unclear how this is ever going to work across networks if not with a native function that exposes the concept somehow. Can someone share the idea here?
Also I am not clear in which networks this code matters, all of them including local ones?
There was a problem hiding this comment.
IIUC the nice Manos suggestion below will fix the X-network issue
36fb04b to
8bd687b
Compare
|
please let's make sure that @damirka and/or @manolisliolios have reviewed and accepted this before it goes in |
crates/sui-framework/packages/sui-system/sources/validator_set.move
Outdated
Show resolved
Hide resolved
crates/sui-framework/packages/sui-system/sources/validator_set.move
Outdated
Show resolved
Hide resolved
crates/sui-framework/packages/sui-system/sources/sui_system_state_inner.move
Outdated
Show resolved
Hide resolved
|
|
||
| /// return (min, low, very low voting power) thresholds | ||
| fun get_voting_power_thresholds(ctx: &TxContext): (u64, u64, u64) { | ||
| // TODO: adjust as needed depending on when this will ship. and how will we deal with the devnet/testnet/mainnet differences? |
There was a problem hiding this comment.
I am really unclear how this is ever going to work across networks if not with a native function that exposes the concept somehow. Can someone share the idea here?
Also I am not clear in which networks this code matters, all of them including local ones?
| self: &mut SuiSystemStateInnerV2, | ||
| ctx: &TxContext, | ||
| ) { | ||
| assert!( |
There was a problem hiding this comment.
Do we need to worry about the vector size growing more than we are ok with? Either on object size limits, or execution limits?
There was a problem hiding this comment.
In theory, yes. To be defensive, perhaps what we should do is add a new threshold based on max object size rather than validator count?
There was a problem hiding this comment.
I looked into this a bit:
- If adding a validator would hit object size limits, the transaction will abort with that error. That's a bit less nice than aborting with
ELimitExceeded, but the effect will be the same, and it's hard to calculate the exact limit on validator count that the object size limits imply (since it depends on lots of variables like metadata size) - On execution limits:
advance_epochruns with metering off (I double-checked this by adding an infinite loop inside it and seeing tests that advance epochs diverge). So if the validator set is very big, advance epoch may take longer than expected, but it won't run into gas limits.
There was a problem hiding this comment.
Thanks @sblackshear
- Yup I can imagine it's not trivial to calculate this and also the effects would be bad regardless if we were catching it on advance_epoch. We would need to enforce it before adding a validator to the pending set to be safe.
- Ah, that's great! That was my major concern on the increased size of the vector.
8bd687b to
7c06bda
Compare
crates/sui-framework/packages/sui-system/sources/validator_set.move
Outdated
Show resolved
Hide resolved
| // prior to this line, `self.total_stake` is the previous epoch's total stake. we want to update it to the current total stake | ||
| // before processing low stake departures, otherwise the `total_stake` denominator in our voting power calculations will be incorrect. | ||
| // note: if a validator is kicked out in the loop below, `total_stake` will be decreased by `process_validator_departure`. otherwise, | ||
| // `total_stake` will be unchanged by the end of the function |
There was a problem hiding this comment.
I noticed that in the previous implementation, we first triggered the processing of departures, then set the total_stake based on the "remaining" active set of validators (from scratch).
Here, we calculate it and set it before we process the departing validators, then allow the departure fn to adjust it.
Are we sure this cannot cause any side-effect issues in any way? I do see that on the codepath, we do deduct the total_stake when we actually process a validator departure, however I thought I'd triple-check / validate my assumptins!
There was a problem hiding this comment.
I read through this and convinced myself that this way is indeed correct. But this made me realize that the statefulness of total_stake makes this code very hard to reason about indeed... I did some light refactoring to only write to total_stake in one location, and make sure that all logic in validator_set and voting_power is using the same value (e.g., there were separate functions in each module for computing total stake before). PTAL and let me know if you also feel more confident with the new setup!
There was a problem hiding this comment.
Thanks Sam. I think the change indeed makes it easier to follow! The consistency we get on voting_power is a great deal.
7c06bda to
713b7f8
Compare
713b7f8 to
2f8d357
Compare
|
|
||
| // If the pool is inactive, we immediately process the withdrawal. | ||
| if (is_inactive(pool)) process_pending_stake_withdraw(pool); | ||
| if (is_inactive(pool) || pool.is_preactive()) { |
There was a problem hiding this comment.
since we touch the line 😅
| if (is_inactive(pool) || pool.is_preactive()) { | |
| if (pool.is_inactive() || pool.is_preactive()) { |
I'll defer to @emmazzz to validate the correctness of this. It feels OK, but this indeed could open side-effects that are not obvious here.
There was a problem hiding this comment.
I think this change is okay, as long as we have added a test for it. When we deposit stake into a preactive pool, we process the deposit immediately so I'm surprised that we don't do this already for stake withdraws.
crates/sui-framework/packages/sui-system/sources/sui_system_state_inner.move
Show resolved
Hide resolved
|
|
||
| /// Key for the `extra_fields` bag to store the start epoch of allowing admission | ||
| /// of new validators based on a minimum voting power rather than a minimum stake. | ||
| public struct VotingPowerAdmissionStartEpochKey has copy, drop, store() |
There was a problem hiding this comment.
nit: for positional structs we usually prefer
| public struct VotingPowerAdmissionStartEpochKey has copy, drop, store() | |
| public struct VotingPowerAdmissionStartEpochKey() has copy, drop, store; |
crates/sui-framework/packages/sui-system/tests/validator_set_tests.move
Outdated
Show resolved
Hide resolved
| vector[hint], | ||
| vector[hint], | ||
| vector[hint], | ||
| copy name, |
There was a problem hiding this comment.
I don't think copy is needed here. 😕
| } | ||
|
|
||
| /// Returns true if `addr` is an active validator | ||
| public fun is_active_validator(self: &ValidatorSet, addr: address): bool { |
There was a problem hiding this comment.
I believe this function should not be public.
2f8d357 to
8fcbda9
Compare
8fcbda9 to
be1c664
Compare
be1c664 to
5635906
Compare
Implement the new validator admission scheme described in SIP-39 that uses voting power thresholds rather than stake thresholds. Conceptual details are in SIP-39, but here's what changes at the code level: - Deprecate the min_validator_joining_stake, validator_low_stake_threshold, validator_very_low_stake_threshold, and max_validator_count constants - Use voting power-based module constants defined in `validator_set::get_voting_power_thresholds` asn the new source of truth for (min, low, very low) voting power - Change low stake departure logic to use these voting power thresholds instead of stake thresholds - Add tests for each of the relevant scenarios: admission with appropriate voting power, failure to admit without appropriate voting power, dismissal if very low stake threshold is hit, eventual dismissal or recovery if low stake threshold is hit
5635906 to
891913a
Compare
## Description - Implements SIP-39, replaces #19836 - Closes DVX-685 ## Test plan - Features tests for new functions. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [x] Protocol: Implements SIP-39 in Sui System - [ ] Nodes (Validators and Full nodes): - [ ] gRPC: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: --------- Co-authored-by: Emma Zhong <emma@mystenlabs.com>
|
Closing in favour of #21303 |
Implement the new validator admission scheme described in SIP-39 that uses voting power thresholds rather than stake thresholds. Conceptual details are in SIP-39, but here's what changes at the code level:
validator_set::get_voting_power_thresholdsasn the new source of truth for (min, low, very low) voting power