Skip to content

Fix: align consensus proposer priorities#828

Open
amimart wants to merge 6 commits into
devfrom
arnaud/engn-3825-align-consensus-proposer-priorities
Open

Fix: align consensus proposer priorities#828
amimart wants to merge 6 commits into
devfrom
arnaud/engn-3825-align-consensus-proposer-priorities

Conversation

@amimart

@amimart amimart commented May 28, 2025

Copy link
Copy Markdown
Contributor

Purpose of Changes and their Description

Add a new v0.13.0 upgrade containing a reset of validator set proposer priorities to ensure all nodes runs the same.

Details

The proposer priorities are stored in comet's state store which is not accessible from the cosmos side of the app so to access it we need to manually open the db, which is not possible at the time the upgrade is performed by the x/upgrade module because at this moment the comet node is already created and a system lock is there to prevent any access to the store.

To solve this I've introduced a PreStartupUpgrade func field in the upgrade type to set a function to be called at application creation but before startup.

In the PreStartupUpgrade implementation it basically use the configuration given to the app to open comet state.db store and update the last state NextValidators field, which contains the validator set to use in the next block, and set the ProposerPriority field to the same value (i.e. the one used for a new validator) on all the validators.

Testing

In order to test this locally here's the steps I've followed:

First setup a localnet with 4 validators with the v0.13.0 version staged:

DO_UPGRADE="true" UPGRADE_VERSION="v0.13.0" VALIDATOR_NUMBER=4 bash test/local_testnet_l1.sh

Then we need to poison one of the validator by shifting its internal consensus state priorities, I did this by adding the following debug cmd:

func poisonCometStateCommand() *cobra.Command {
	return &cobra.Command{
		Use:   "poison-comet-state",
		Short: "Poison CometBFT consensus state",
		Long:  `An utility commands to help poison a node's comet state by incrementing next validator set proposer priorities by one round.`,
		PreRunE: func(cmd *cobra.Command, _ []string) error {
			return server.GetServerContextFromCmd(cmd).Viper.BindPFlags(cmd.Flags())
		},
		RunE: func(cmd *cobra.Command, _ []string) error {
			svrCtx := server.GetServerContextFromCmd(cmd)
			cfg := svrCtx.Config
			stateDB, err := cmtcfg.DefaultDBProvider(&cmtcfg.DBContext{ID: "state", Config: cfg})
			if err != nil {
				return err
			}
			stateStore := sm.NewStore(stateDB, sm.StoreOptions{
				DiscardABCIResponses: cfg.Storage.DiscardABCIResponses,
			})
			state, err := stateStore.Load()
			if err != nil {
				return err
			}
			state.NextValidators.IncrementProposerPriority(1)
			return stateStore.Save(state)
		},
	}
}

It can be run on one of the validators (after being temporarily stopped) with:

allorad debug poison-comet-state --home test/localnet/validator1

After restarting the poisoned node we can observe some logs showing that it is receiving proposals from an unexpected proposer:

10:29PM ERR failed to process message err="error invalid proposal signature" height=608 module=consensus msg_type=*consensus.ProposalMessage peer=34d2dbfc6a7794940a6c74e04b65071d3f32f5b5 round=0

Finally we can trigger the upgrade with UPGRADE=TRUE go test -timeout 15m ./test/integration/ -v and after the upgrade is applied the logs disappears.

Are these changes tested and documented?

  • If tested, please describe how. If not, why tests are not needed.
  • If documented, please describe where. If not, describe why docs are not needed.
  • Added to Unreleased section of CHANGELOG.md?

@amimart amimart requested review from guilherme-brandao, spooktheducks, xmariachi and zale144 and removed request for guilherme-brandao May 28, 2025 16:43
@amimart amimart marked this pull request as ready for review May 28, 2025 16:43
xmariachi
xmariachi previously approved these changes May 30, 2025

@xmariachi xmariachi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm (needs linter fixes)

@amimart amimart force-pushed the arnaud/engn-3825-align-consensus-proposer-priorities branch from 0fbeaab to 9a094f6 Compare May 30, 2025 14:25
@xmariachi xmariachi force-pushed the arnaud/engn-3825-align-consensus-proposer-priorities branch from 9a094f6 to 86613bf Compare August 6, 2025 10:51

@xmariachi xmariachi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rebased, updated changelog

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.

2 participants