docs(runbooks): validate gas-vs-execution-time methodology end to end#459
Open
bdchatham wants to merge 6 commits into
Open
docs(runbooks): validate gas-vs-execution-time methodology end to end#459bdchatham wants to merge 6 commits into
bdchatham wants to merge 6 commits into
Conversation
Replaces the eBPF-first draft with a validated RPC-native approach: debug_traceTransactionProfile gives clean per-tx gas + execution time in one call (r=0.9914 vs gas on real replayed pacific-1 txs, vs r=0.301 for whole-block latency). Documents the version-pinning requirement (app-hash mismatch on any binary divergence), the mock_chain_validation build mode for testing unreleased code against real transaction load, the Y=X-1-only limitation for Functionality 1, the debug_traceCall execution-time gap for Functionality 2, and the MaxTraceLookbackBlocks race condition on a fast-catching-up replayer. eBPF is demoted to a fallback tier, no longer the default path.
PR SummaryLow Risk Overview The runbook now centers on replayer image pinning (release tag vs eBPF moves to a fallback section with stricter harbor attach/sign-off guidance; Appendix A records OpenTelemetry tracing as tested and rejected for per-tx gas–time correlation. Adds §8 on existing per-block log/metrics signals and expands failure modes, pre-flight checklist, and sei-chain references. Reviewed by Cursor Bugbot for commit 901cccf. Bugbot is set up for automated code reviews on this repo. Configure here. |
Four independent reviewers (prose-steward, kubernetes-specialist, systems-engineer as dissenter, security-specialist) found real gaps, all now fixed: - spec.peers was missing from the replayer render example (admission CEL rule requires it -- kubernetes-specialist) - the replayer is controller-flagged RPC-less (.status.endpoint absent); now documents port-forwarding 8545 directly (kubernetes-specialist) - "Tendermint state-sync" was the wrong term for the replayer's actual S3-snapshot restore mechanism (kubernetes-specialist) - the <archive-node> placeholder wired state-mutating §6 commands to shared archive infra; now scoped read-only with the boundary restated in §5/§6 (security-specialist) - "get sign-off first" now requires an affirmative response, not silence; added IMDS/node-credential blast radius, TTL/teardown, and fixed example commands contradicting their own minimize-caps advice (security-specialist) - §2b now states gas metering is untouched by mock_chain_validation, adds revert-rate monitoring alongside the skip counter, and restates the throwaway-node-only boundary (security-specialist, systems-engineer) - §5 no longer overstates debug_traceCall's historical reach -- it's capped by the same 10k-block lookback as the rest of debug_trace* (systems-engineer) - r=0.9914 now carries its n=9/one-session qualifier everywhere it's cited as justification; "447 commits" softened to a non-exact, rot-resistant phrasing (prose-steward, systems-engineer) - README.md's index entry updated to match the new methodology, was still describing the pre-rewrite eBPF-first draft (prose-steward)
…gaps Empirical follow-up this session found the default debug_traceTransactionProfile invocation attaches a full StructLogger inside the executionNanos timing bracket, inflating the gas-vs-time correlation (r=0.9914 -> r=0.8093 on a diverse sample once switched to callTracer). Also folds in a peer collaboration with systems-engineer that ruled out eBPF for the begin/end-blocker gap and per-opcode timing, endorsed perf-stat cycle counters as the one worthwhile (non-uprobe) kernel-level cross-validation, and flagged Y=X-1 as blocking the state-opcode cost-vs-state-size question specifically -- the biggest open gap, not closed by any instrumentation choice here. Adds min-of-N/forced-GC timing discipline, separates interpreter-compute from state-I/O in reporting, documents a real sampling mistake (near-zero-variance gas sample gave a meaningless r=0.11), and notes that release tags and backport branches can diverge on whether the profiling endpoint exists -- check the exact image, not the release family.
…st + systems-engineer kubernetes-specialist (re-verified against current source): - sei.io/dedicated-node must be set at initial render, not toggled on a running replayer -- buildRunningPlan only rebuilds the pod on image drift or sidecar-reapproval, an annotation-only change triggers neither, and OnDelete means nothing else replaces the pod either. The prior guidance was silently ineffective if applied after the fact. - added the no-SeiNode-condition-on-stuck-Pending caveat (check the pod's own PodScheduled condition/events, not the SeiNode object). - confirmed the "highest snapshot <= targetHeight" claim directly against seictl's own source (resolveKeyForHeight), resolving an apparent conflict with the SeiNode CRD's own (incomplete) doc comment in favor of the runbook's original claim -- added the precise citation and a heads-up not to trust the CRD comment over seictl's actual logic. systems-engineer (second pass, fresh eyes on the prior fix): - caught a new, higher-consequence bug in the just-added executionNanos-minus-storeNanos guidance: the store trace spans the whole statedb lifetime including every replayed preceding tx, not just the target tx, so the subtraction silently goes negative past the first tx or two in a block. Replaced with an honest caveat and flagged the real fix (resetting the tracer at the execution boundary) as a sei-chain change, not a subtraction on today's fields. - caught a category error: a same-gas cluster's 2x timing spread was mischaracterized as noise addressable by min-of-N, which only reduces noise across repeats of one tx, not variance across different txs. Reframed as a real signal to check (repeat each tx individually) before dismissing it. - added statistical caveats to the r=0.8093 headline (single-sample, not min-of-N; Pearson r is a smoke test not a pricing-fidelity measure; r-squared/Spearman/residuals recommended over raw r). - corrected the callTracer overhead model (scales with call-frame/log count, not opcode count -- near-zero for the primary interpreter-local target), warned off noopTracer as a false lighter baseline, added the onlyTopCall differencing technique, and softened an overclaim about perf stat cycles being immune to GC.
…ve OTel tracing to appendix Adds two validated findings to the main body: subtracting intrinsic gas before correlating (a client-side fix, no sei-chain change), and the full per-block FinalizeBlock timing answer via the existing "execution block time" log line, with the replayer-vs-validator and EVM-vs-native-traffic caveats. Moves the OpenTelemetry tracing investigation to Appendix A. It was built and tested with real data on two replayer nodes (concurrent and serial OCC execution against the identical historical chain) and found to have an irreducible, deterministic node-level confound (cross-node Jaccard 0.92 on the same outlier transactions rules out both OCC contention and random per-process noise) that biases the correlation rather than measuring it. Documented so it isn't re-proposed without this context.
Four independent blinded reviewers (systems-engineer, kubernetes-specialist, opentelemetry-expert as dissenter, prose-steward) reviewed the restructuring in 9ffe227. Fixes: - run_msg_latency label value was wrong (cited the gRPC service/method path instead of the actual proto message-type name MsgTypeURL emits) - would have silently matched zero series if copy-pasted. - block_process_duration metric name was missing its rendered app_ prefix and _seconds unit suffix. - Appendix A's wrapper mechanism didn't state it only works by shadowing seid on $PATH, not via image ENTRYPOINT - the controller hardcodes the container Command/Args and never invokes the image's own entrypoint. - Appendix A's causal explanation for the AnteHandler-inflation outlier is corrected: two reviewers, independently and blinded from each other, converged on the same fix - the tracer's own in-process batch-export flush (size-triggered, deterministic by replay position) is a better-fitting candidate than GC/memIAVL, and "rules out the Jaeger collector stealing CPU" was overclaimed (only rules out random/wall-clock contention, not a deterministic in-process artifact both nodes share by construction). - The "still useful for phase attribution" carve-out is down-scoped: the same confound that corrupts the correlation also corrupts per-tx relative phase timing; only structural shape and aggregate/fleet-level timing survive. - Stale "eBPF (§8)" cross-reference in the adjacent runbooks/README.md index, missed by the in-document renumbering. Full ledger: .xreview/gas-vs-execution-time-runbook.md, Round 2.
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.
Summary
benchmarking-evm-gas-vs-execution-time.mdwith a validated RPC-native approach:debug_traceTransactionProfilegives clean per-tx gas + execution time in one call.executionNanoson real replayed pacific-1 transactions, vs. r=0.301 for whole-block log-line latency (the previously-assumed proxy).mock_chain_validationbuild mode for testing unreleased code against real transaction load (with its compounding-divergence caveat), theY=X-1-only limitation for Functionality 1 (confirmed by reading the actual RPC/replayer code), thedebug_traceCallexecution-time gap for Functionality 2, and theMaxTraceLookbackBlocksrace condition on a fast-catching-up replayer.Test plan