[sui system] implementing SIP-39 #19836

sblackshear · 2024-10-12T23:24:14Z

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 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

vercel · 2024-10-12T23:24:21Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 29, 2025 9:04pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Jan 29, 2025 9:04pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Jan 29, 2025 9:04pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Jan 29, 2025 9:04pm

sblackshear · 2024-10-12T23:27:30Z

crates/sui-framework/packages/sui-system/sources/validator_set.move

+    }
+
+    /// return (min, low, very low voting power) thresholds
+    fun get_voting_power_thresholds(ctx: &TxContext): (u64, u64, u64) {


@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.

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?

Do we need different values for these in different networks?

sblackshear · 2024-10-12T23:30:15Z

crates/sui-framework/packages/sui-system/tests/validator_set_tests.move

@@ -313,90 +300,171 @@ module sui_system::validator_set_tests {
    }

    #[test]
-    fun test_low_stake_departure() {
+    #[expected_failure(abort_code = validator_set::ENotAValidator)]
+    fun test_request_add_then_pull_stake() {


@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)

bmwill · 2024-10-12T23:36:57Z

crates/sui-framework/packages/sui-system/sources/validator_set.move

+
+    /// 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?


probably best to use protocol version for this maybe?

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

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

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?

IIUC the nice Manos suggestion below will fix the X-network issue

dariorussi · 2024-12-10T13:36:22Z

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

crates/sui-framework/packages/sui-system/sources/sui_system_state_inner.move

dariorussi · 2024-12-20T14:45:35Z

crates/sui-framework/packages/sui-system/sources/validator_set.move

+
+    /// 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?


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?

manolisliolios · 2024-12-20T15:58:47Z

crates/sui-framework/packages/sui-system/sources/sui_system_state_inner.move

@@ -404,12 +412,7 @@ module sui_system::sui_system_state_inner {
        self: &mut SuiSystemStateInnerV2,
        ctx: &TxContext,
    ) {
-        assert!(


Do we need to worry about the vector size growing more than we are ok with? Either on object size limits, or execution limits?

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?

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_epoch runs 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.

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.

crates/sui-framework/packages/sui-system/sources/validator_set.move

manolisliolios · 2024-12-23T15:21:30Z

crates/sui-framework/packages/sui-system/sources/validator_set.move

+        // 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


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!

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!

Thanks Sam. I think the change indeed makes it easier to follow! The consistency we get on voting_power is a great deal.

manolisliolios · 2024-12-25T09:32:11Z

crates/sui-framework/packages/sui-system/sources/staking_pool.move

@@ -177,7 +177,9 @@ module sui_system::staking_pool {
        pool.pending_pool_token_withdraw = pool.pending_pool_token_withdraw + pool_token_withdraw_amount;

        // 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()) {


since we touch the line 😅

Suggested change

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.

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

damirka · 2025-01-22T10:17:31Z

crates/sui-framework/packages/sui-system/sources/validator_set.move

@@ -105,6 +105,10 @@ module sui_system::validator_set {
        is_voluntary: bool,
    }

+    /// 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()


nit: for positional structs we usually prefer

Suggested change

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

damirka · 2025-01-22T10:33:45Z

crates/sui-framework/packages/sui-system/tests/validator_set_tests.move

+            vector[hint],
+            vector[hint],
+            vector[hint],
+            copy name,


I don't think copy is needed here. 😕

damirka · 2025-01-24T16:00:54Z

crates/sui-framework/packages/sui-system/sources/validator_set.move

@@ -1287,11 +1352,21 @@ module sui_system::validator_set {
        self.validator_candidates.contains(addr)
    }

+    /// Returns true if `addr` is an active validator
+    public fun is_active_validator(self: &ValidatorSet, addr: address): bool {


I believe this function should not be public.

crates/sui-framework/packages/sui-framework/sources/tx_context.move

Re-reviewing latest changes!

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

sblackshear requested review from lxfind, tzakian and emmazzz October 12, 2024 23:24

sblackshear commented Oct 12, 2024

View reviewed changes

sblackshear mentioned this pull request Oct 12, 2024

SIP-39: Lowering the validator set barrier to entry sui-foundation/sips#39

Merged

bmwill reviewed Oct 12, 2024

View reviewed changes

dariorussi requested a review from longbowlu November 17, 2024 14:36

sblackshear force-pushed the stake_change branch from 36fb04b to 8bd687b Compare December 8, 2024 23:58

sblackshear had a problem deploying to sui-typescript-aws-kms-test-env December 8, 2024 23:58 — with GitHub Actions Failure

vercel bot deployed to Preview – sui-docs December 9, 2024 00:00 View deployment

dariorussi requested review from damirka and manolisliolios December 10, 2024 13:35

manolisliolios reviewed Dec 13, 2024

View reviewed changes

crates/sui-framework/packages/sui-system/sources/validator_set.move Outdated Show resolved Hide resolved

manolisliolios reviewed Dec 13, 2024

View reviewed changes

crates/sui-framework/packages/sui-system/sources/validator_set.move Outdated Show resolved Hide resolved

dariorussi reviewed Dec 20, 2024

View reviewed changes

manolisliolios reviewed Dec 20, 2024

View reviewed changes

sblackshear force-pushed the stake_change branch from 8bd687b to 7c06bda Compare December 23, 2024 01:49

sblackshear temporarily deployed to sui-typescript-aws-kms-test-env December 23, 2024 01:49 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 23, 2024 01:50 View deployment

manolisliolios reviewed Dec 23, 2024

View reviewed changes

crates/sui-framework/packages/sui-system/sources/validator_set.move Outdated Show resolved Hide resolved

manolisliolios reviewed Dec 23, 2024

View reviewed changes

sblackshear force-pushed the stake_change branch from 7c06bda to 713b7f8 Compare December 24, 2024 03:46

sblackshear temporarily deployed to sui-typescript-aws-kms-test-env December 24, 2024 03:46 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 24, 2024 03:48 View deployment

manolisliolios previously approved these changes Dec 24, 2024

View reviewed changes

sblackshear force-pushed the stake_change branch from 713b7f8 to 2f8d357 Compare December 25, 2024 01:47

sblackshear temporarily deployed to sui-typescript-aws-kms-test-env December 25, 2024 01:47 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 25, 2024 01:49 View deployment

manolisliolios self-requested a review December 25, 2024 09:02

manolisliolios reviewed Dec 25, 2024

View reviewed changes

damirka reviewed Jan 22, 2025

View reviewed changes

crates/sui-framework/packages/sui-system/sources/sui_system_state_inner.move Show resolved Hide resolved

damirka reviewed Jan 22, 2025

View reviewed changes

crates/sui-framework/packages/sui-system/tests/validator_set_tests.move Outdated Show resolved Hide resolved

damirka reviewed Jan 22, 2025

View reviewed changes

damirka reviewed Jan 24, 2025

View reviewed changes

rjs-mysten reviewed Jan 24, 2025

View reviewed changes

crates/sui-framework/packages/sui-framework/sources/tx_context.move Show resolved Hide resolved

sblackshear force-pushed the stake_change branch from 2f8d357 to 8fcbda9 Compare January 24, 2025 21:22

sblackshear temporarily deployed to sui-typescript-aws-kms-test-env January 24, 2025 21:22 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 24, 2025 21:26 View deployment

sblackshear force-pushed the stake_change branch from 8fcbda9 to be1c664 Compare January 27, 2025 17:55

sblackshear requested a review from ronny-mysten as a code owner January 27, 2025 17:55

sblackshear temporarily deployed to sui-typescript-aws-kms-test-env January 27, 2025 17:55 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 27, 2025 17:57 View deployment

sblackshear force-pushed the stake_change branch from be1c664 to 5635906 Compare January 28, 2025 01:23

sblackshear temporarily deployed to sui-typescript-aws-kms-test-env January 28, 2025 01:23 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 28, 2025 01:25 View deployment

sblackshear force-pushed the stake_change branch from 5635906 to 891913a Compare January 29, 2025 21:02

sblackshear temporarily deployed to sui-typescript-aws-kms-test-env January 29, 2025 21:02 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 29, 2025 21:04 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sui system] implementing SIP-39 #19836

[sui system] implementing SIP-39 #19836

sblackshear commented Oct 12, 2024 •

edited

Loading

vercel bot commented Oct 12, 2024 •

edited

Loading

sblackshear Oct 12, 2024

lxfind Oct 15, 2024

sblackshear Oct 22, 2024

sblackshear Oct 12, 2024

bmwill Oct 12, 2024

sblackshear Oct 22, 2024

bmwill Oct 23, 2024

dariorussi Dec 20, 2024

sblackshear Dec 20, 2024

dariorussi commented Dec 10, 2024

dariorussi Dec 20, 2024

manolisliolios Dec 20, 2024 •

edited

Loading

sblackshear Dec 20, 2024 •

edited

Loading

sblackshear Dec 23, 2024 •

edited

Loading

manolisliolios Dec 23, 2024

manolisliolios Dec 23, 2024 •

edited

Loading

sblackshear Dec 25, 2024 •

edited

Loading

manolisliolios Dec 25, 2024

manolisliolios Dec 25, 2024 •

edited

Loading

emmazzz Jan 28, 2025

damirka Jan 22, 2025

damirka Jan 22, 2025

damirka Jan 24, 2025

	if (is_inactive(pool) \|\| pool.is_preactive()) {
	if (pool.is_inactive() \|\| pool.is_preactive()) {

	public struct VotingPowerAdmissionStartEpochKey has copy, drop, store()
	public struct VotingPowerAdmissionStartEpochKey() has copy, drop, store;

[sui system] implementing SIP-39 #19836

Are you sure you want to change the base?

[sui system] implementing SIP-39 #19836

Conversation

sblackshear commented Oct 12, 2024 • edited Loading

vercel bot commented Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariorussi commented Dec 10, 2024

Choose a reason for hiding this comment

manolisliolios Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

sblackshear Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

sblackshear Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manolisliolios Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

sblackshear Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manolisliolios Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sblackshear commented Oct 12, 2024 •

edited

Loading

vercel bot commented Oct 12, 2024 •

edited

Loading

manolisliolios Dec 20, 2024 •

edited

Loading

sblackshear Dec 20, 2024 •

edited

Loading

sblackshear Dec 23, 2024 •

edited

Loading

manolisliolios Dec 23, 2024 •

edited

Loading

sblackshear Dec 25, 2024 •

edited

Loading

manolisliolios Dec 25, 2024 •

edited

Loading