Refuse to start a mock_chain_validation build outside its reserve role - #4147
Refuse to start a mock_chain_validation build outside its reserve role#4147blindchaser wants to merge 2 commits into
Conversation
A mock_chain_validation binary swallows ErrAppHash and the validator-set sentinels, which is what lets a reserve node keep following a chain it disagrees with. That same property makes two configurations dangerous rather than merely wrong: as a validator it would prevote and precommit blocks whose app hash contradicts its own state instead of prevoting nil, and left unpinned it resolves to auto and joins the migration on the first block after governance raises the batch size, spending the reserve with nothing to signal that it happened. Both are now refused in startInProcess, which every deployed node reaches and no test does. The unpinned case is the default state of a fresh node rather than an unlikely slip: a generated app.toml renders sc-write-mode = "memiavl_only" but not sc-write-mode-enable-auto, and while that unrendered key stays true it discards the rendered one. Production builds compile the no-op variant and are unaffected. Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit a161677)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
PR SummaryMedium Risk Overview Startup now calls Reserve-specific state-commit defaults go through Tests and golden files are split by build tag ( Reviewed by Cursor Bugbot for commit feb9aa7. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v6.7 #4147 +/- ##
================================================
- Coverage 61.34% 60.28% -1.07%
================================================
Files 2163 2061 -102
Lines 188785 176640 -12145
================================================
- Hits 115813 106488 -9325
+ Misses 62254 60406 -1848
+ Partials 10718 9746 -972
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A small, well-placed startup guard: startInProcess is genuinely the only production node-start path (the other node.New call sites are test/inprocess-tagged), and config.StateCommit.WriteMode from GetConfig is the post-ApplyWriteModeAuto effective mode, so the pin check is reading the right value. No blockers; two non-blocking gaps — the sibling mock_block_validation image has the same validator hazard and is left unguarded, and the guard's godoc carries rationale the repo guide says belongs inline.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion] No CI job runs
go test -tags mock_chain_validation(go-test.ymlandsei-db-tests.ymlsetGOFLAGS=-tags=ledger,test_ledger_mock), so the entiremock_chain_validationtest surface — the pre-existingsei-tendermint/types/consensus_policy_mock_chain_validation_test.goand friends, and nowsei-cosmos/server/reserve_policy_mock_chain_validation_test.go— is neither run nor compile-checked on a PR. The only thing that compiles that build is the on-demand/nightly ECR image job, so a break in tag-gated code surfaces at image-build time rather than at review time.
| @@ -0,0 +1,10 @@ | |||
| //go:build !mock_chain_validation | |||
There was a problem hiding this comment.
[suggestion] The !mock_chain_validation tag means a mock_block_validation binary 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.go swallows ErrAppHash (plus ErrDataHash, 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.yml pushes sei-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_validation and giving mock_block_validation a variant that refuses ModeValidator only 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.
| // 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.
[suggestion] AGENTS.md § Godoc asks for what, not why or how: "Rationale, trade-offs, and mechanism belong in an inline comment at the line that needs them, or nowhere", and "Multi-paragraph godocs are rare." These two paragraphs are rationale (why a validator is unsafe, why an unpinned node spends the reserve) and a trade-off (restart cost vs. divergent block) attached to the doc comment.
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 nodeMode == tmcfg.ModeValidator check and the migration/pin rationale above the writeMode != sctypes.MemiavlOnly check.
A reserve node follows the chain on memiavl_only so it holds pre-H_start state that the fleet can recover from once the FlatKV migration is under way. Getting a node into that state depended on the operator pinning the write mode by hand at creation time, and a value that misses `seid init` cannot reach the node afterwards, so a missed key cost a rebuild. Under the mock_chain_validation tag the build now defaults to the reserve configuration instead of depending on that step. sc-write-mode-enable-auto defaults to false, so the shipped memiavl_only write mode is honoured rather than being forced to auto, and the key is rendered into app.toml so the file states what the node actually does. Production builds are unaffected: the default stays true and the key stays unrendered. The existing start-up guard already refuses a validator role or a write mode other than memiavl_only, so a misconfigured reserve node fails loudly rather than diverging quietly. This change removes the configuration step that guard was most likely to catch. The configuration characterization suite pins this behaviour, so its records are now kept per build tag: the default-build golden is unchanged and a `.reserve.golden` records what the reserve build resolves to. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
On top of
v6.7.0-rc1. That tag is currently the head ofrelease/v6.7(
e59189ea7), so this branch is the tag plus one commit and the cherry-pick wasclean.
What this changes
mock_chain_validationbuilds now refuse to start unless they are a non-validatorpinned to
memiavl_only. Production builds compile a no-op and are unaffected.The check is one function with two build variants, called once from
startInProcess:Why
A
mock_chain_validationbinary swallowsErrAppHashand the validator-setsentinels. That is exactly what makes it useful as a reserve node — it keeps
following a chain it disagrees with — and it is also what makes two configurations
dangerous rather than merely wrong.
As a validator it would prevote and precommit blocks whose app hash contradicts
its own state, instead of prevoting nil. A node that cannot detect divergence must not
vote on it.
Left unpinned it resolves to
autoand joins the migration on the first blockafter governance raises the batch size, spending the reserve with nothing to signal
that it happened.
The unpinned case is the default state of a fresh node rather than an unlikely slip.
A generated
app.tomlrenderssc-write-mode = "memiavl_only"but does notrender
sc-write-mode-enable-auto, and while that unrendered key staystrueitdiscards the rendered one. So pinning the visible key is not enough, which is why the
error message names the invisible one.
Where the guard lives
startInProcessis the single function every deployed node passes through and no testreaches, so the guard is an invariant rather than a convention a later call site can
forget. Refusing at startup costs a restart to discover, and saves finding out at the
first divergent block.
Testing
Both build variants are covered, since the interesting behaviour is which variant
compiles:
TestAssertReserveNodeAllowed_Default_AcceptsEveryCombinationsweeps all 3 nodemodes × 9 write modes and asserts a production build refuses nothing.
TestAssertReserveNodeAllowed_MockChainValidation_Matrixsweeps the same grid andasserts exactly one cell is allowed.
TestAssertReserveNodeAllowed_MockChainValidation_RefusesPinnedValidatorcovers acorrectly pinned validator, so the write-mode half cannot mask the mode half.
TestAssertReserveNodeAllowed_MockChainValidation_ErrorNamesTheAutoKeypins thatthe message names
sc-write-mode-enable-auto, because that is the key the operatorhas to set and the one a generated
app.tomlnever shows them.go test ./sei-cosmos/server/passes with and without-tags mock_chain_validation,and
make fmtcheckis clean.Release note
v6.7.0-rc1is already cut, so merging this implies an rc2.