commitment: catch cases when parallel commitment not enabled - #23596
Draft
AskAlexSharov wants to merge 10 commits into
Draft
commitment: catch cases when parallel commitment not enabled#23596AskAlexSharov wants to merge 10 commits into
AskAlexSharov wants to merge 10 commits into
Conversation
…ode review --limit could push block past the real stages.Senders ceiling when sendersProgress==0 or execProgress>=sendersProgress, causing the exec loop to spin forever waiting for an unreachable target. Clamp --limit to sendersProgress instead. Also restores the execProgress==0-only gate around the SeekCommitment fallback (was running unconditionally on every invocation) and closes SharedDomains on its error path.
# Conflicts: # cmd/integration/commands/flags.go # cmd/integration/commands/stages.go
…p mode The chain-tip loop ran execBlocksBatch(bn) for bn in [execProgress, block), so the first iteration re-targeted a block already executed and the resolved target was never reached: progress 100 with --limit=1 resolves to 101 and executed nothing. Iterate (execProgress, block] instead. Also from review: - profiling starts before ApplyMigrations, so its failure path still produces the profiles the flag promises. - StopCPUProfile only runs when this helper started one; --pprof.cpuprofile already holds it otherwise, and stopping it closed it out from under its owner. The mutex rate is restored to its previous value rather than 0 for the same reason. - target resolution moves into resolveExecTarget with table-driven coverage of the --limit/--block/progress boundaries.
… trie stage_exec, state_stages and loop_exec all compute commitment but never registered --experimental.parallel-commitment, so the flag was unknown there and only ERIGON_COMMITMENT_PARALLEL could reach statecfg. A stage_exec profile on n5 shows the cost: the sequential trie saturates one core for the whole exec window while the executor's workers sit 96% idle on a 16-core box. The flag default now ORs erigon's own flag default with the env-derived value, so flipping the default in one binary cannot leave the other on a different trie.
…8n and custom_trace Both compute commitment against a SharedDomains that never called EnableParaTrieDB. NewSharedDomainsCommitmentContext starts on the sequential trie and only upgrades once a DB arrives, so these two ran the sequential trie whatever the flag or ERIGON_COMMITMENT_PARALLEL said -- silently, since the fallback is the design's escape hatch for RPC and integrity contexts that have no DB to give. t8n is the sharper case: cmd/evm statetest already wires it, so one binary computed state roots two different ways. Construction moves into a named helper per site so a test can assert the wiring rather than the whole command.
…e test harnesses GenerateChain, the state-test harness and MakePreState all compute a state root on a SharedDomains that never called EnableParaTrieDB, so the CI shards that set ERIGON_COMMITMENT_PARALLEL were exercising the sequential trie in those paths. MakePreState has no DB of its own, so it takes one; every caller already holds m.DB. InitPraguePreDeploys is left alone: it only flushes, and Commit does no trie work.
… loud NewSharedDomainsCommitmentContext starts on the sequential trie and upgrades only once EnableParaTrieDB arrives. That fallback is deliberate for the DB-less RPC and integrity contexts, but nothing separated it from a caller that simply forgot, so several sites computed state roots on the sequential trie with the flag on and no sign of it. WithParaTrieDB folds the wiring into construction, so selecting the trie and supplying its DB are one expression with no second call to forget. ComputeCommitment now refuses to be quiet about the remaining case: a context that still holds a pending parallel selection fails under ERIGON_ASSERT and logs a one-shot warning otherwise. Assertions are on in CI, so a new site that forgets the DB goes red instead of testing the wrong trie. It returns an error rather than panicking because the calculator drives ComputeCommitment from a goroutine the exec loop waits on, where a panic parks the loop instead of failing it. The guard immediately caught setupStepTest, which is wired here.
AskAlexSharov
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on #23544.
NewSharedDomainsCommitmentContextstarts on the sequential trie and upgrades only onceEnableParaTrieDBarrives. That fallback is deliberate for the DB-less RPC and integrity contexts, but nothing separated it from a caller that simply forgot — which is howcmd/evm t8n,stage_custom_trace,GenerateChain, the state-test harness andMakePreStateall ended up computing state roots on the sequential trie with the flag on, silently (fixed in #23544).Two layers:
WithParaTrieDB(db)folds the wiring into construction, so selecting the trie and supplying its DB are one expression with no second call to forget.ComputeCommitmentstops being quiet about the rest: a context still holding a pending parallel selection fails underERIGON_ASSERTand logs a one-shot warning otherwise. Assertions are on in CI, so a new site that forgets the DB goes red instead of testing the wrong trie.An error rather than a panic — the calculator drives
ComputeCommitmentfrom a goroutine the exec loop waits on, where a panic parks the loop instead of failing it. That is not hypothetical: the first draft panicked and turned a committer test into a 10-minute hang.The guard immediately caught one more unwired site,
setupStepTest, wired here.