Use clean context snapshot for ProcessProposal gas validation (CON-173)#3197
Use clean context snapshot for ProcessProposal gas validation (CON-173)#3197wen-coding merged 13 commits intomainfrom
Conversation
processProposalState was only initialized once and reused across rounds at the same height. If a round timed out, speculative state from optimistic processing carried over into the next round's ProcessProposal. This is unnecessary since the old speculative state is stale and will never be committed. Now we create a fresh CacheMultiStore on each ProcessProposal call, except for the very first block where InitChain writes genesis state into the cache without committing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3197 +/- ##
==========================================
+ Coverage 58.64% 58.66% +0.02%
==========================================
Files 2055 2055
Lines 168494 168601 +107
==========================================
+ Hits 98810 98916 +106
+ Misses 60900 60896 -4
- Partials 8784 8789 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Only create a fresh CacheMultiStore when the proposal hash differs from the previous round. When the same hash is re-proposed after a timeout, reuse the existing cache so optimistic processing results remain valid. On hash mismatch, the app layer now clears optimistic processing state and starts a new goroutine on the fresh CacheMultiStore. The old goroutine detects the replacement via hash check and silently discards its results. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unconditionally create a fresh CacheMultiStore on every ProcessProposal call (except block 1 where InitChain genesis state must be preserved). Even for the same proposal hash, optimistic ProcessBlock writes (e.g. address associations) change the results of state-dependent checks like gasless eligibility, so the store must always be clean. On the app side, always clear optimistic processing info and start a new goroutine. The old goroutine detects replacement via hash check and silently discards its results. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Keep baseapp processProposalState reset, but revert app.go ProcessProposalHandler to main's original code to isolate which layer causes the AppHash mismatch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove processProposalState setup from InitChain — no longer needed since setProcessProposalState now branches from deliverState for block 1 (before genesis is committed to the root store). ProcessProposal unconditionally creates a fresh CacheMultiStore on every call, preventing speculative state from a previous round's optimistic processing from leaking into state-dependent checks. App layer (ProcessProposalHandler) is unchanged — shouldStartOptimistic Processing prevents concurrent ProcessBlock goroutines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SetProcessProposalStateToCommit now falls back to deliverState when processProposalState is nil. This handles test helpers that call Commit without going through ProcessProposal (e.g. sei-wasmd and sei-ibc-go snapshotter tests). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Branching processProposalState from deliverState for block 1 does not work because CacheMultiStore branching from a CacheMultiStore does not reliably surface uncommitted writes (e.g. distribution module's previous proposer). Restore InitChain's processProposalState setup and preserve it for block 1. Reset unconditionally for all subsequent blocks. The block 1 round timeout case (dirty state from round 0 leaking) is a pre-existing issue tracked separately. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
checkTotalBlockGas now reads from deliverState context instead of processProposalState context. deliverState always reflects the last committed state (or genesis for block 1) and is never written to between ProcessProposal calls, making gas validation immune to speculative writes from optimistic processing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GetDeliverStateContext falls back to committed root store when deliverState is nil (after Commit, before FinalizeBlock recreates it). Copy consensus params from processProposalState context onto the check context since they are not stored in the root store. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Snapshot a clean context at the start of ProcessProposal by branching from the source store (deliverState for block 1, committed root store for blocks 2+) rather than from processProposalState. This ensures checkTotalBlockGas is immune to speculative writes from optimistic processing, while preserving correct consensus params and header from processProposalState's context. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Branch clean context from source store (deliverState or cms) rather than from processProposalState, since CacheMultiStore branching reads through to the parent's cached writes. Update gas validation tests to go through BaseApp.ProcessProposal instead of calling ProcessProposalHandler directly, and to store consensus params on deliverState before committing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| if !app.checkTotalBlockGas(ctx, req.Txs) { | ||
| // Use the clean context snapshotted at the start of ProcessProposal, | ||
| // before optimistic processing can dirty the store. | ||
| checkCtx := app.GetProcessProposalCleanContext() |
There was a problem hiding this comment.
i think we can just assign to ctx here. If ctx is dirty then it probably shouldn't be referenced anywhere below
There was a problem hiding this comment.
I think that makes sense, had to fix more tests which call ProcessProposalHandler directly (while I think in production we should always go through ProcessProposal).
There was a problem hiding this comment.
Hmm, tried that and actually it didn't work:
The flow within a single ProcessProposalHandler call:
ctxcomes in asprocessProposalState.ctx(the writable store)- Line 1222:
ctx = cleanCtx— nowctxpoints to the read-only snapshot - Line 1243:
app.UpgradeKeeper.GetUpgradePlan(ctx)— reads from clean snapshot, fine - Line 1252: goroutine launches, captures
ctx - Line 1261:
app.ProcessBlock(ctx, ...)— writes to the clean snapshot's CacheMultiStore
Those writes are orphaned. Nobody commits the clean snapshot branch back toprocessProposalState. WhenFinalizeBlocklater commitsprocessProposalState, the state changes from optimistic processing aren't there.
So all integration tests now fail with "insufficient fund".
ProcessBlock writes to ctx's store for optimistic processing, so ctx must remain on processProposalState. The clean context is only used for the read-only gas validation check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f95cb2e to
eef4bb7
Compare
|
Ran local docker cluster, stopped and restarted one node, looks happy, submitting |
Summary
checkTotalBlockGasinProcessProposalHandlerreads fromprocessProposalState's context, which can contain stale writes from a previous round's optimistic processing. This PR snapshots a clean context at the start of eachProcessProposalcall and uses it for all downstream reads (gas validation, upgrade plan checks, optimistic processing).How it works
At the start of
ProcessProposal, afterprepareProcessProposalStatesets up the header and consensus params, we branch a newCacheMultiStorefrom the clean source store (deliverStatefor block 1, committed root store for blocks 2+). This is a lightweight read-through cache — no data is copied, reads fall through to the parent.The handler reassigns
ctxto the clean snapshot so all downstream reads use it. A defensive error is returned if the clean context is not set up (i.e. if the handler is called directly instead of throughProcessProposal).Changes
sei-cosmos/baseapp/abci.go: Snapshot clean context afterprepareProcessProposalState, before calling the handler.sei-cosmos/baseapp/baseapp.go: AddprocessProposalCleanCtxfield andGetProcessProposalCleanContext()accessor.sei-cosmos/baseapp/test_helpers.go: AddSetProcessProposalCleanCtxtest helper.app/app.go: Reassignctxfrom clean snapshot; add comment thatProcessProposalHandlermust not be called directly.app/app_test.go,app/upgrade_test.go: Convert tests to go throughProcessProposalinstead of calling handler directly.app/optimistic_processing_test.go: Set up clean context in test setup for internal tests.Test plan
TestProcessProposalCleanContextNotAffectedByHandler— verifies writes toprocessProposalStateare NOT visible through clean contextTestInvalidProposalWithExcessiveGasWanted,TestInvalidProposalWithExcessiveGasEstimates,TestOverflowGas— gas validation testsTestSkipOptimisticProcessingOnUpgrade— upgrade detection through clean contextTestProcessProposalHandlerPanicRecovery— panic recovery viaProcessProposal🤖 Generated with Claude Code