feat(proposer-set-reduction): Add proposer set state to x/staking by tqin7 · Pull Request #71 · dydxprotocol/cosmos-sdk

tqin7 · 2025-07-23T03:25:18Z

Description

Add new state to x/staking that keeps track of which validators are eligible proposers. Changes are around genesis state, keys, and store.

will add queries in a separate PR

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

github-actions · 2025-07-23T03:25:31Z

@tqin7 your pull request is missing a changelog!

northstar456 · 2025-07-25T00:54:16Z


 	DelegationByValIndexKey = []byte{0x71} // key for delegations by a validator
+
+	ProposerKeyPrefix = []byte("Proposer:") // prefix for proposers (only a subset of validators can propose blocks)


To save space, let's use P:. We could also use hex like above, which carries a risk of colliding with future staking keys

yeah went with a string to avoid future collision. changed this to P:

northstar456 · 2025-07-25T01:07:19Z


+	if len(data.Proposers) == 0 {
+		// If no proposers are specified, set all validators as eligible proposers
+		for _, validator := range data.Validators {


We should have a "default state" where all active validators can propose.

Right now even though we have this default pre-genesis, it gets translated to an explicit list of proposers at genesis. This means that new validators that join the active set do not get to propose, even though "proposer set filter is not turned on". This is also important for the network upgrade - instead of setting the explicit list at the time of upgrade, you can just use this default state.

Two example ways to do this:

If ProposerKeyPrefix store is empty, then "filter is off" and all active validators can propose

An additional boolean that defaults to false

good idea! will go with filter is off if ProposerKeyPrefix store is empty

northstar456 · 2025-07-25T01:11:32Z

+}
+
+// SetProposer sets a validator (given its operator address) as a block proposer.
+func (k Keeper) SetProposer(ctx context.Context, operatorAddrStr string) error {


For security: let's add an invariant checker anytime proposer state is modified, that there's at least one validator that's able to propose blocks. We don't want to get into some state where no one is able to propose.

This shouldn't be expensive since we expect to change this state very infrequently.

agreed. will check if given operator is a bonded validator

also thinking that in the gov msg that sets proposers, will check proposers list

See my comment here

northstar456 · 2025-07-25T01:34:14Z

+	for ; iterator.Valid(); iterator.Next() {
+		key := iterator.Key()
+		// Skip prefix and length prefix to get actual address bytes
+		addrBytesWithLen := key[len(types.ProposerKeyPrefix):]


This logic seems error prone - are we following any existing code pattern that does this? Did we consider using storetypes.KVStorePrefixIterator?

yeah. looked across cosmos-sdk and everywhere uses raw keys instead of a child store. and thus we need to parse these keys manually, which also is the pattern

however, i moved this address parsing logic to a helper function to make it clearer

northstar456 · 2025-07-25T01:39:23Z

+)
+
+// GetIsProposer returns whether a validator (given its operator address) is a block proposer.
+func (k Keeper) GetIsProposer(ctx context.Context, operatorAddrStr string) (bool, error) {


Similar to below, this should also return true in the default state (no proposer set filter is active)

northstar456 · 2025-07-25T01:42:45Z

+
+// SetProposer sets a validator (given its operator address) as a block proposer.
+func (k Keeper) SetProposer(ctx context.Context, operatorAddrStr string) error {
+	operatorAddr, err := k.validatorAddressCodec.StringToBytes(operatorAddrStr)


Does this return error if the input address is a valid but different form of address (eg consensus, delegator, etc)? Could we add test for this?

yeah it returns error for any form other than operator address because k.validatorAddressCodec only handles operator addresses. added tests for this

northstar456 · 2025-07-28T16:54:37Z

+	defer iterator.Close()
+
+	// Return true if not a single proposer is set
+	if !iterator.Valid() {


Are we sure this is the right condition? The method docstring simply says

// Valid returns whether the current iterator is valid. Once invalid, the Iterator remains // invalid forever. Valid() bool

Have we considered actually iterating throughe the iterator, keep a counter, and use counter = 0 as the condition?

northstar456 · 2025-07-28T17:02:17Z

+	if err != nil {
+		return err
+	}
+	if validator.Status != types.Bonded {


Downside of this approach

Gov vote starts to whitelist A, B, C as a proposer

A short while before vote passes, A goes through a temporary outage and gets jailed

One vote gets executed, the whole proposal fails since A is not bonded at that moment

This could be acceptable edge case. Possible alternative:

Change this method to private

Add a public interface SetProposers since the application never actually sets individual proposers, but just updates the list

At the end of public SetProposers, call a checkInvariant function which verifies the following: call GetAllProposers, and check that at least 1 or X of them is bonded. This may be a more holistic approach, and also covers the check you were thinking to add for the gov msg.

To confirm, once a validator gets jailed, it will start unbonding?

combining with your comment below, think that getting and setting the entire proposer list (instead of individual proposers) is sufficient and would cover these cases

northstar456 · 2025-07-28T17:08:24Z

+
+// GetIsProposer returns whether a validator (given its operator address) is a block proposer.
+// Note: all validators are proposers if none is set.
+func (k Keeper) GetIsProposer(ctx context.Context, operatorAddrStr string) (bool, error) {


Actually, how do we plan to use GetIsProposer for individual validator? I imagine this is somewhere in x/staking where you need to output the ValidateUpdate, and you need to augment the VP change with whether the validator is a proposer. Could we just call GetAllProposers, cache in memory, and use that in-memory object instead?

Not having to do GetIsProposer means you could just store one proto with a list of proposers addresses, which greatly reducing the complexity around working individual validator keys

teddyding · 2025-07-30T00:49:00Z

+func (k Keeper) GetProposers(ctx context.Context) ([]string, error) {
+	store := k.storeService.OpenKVStore(ctx)
+
+	bz, err := store.Get(types.ProposerSetKey)


For my understanding, what does this return if types.ProposerSetKey is not touched at all yet?

this will return bz=nil, err=nil

teddyding · 2025-07-30T00:52:08Z


+	// Set proposers if specified in genesis.
+	// Note: if no proposer is set, all validators default to being proposers.
+	if len(data.Proposers) > 0 {


Did you run this locally from genesis? Wonder what happens if you call GetProposers before calling SetProposers at all? Trying to reason if a state migration is needed

Yeah GetProposers just returns empty list (of strings)

in the abci method where x/staking creates validatorUpdates, i'm thinking that we call GetProposers, if it's empty, every ValidatorUpdate will set CanPropose=true, otherwise, set accordingly

so yeah no state migration should be needed

teddyding · 2025-07-30T00:52:38Z

+
+// SetProposers sets proposers in state by storing their operator addresses.
+// Returns error if proposer set invariants are violated.
+func (k Keeper) SetProposers(ctx context.Context, proposers []string) error {


You mentioned we tested locally through governance proposal - was this implemented somewhere else not in this PR?

yep i have it locally and going to create another PR for it after this one is merged

teddyding · 2025-07-30T00:53:18Z

+
+	// key for proposer set
+	// Note: all validators default to being proposers if proposer set is empty
+	ProposerSetKey = []byte("ProposerSet")


PS for storage efficiency

teddyding · 2025-07-30T00:57:56Z

+		}
+	}
+
+	return fmt.Errorf("at least one proposer must be bonded")


Instead of just a raw fmt.Errorf, consider wrapping an error code defined here. The error code can start at a different magnitude to avoid collision with upstream (eg 500, 501...)

teddyding · 2025-07-30T00:58:57Z

+		if err != nil {
+			return err
+		}
+		if validator.Status == types.Bonded {


I think we can generalize this by adding a code level constant MIN_BONDED_IN_PROPOSER SET, something like 5. Wdyt?

sure! was trying to avoid hardcoding but think it's more robust than just requiring 1

northstar456 · 2025-07-30T16:11:30Z


+const (
+	// minimum number of bonded validators required in a proposer set
+	MIN_BONDED_IN_PROPOSER_SET = 5


Fix formatting lint

northstar456 · 2025-07-30T16:12:23Z


-	return fmt.Errorf("at least one proposer must be bonded")
+	if bonded < MIN_BONDED_IN_PROPOSER_SET {
+		return errorsmod.Wrapf(types.ErrInsufficientBondedValidators,


nit; ErrTooFewBondedProposers

* feat(proposer-set-reduction): Add proposer set state to x/staking (#71) * add proposer set state to x/staking * all validators are proposers when none is set and add more tests * get and set entire proposers list instead of each individual proposer * have a constant define min num of bonded in proposer set * fix lint and better error name * add gov msg that sets and query that reads proposer set (#72) * add gov msg that sets and query that reads proposer set * add comment on invariant checks * send proposer set updates in x/staking EndBlocker ABCI validator updates (#73) * send proposer set updates in x/staking EndBlocker ABCI validator updates * set full update flag in SetProposers keeper method and set CanPropose when constructing validator updates * update simapp's cometbft dependency * update cometbft dependency of /tests * add sanity check on full proposer set update * use cometbft commit on main (#74)

* feat(proposer-set-reduction): Add proposer set state to x/staking (dydxprotocol#71) * add proposer set state to x/staking * all validators are proposers when none is set and add more tests * get and set entire proposers list instead of each individual proposer * have a constant define min num of bonded in proposer set * fix lint and better error name * add gov msg that sets and query that reads proposer set (dydxprotocol#72) * add gov msg that sets and query that reads proposer set * add comment on invariant checks * send proposer set updates in x/staking EndBlocker ABCI validator updates (dydxprotocol#73) * send proposer set updates in x/staking EndBlocker ABCI validator updates * set full update flag in SetProposers keeper method and set CanPropose when constructing validator updates * update simapp's cometbft dependency * update cometbft dependency of /tests * add sanity check on full proposer set update * use cometbft commit on main (dydxprotocol#74)

github-actions Bot added the C:x/staking label Jul 23, 2025

tqin7 changed the title ~~Add proposer set state to x/staking~~ feat(proposer-set-reduction): Add proposer set state to x/staking Jul 23, 2025

tqin7 force-pushed the tq/add-proposer-set-state branch from e700587 to d982fb5 Compare July 23, 2025 05:04

add proposer set state to x/staking

8d8510f

tqin7 force-pushed the tq/add-proposer-set-state branch from d982fb5 to 8d8510f Compare July 23, 2025 07:33

tqin7 changed the base branch from dydx-fork-v0.50.5 to feature/proposer-set-reduction July 23, 2025 14:25

northstar456 reviewed Jul 25, 2025

View reviewed changes

all validators are proposers when none is set and add more tests

0c12de3

northstar456 reviewed Jul 28, 2025

View reviewed changes

get and set entire proposers list instead of each individual proposer

5a2e6d4

teddyding reviewed Jul 30, 2025

View reviewed changes

have a constant define min num of bonded in proposer set

958fa08

tqin7 force-pushed the tq/add-proposer-set-state branch from fdf64ad to 958fa08 Compare July 30, 2025 05:56

northstar456 reviewed Jul 30, 2025

View reviewed changes

fix lint and better error name

718989e

tqin7 merged commit 025627b into feature/proposer-set-reduction Jul 30, 2025
37 of 38 checks passed

tqin7 deleted the tq/add-proposer-set-state branch July 30, 2025 18:22


		DelegationByValIndexKey = []byte{0x71} // key for delegations by a validator

		ProposerKeyPrefix = []byte("Proposer:") // prefix for proposers (only a subset of validators can propose blocks)

Conversation

tqin7 commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Author Checklist

Reviewers Checklist

Uh oh!

github-actions Bot commented Jul 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tqin7 Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

northstar456 Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tqin7 Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teddyding Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teddyding Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

tqin7 commented Jul 23, 2025 •

edited

Loading

tqin7 Jul 25, 2025 •

edited

Loading

northstar456 Jul 25, 2025 •

edited

Loading

tqin7 Jul 25, 2025 •

edited

Loading

teddyding Jul 30, 2025 •

edited

Loading

teddyding Jul 30, 2025 •

edited

Loading