-
Notifications
You must be signed in to change notification settings - Fork 886
Refuse to start a mock_chain_validation build outside its reserve role #4147
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
base: release/v6.7
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| //go:build !mock_chain_validation | ||
|
|
||
| package server | ||
|
|
||
| import sctypes "github.com/sei-protocol/sei-chain/sei-db/state_db/sc/types" | ||
|
|
||
| // assertReserveNodeAllowed reports whether a node may start with the given | ||
| // Tendermint mode and effective state-commit write mode. Production builds | ||
| // accept every combination the configuration itself accepts. | ||
| func assertReserveNodeAllowed(string, sctypes.WriteMode) error { return nil } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| //go:build !mock_chain_validation | ||
|
|
||
| package server | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| sctypes "github.com/sei-protocol/sei-chain/sei-db/state_db/sc/types" | ||
| tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" | ||
| ) | ||
|
|
||
| // A production build must not refuse any mode/write-mode combination; the | ||
| // reserve guard exists only in the mock_chain_validation build. | ||
| func TestAssertReserveNodeAllowed_Default_AcceptsEveryCombination(t *testing.T) { | ||
| for _, nodeMode := range allNodeModes() { | ||
| for _, writeMode := range allSCWriteModes() { | ||
| require.NoError(t, assertReserveNodeAllowed(nodeMode, writeMode), | ||
| "mode %q with write mode %q must be accepted by a production build", nodeMode, writeMode) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func allNodeModes() []string { | ||
| return []string{tmcfg.ModeFull, tmcfg.ModeValidator, tmcfg.ModeSeed} | ||
| } | ||
|
|
||
| func allSCWriteModes() []sctypes.WriteMode { | ||
| return []sctypes.WriteMode{ | ||
| sctypes.MemiavlOnly, | ||
| sctypes.MigrateEVM, | ||
| sctypes.EVMMigrated, | ||
| sctypes.MigrateAllButBank, | ||
| sctypes.AllMigratedButBank, | ||
| sctypes.MigrateBank, | ||
| sctypes.FlatKVOnly, | ||
| sctypes.TestOnlyDualWrite, | ||
| sctypes.Auto, | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| //go:build mock_chain_validation | ||
|
|
||
| package server | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| sctypes "github.com/sei-protocol/sei-chain/sei-db/state_db/sc/types" | ||
| tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" | ||
| ) | ||
|
|
||
| // assertReserveNodeAllowed reports whether a node may start with the given | ||
| // Tendermint mode and effective state-commit write mode. This build starts | ||
| // only as a non-validator pinned to memiavl_only. | ||
| // | ||
| // Both conditions are what make the build a reserve rather than a liability. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] The content is worth keeping — it just belongs at the branches it explains. Suggest trimming the godoc to the first paragraph (lines 12-14, which already says exactly what the function does) and moving the validator rationale above the |
||
| // The consensus policy compiled in here swallows ErrAppHash and the | ||
| // validator-set sentinels, so a validator running it would prevote and | ||
| // precommit blocks whose app hash contradicts its own state instead of | ||
| // prevoting nil. And a node that is not pinned joins the migration on the | ||
| // first block after governance raises the batch size, which spends the reserve | ||
| // with nothing to signal that it happened. | ||
| // | ||
| // Refusing here rather than trusting app.toml costs a restart to discover and | ||
| // saves finding out at the first divergent block. | ||
| func assertReserveNodeAllowed(nodeMode string, writeMode sctypes.WriteMode) error { | ||
| if nodeMode == tmcfg.ModeValidator { | ||
| return fmt.Errorf( | ||
| "mock_chain_validation builds must not run as a validator: this build "+ | ||
| "swallows app-hash and validator-set validation failures, so it cannot "+ | ||
| "safely vote; set mode = %q in config.toml", | ||
| tmcfg.ModeFull, | ||
| ) | ||
| } | ||
| if writeMode != sctypes.MemiavlOnly { | ||
| return fmt.Errorf( | ||
| "mock_chain_validation builds must run %[1]q, got %[2]q: set "+ | ||
| "state-commit.sc-write-mode = %[1]q and "+ | ||
| "state-commit.sc-write-mode-enable-auto = false in app.toml (the latter "+ | ||
| "is absent from a generated app.toml and defaults to true, which "+ | ||
| "discards the former)", | ||
| sctypes.MemiavlOnly, writeMode, | ||
| ) | ||
| } | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| //go:build mock_chain_validation | ||
|
|
||
| package server | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| sctypes "github.com/sei-protocol/sei-chain/sei-db/state_db/sc/types" | ||
| tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config" | ||
| ) | ||
|
|
||
| // Exactly one combination may start: a non-validator pinned to memiavl_only. | ||
| func TestAssertReserveNodeAllowed_MockChainValidation_Matrix(t *testing.T) { | ||
| for _, nodeMode := range allNodeModes() { | ||
| for _, writeMode := range allSCWriteModes() { | ||
| allowed := nodeMode != tmcfg.ModeValidator && writeMode == sctypes.MemiavlOnly | ||
| err := assertReserveNodeAllowed(nodeMode, writeMode) | ||
| if allowed { | ||
| require.NoError(t, err, "mode %q with write mode %q must be accepted", nodeMode, writeMode) | ||
| continue | ||
| } | ||
| require.Error(t, err, "mode %q with write mode %q must be refused", nodeMode, writeMode) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Validator mode is refused whatever the write mode, so a correctly pinned | ||
| // validator does not slip through. | ||
| func TestAssertReserveNodeAllowed_MockChainValidation_RefusesPinnedValidator(t *testing.T) { | ||
| err := assertReserveNodeAllowed(tmcfg.ModeValidator, sctypes.MemiavlOnly) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), tmcfg.ModeFull) | ||
| } | ||
|
|
||
| // Auto is what a forgotten sc-write-mode-enable-auto resolves to, and it is the | ||
| // failure the write-mode half of this guard exists for, so the message must | ||
| // name that key. | ||
| func TestAssertReserveNodeAllowed_MockChainValidation_ErrorNamesTheAutoKey(t *testing.T) { | ||
| err := assertReserveNodeAllowed(tmcfg.ModeFull, sctypes.Auto) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "sc-write-mode-enable-auto") | ||
| } | ||
|
|
||
| func allNodeModes() []string { | ||
| return []string{tmcfg.ModeFull, tmcfg.ModeValidator, tmcfg.ModeSeed} | ||
| } | ||
|
|
||
| func allSCWriteModes() []sctypes.WriteMode { | ||
| return []sctypes.WriteMode{ | ||
| sctypes.MemiavlOnly, | ||
| sctypes.MigrateEVM, | ||
| sctypes.EVMMigrated, | ||
| sctypes.MigrateAllButBank, | ||
| sctypes.AllMigratedButBank, | ||
| sctypes.MigrateBank, | ||
| sctypes.FlatKVOnly, | ||
| sctypes.TestOnlyDualWrite, | ||
| sctypes.Auto, | ||
| } | ||
| } |
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.
[suggestion] The
!mock_chain_validationtag means amock_block_validationbinary compiles this no-op, but that build has the same voting hazard the guard exists to close:sei-tendermint/types/consensus_policy_mock_block_validation.goswallowsErrAppHash(plusErrDataHash,ErrUpgradeBeforeTrigger), so such a validator would also prevote and precommit blocks whose app hash contradicts its own state. It is a shipped image too —.github/workflows/ecr.ymlpushessei-chain:mock_block_validation-<ref>alongside the chain-validation one.The write-mode pin is specific to the reserve-node role and shouldn't apply there, but the validator half arguably should. Widening the guard file to
!mock_block_validation && !mock_chain_validationand givingmock_block_validationa variant that refusesModeValidatoronly would make "a build that cannot detect divergence must not vote" the invariant rather than a per-tag convention. If that is deliberately out of scope for this PR, a one-line note saying so would help the next person reading this tag.