feat(seidb-bench): add write-set replay adapter for opcode storage-cost measurement#3772
feat(seidb-bench): add write-set replay adapter for opcode storage-cost measurement#3772blindchaser wants to merge 5 commits into
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| Slot: padTo32(slot), | ||
| Value: padTo32(post.Storage[slot]), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Zero storage emits write not delete
Medium Severity
convertStorage always emits a storage write when a slot appears in the tracer post map, including all-zero values. On Sei, clearing a slot goes through SetState, which deletes the key when the value is zero. Replay then measures a set of 32 zero bytes instead of a delete, skewing per-key apply/commit costs for SSTORE-to-zero traces.
Reviewed by Cursor Bugbot for commit cadec99. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cadec99411
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return []WriteSetEntry{ | ||
| {Kind: WriteKindCode, Address: addr, Value: hex.EncodeToString(code)}, | ||
| {Kind: WriteKindCodeHash, Address: addr, Value: hex.EncodeToString(ethcrypto.Keccak256(code))}, | ||
| {Kind: WriteKindRaw, Key: hex.EncodeToString(append(codeSizeKeyPrefix, addrBytes...)), Value: hex.EncodeToString(size)}, |
There was a problem hiding this comment.
When converting a code-deployment trace for an address without an existing Sei/EVM association, this only replays code, codehash, and codesize. The production path in x/evm/keeper/code.go lines 27-35 also calls SetAddressMapping on that miss, and x/evm/keeper/address.go lines 10-13 writes the 0x01||evm and 0x02||sei EVM raw keys; omitting those writes makes the replay undercount apply/commit cost and benchmark a different write set for new contract deployments.
Useful? React with 👍 / 👎.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-tested, self-contained benchmark-only addition (write-set capture/replay adapter for storage-cost measurement) that touches no production code paths. No blockers; a few benchmark-accuracy and documentation issues are worth addressing.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion review (
./cursor-review.md) is empty — that pass produced no output. Codex's three findings were reviewed and are reflected below. - Scope note (not a defect): everything here lives under
sei-db/state_db/benchand is gated behindTRACE_PATH/WRITESET_PATHenv vars, so there is no runtime impact on the chain. Findings are about the fidelity of the benchmark numbers, not correctness of shipped code. - No prompt-injection or suspicious content was found in the PR.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3772 +/- ##
==========================================
- Coverage 59.99% 59.03% -0.97%
==========================================
Files 2290 2201 -89
Lines 190586 180294 -10292
==========================================
- Hits 114344 106436 -7908
+ Misses 66052 64504 -1548
+ Partials 10190 9354 -836
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — approving despite the inline nit, which the bug report itself notes doesn't block merge.
Extended reasoning...
Overview
This PR adds new benchmark tooling (sei-db/state_db/bench/writeset.go, writeset_convert.go, writeset_bench_test.go, writeset_test.go) that captures a debug_traceCall prestateTracer diff and replays it as a write-set against both the memiavl and FlatKV storage backends, timing ApplyChangeSets/Commit separately. All four files are new; nothing in production code paths (x/evm, consensus, RPC serving) is modified.
Security risks
None of substance. os.ReadFile on operator-supplied paths is annotated //nolint:gosec appropriately (benchmark-only, local operator input, not attacker-controlled network input). No auth, crypto, or permission logic is touched. Hex decoding is bounds-checked via decodeHexField.
Level of scrutiny
Low-to-moderate. This is opt-in benchmark/test tooling gated behind environment variables (TRACE_PATH/WRITESET_PATH), not wired into any production binary, RPC handler, or consensus path. Correctness bugs here affect benchmark operators, not chain safety or user funds.
Other factors
The one bug found (missing per-kind value-length enforcement in Validate()/BlockChangesets) only matters for the non-primary WRITESET_PATH hand-authored-file input; the documented main flow (TRACE_PATH → ConvertPrestateDiff) always emits correctly-sized values and is unaffected. Test coverage is solid: converter fixtures (delete, code deployment, diffMode validation) and a round-trip replay+read-back test against both backends. The candidate issues raised by finder agents (storage-delete normalization, selfdestruct cleanup) were examined and refuted by verifiers. Given the tooling-only scope and the nit-level severity of the one confirmed finding, this does not need to block on human review.
…st measurement eth_call/debug_traceCall execute bytecode against a simulation StateDB whose writes are discarded, so they cannot measure the storage-commit cost of opcodes like SSTORE. This adds the capture-and-replay path the gas-repricing work needs: - writeset.go: write-set file format (per-block storage/code/nonce/codehash/ raw entries with deletes), parser/validator, and ReplayWriteSet, which replays one block per version timing ApplyChangeSets and Commit separately. - writeset_convert.go: converts a debug_traceCall prestateTracer diffMode result into a write set. Emits storage writes/deletes, nonce bumps (8-byte big-endian), and code deployments (code + keccak codehash + codesize), mirroring x/evm's key encodings. Balance changes are bank-module writes and are counted but not converted (v1 scope). - BenchmarkWriteSetReplay: replays TRACE_PATH or WRITESET_PATH against both memiavl and FlatKV with apply_ns/key and commit_ns/key metrics, plus optional SNAPSHOT_PATH pre-population. Verified end to end against a pacific-1 debug_traceCall capture. Co-authored-by: Cursor <cursoragent@cursor.com>
cadec99 to
cf05949
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cf05949. Configure here.
There was a problem hiding this comment.
A well-tested, self-contained benchmark tool (write-set capture/replay for opcode storage-cost measurement) that touches only test/bench code with no production impact. No blocking issues; a few measurement-accuracy caveats worth addressing so the reported numbers are interpreted correctly.
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. Codex's review was present and its three findings are incorporated below.
- Scope docs are clear that balance/bank writes are intentionally out of v1 scope, but code-removal (self-destruct: post.Code empty while pre.Code set) is silently not converted to a code/codehash delete. Fine for the SSTORE-focused v1 goal, but worth a one-line note in the converter's mapping-rules comment so the omission is explicit alongside the balance one.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| func BenchmarkWriteSetReplay(b *testing.B) { | ||
| ws := loadBenchWriteSet(b) | ||
|
|
||
| for _, backend := range []wrappers.DBType{wrappers.MemIAVL, wrappers.FlatKV} { |
There was a problem hiding this comment.
[suggestion] Caveat for the commit-cost comparison: memiavl is opened with AsyncCommitBuffer=10 (wrappers/db_implementations.go:44), so Commit() returns before the WAL is durably persisted, whereas FlatKV commits synchronously. The commit_ns/key numbers therefore compare different durability semantics across the two backends. The config is preexisting/shared, but since this benchmark's whole point is a cross-backend commit-cost comparison, it's worth either setting AsyncCommitBuffer=0 for this run or documenting the asymmetry so results aren't read as apples-to-apples.
| }) | ||
| } | ||
|
|
||
| if post.Code != "" && post.Code != pre.Code { |
There was a problem hiding this comment.
[suggestion] Code deployment omits the address-mapping writes that Keeper.SetCode performs for previously-unassociated contracts (x/evm/keeper/code.go:34-36 calls SetAddressMapping, writing both the Sei→EVM and EVM→Sei mapping entries). convertCode emits only code + codehash + codesize, so a deploy write-set undercounts the real storage work of a deployment. Either emit the mapping writes (as raw entries) or document that they're excluded like balance changes are.
| if keys == 0 { | ||
| return | ||
| } | ||
| b.ReportMetric(result.ApplyDuration.Seconds()/keys*1e9, "apply_ns/key") |
There was a problem hiding this comment.
[suggestion] ReportMetric is called once per b.N iteration (via runWriteSetReplay inside the for range b.N loop) with a per-single-replay value that isn't normalized by b.N. Go reports the last value for a given unit, so with e.g. -benchtime=5x only the 5th iteration's apply_ns/key/commit_ns/key is shown rather than an aggregate across samples. Consider accumulating across iterations and reporting the average once, or divide by the iteration count.
There was a problem hiding this comment.
Adds a well-documented, well-tested write-set capture-and-replay benchmark adapter under sei-db/state_db/bench (test/benchmark-only, non-app-hash-breaking). No blocking issues; two fidelity/robustness notes carried over from Codex are worth addressing but do not block merge.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor produced no review output (cursor-review.md is empty) and the base-branch REVIEW_GUIDELINES.md is also empty, so this synthesis reflects only Codex's pass plus a direct read of the diff.
- Scope is clearly benchmark/test-only tooling and the file headers already document the main v1 fidelity gaps (bank/balance writes, Sei<->EVM address-mapping on deploy). The two findings below extend that same fidelity-gap discussion rather than indicating production risk.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // when pre and post encode the same slot differently (padded vs unpadded, 0x | ||
| // prefix, case), emitting a spurious delete that clobbers the write — deletes | ||
| // are appended after writes, and both engines apply last-write-wins per key. | ||
| for _, addr := range sortedKeys(diff.Pre) { |
There was a problem hiding this comment.
[suggestion] Accounts present in pre but absent from post (e.g. removed via SELFDESTRUCT) only get storage deletions here — their nonce, code, codehash, and codesize (0x09) keys are never deleted. Replayed state and the measured apply/commit cost for account-destruction workloads are therefore incomplete. Consider emitting deletes for those non-storage keys too, or at minimum documenting this alongside the existing v1 fidelity-gap note in the file header (which currently only calls out balance and address-mapping gaps).
| WriteKindCodeHash: keys.EVMKeyCodeHash, | ||
| }[w.Kind] | ||
| return keys.BuildEVMKey(kind, addr), nil | ||
| case WriteKindRaw: |
There was a problem hiding this comment.
[suggestion] raw entries bypass all value width validation (valueLenForKind returns 0 for raw). A hand-authored write-set whose raw key targets an optimized FlatKV prefix (e.g. nonce 0x02 or storage 0x03) with a wrong-width value would be silently accepted by memiavl but hard-rejected by FlatKV inside ApplyChangeSets, breaking the same-write-set-across-backends premise of the benchmark. The converter itself only emits raw for the legacy 0x09 codesize prefix (safe), so this only affects hand-authored inputs — consider validating raw values against the target prefix, or documenting that raw entries must target legacy (non-width-checked) key families.
There was a problem hiding this comment.
LGTM — the two remaining findings are both nits (self-destruct account-level deletes, a doc/code mismatch for WRITESET_PATH), and prior review feedback (value-length validation, slot-encoding-mismatch delete normalization, per-iteration ReportMetric averaging) has already been addressed with accompanying tests in this revision.
Extended reasoning...
Overview
This PR adds a benchmark-only write-set replay adapter under sei-db/state_db/bench: a write-set JSON format with per-block storage/code/nonce/codehash/raw entries and delete support (writeset.go), a converter from debug_traceCall prestateTracer diffMode output into that format (writeset_convert.go), and a BenchmarkWriteSetReplay harness that replays the write set against both memiavl and FlatKV backends, timing ApplyChangeSets and Commit separately (writeset_bench_test.go). None of this touches production/consensus code paths — it's purely a local dev/benchmarking tool.
Security risks
None of consequence. Inputs are operator-supplied local files (trace JSON or write-set JSON) read via os.ReadFile in a benchmark binary, not user-facing or network-facing code. There's no auth, crypto, or permission logic involved.
Level of scrutiny
This warrants a light-to-moderate scrutiny level: it's benchmark tooling (v1-scoped, per the file's own header comments), not chain logic that affects app hash or state transitions (the PR is correctly labeled non-app-hash-breaking). Correctness bugs here affect measurement fidelity, not correctness of the chain itself. That said, I did check that per-kind value-length validation and delete/write ordering are correct, since a bug in either would silently produce misleading benchmark numbers rather than a loud failure.
Other factors
This is a second revision responding to prior review comments (mine and others'): the WriteSet.Validate/BlockChangesets value-length gap I flagged previously is now fixed via valueLenForKind with dedicated tests (TestValidateRejectsWrongLengthValue); the storage-delete slot-encoding mismatch cursor[bot] flagged is now fixed via padTo32 normalization on both sides with a regression test (TestConvertPrestateDiffNoSpuriousDeleteOnEncodingMismatch); and the ReportMetric-inside-b.N-loop issue seidroid flagged is now fixed by accumulating across iterations and reporting once. The two remaining findings from this run (missing account-level deletes for self-destructed accounts, and a doc/code mismatch for WRITESET_PATH) are both explicitly scoped as nits by the bug-hunting system, consistent with the PR's own documented v1 scope limitations (e.g. the balance-skip and address-mapping gaps are already called out in the file header). Test coverage is solid: converter fixtures use a real pacific-1 trace, plus dedicated delete/code-deployment/diffMode-validation/encoding-mismatch cases and a replay round-trip with read-back on both backends.
There was a problem hiding this comment.
Additive, benchmark-only write-set replay adapter under sei-db/state_db/bench/ with no production/consensus impact, solid test coverage, and well-documented scope limitations. The one substantive gap is that whole-account (self-destruct) deletions are undercounted; no blocking issues.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Verified the key encodings the converter mirrors (storage 0x03, nonce 0x0a, code 0x07, codehash 0x08, codesize 0x09) against sei-db/common/keys/evm.go — they match, and the storage-key length assertions in the tests are correct.
- The Cursor second-opinion review (cursor-review.md) was empty/produced no output; only the Codex pass produced a finding.
- REVIEW_GUIDELINES.md is empty, so no repo-specific standards were applied beyond AGENTS.md conventions.
- No prompt-injection or suspicious instruction content was found in the PR diff, title, or description.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // when pre and post encode the same slot differently (padded vs unpadded, 0x | ||
| // prefix, case), emitting a spurious delete that clobbers the write — deletes | ||
| // are appended after writes, and both engines apply last-write-wins per key. | ||
| for _, addr := range sortedKeys(diff.Pre) { |
There was a problem hiding this comment.
[suggestion] Whole-account deletions (self-destruct) are undercounted. When an account is absent from post, chain cleanup deletes its code, codehash, codesize, nonce, and ALL storage — including slots the tracer never read — but this delete pass only emits deletes for storage slots that happen to appear in pre, and never emits code/codehash/codesize/nonce deletes. So replaying a trace that includes a self-destruct materially undercounts the apply/commit cost being measured, silently. Since the tool already documents other fidelity gaps (balance changes, address mapping) in the file header, consider either handling account removal explicitly or at least noting this gap there so operators know the numbers are a lower bound for such traces. (Matches Codex P2.)
There was a problem hiding this comment.
Beyond the two inline nits from this run, I also re-examined cursor's earlier concern that convertStorage writes zero-valued slots as a plain write instead of a delete (since Keeper.SetState deletes on an all-zero value) — this run's verifiers ruled it out as not a functional bug for the benchmark's purposes.
Extended reasoning...
This is an informational note, not a new review pass: no bugs beyond the two inline nits (AsyncCommitBuffer durability asymmetry, raw-entry value-length validation gap) were found this run. Cursor's prior 'zero storage emits write not delete' comment (Medium severity, still open in the thread) was re-examined by this run's finder/verifier agents and refuted, so I'm recording that here to avoid it being re-explored from scratch in a future pass.
…fixes - Open memiavl with AsyncCommitBuffer=0 in OpenReplayWrapper so commit_ns/key compares synchronous WAL writes on both backends instead of memiavl's async enqueue vs FlatKV's synchronous write (seidroid/claude). - Convert accounts present in pre but absent from post (SELFDESTRUCT shape) into nonce/code/codehash/codesize deletes, count their balance zeroing in SkippedBalanceChanges, and document that pre-only slots bound the delete volume (codex/seidroid/claude). - Fix the WRITESET_PATH doc comment: raw tracer output goes through TRACE_PATH only (claude). - Document that raw entries bypass width validation and must target non-width-checked key families (claude). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A well-tested, well-documented benchmark-only write-set replay adapter for storage-cost measurement (non-consensus, non-app-hash-breaking). One fidelity gap worth noting: code that transitions to empty on an account still present in post-state is not converted to code deletes; otherwise the change is clean.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
REVIEW_GUIDELINES.mdandcursor-review.mdare both empty/missing, so no repo-specific guidelines and no Cursor second-opinion pass were available for this review. Codex contributed one P2 finding (the code-clearing gap, surfaced as an inline comment).- Fidelity gap (echoes Codex P2,
writeset_convert.go:124): the converter emits code writes/deletes only for deploy (post.Code non-empty & changed) and full account removal (SELFDESTRUCT). An account that remains in post-state but whose code becomes empty — e.g. EIP-7702 delegation clearing — produces no code/codehash/codesize delete, so the replay retains stale code and undercounts writes. Given this tooling supports gas-repricing storage work where EIP-7702 is relevant, consider handlingpre.Code != "" && post.Code == ""(emit deletes) and documenting it alongside the other scope notes. Non-blocking since this is offline benchmark tooling, not production code. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| }) | ||
| } | ||
|
|
||
| if post.Code != "" && post.Code != pre.Code { |
There was a problem hiding this comment.
[suggestion] This branch only emits code writes when post.Code is non-empty and changed. The sole code delete path is convertAccountRemoval, which fires only when the account is absent from post (SELFDESTRUCT). An account that stays in post-state but whose code transitions to empty (e.g. EIP-7702 delegation clearing → post.Code == "") is silently skipped, so the replay leaves the old code, codehash, and codesize keys intact and undercounts writes. Consider emitting code/codehash/codesize deletes when pre.Code != "" && post.Code == "" (and documenting it with the other scope notes in the file header). Non-blocking — this is offline benchmark tooling.
There was a problem hiding this comment.
LGTM — the review nits raised across earlier rounds (value-length validation, storage-delete key normalization, memiavl/FlatKV commit-timing asymmetry, self-destruct account-key deletes, and the WRITESET_PATH doc mismatch) have all been addressed in the latest commits, and this pass found no new issues.
Extended reasoning...
Overview
This PR adds a write-set capture-and-replay adapter for the sei-db benchmark harness (writeset.go, writeset_convert.go, writeset_bench_test.go, plus a small config-plumbing change in wrappers/db_implementations.go). It lets a debug_traceCall+prestateTracer diff be converted into a write set and replayed against memiavl and FlatKV to measure per-opcode storage-commit cost. This is benchmark tooling only — it does not touch consensus, RPC serving, or any production write path.
Security risks
None relevant to production: the code only runs under go test -bench, reads operator-supplied local files (trace/write-set/snapshot paths), and writes to a b.TempDir(). No user input, network exposure, or auth/crypto surface.
Level of scrutiny
Low-to-moderate is appropriate given the benchmark-only blast radius, but the PR went through multiple review rounds (Cursor Bugbot, Codex, seidroid, and my own prior passes) that surfaded real correctness issues in the write-set validation and conversion logic. I verified via git log that each of those — the missing per-kind value-length check in Validate(), the storage-delete key-encoding mismatch, the memiavl async-commit-buffer vs. FlatKV sync-commit asymmetry skewing commit_ns/key, the missing self-destruct account-key deletes, and the WRITESET_PATH doc/behavior mismatch — were fixed in subsequent commits (cf05949, 0684ce1, 9ad22eb), with the fixes matching the reported root causes.
Other factors
Test coverage is solid for this kind of tooling: writeset_test.go covers the self-destruct deletion case, the padded-slot delete/write ordering edge case, code deployment, and value-length validation, and includes an actual round-trip replay against both backends. The two remaining minor scope gaps (address-mapping writes omitted on fresh deploys; raw entries bypassing width checks) are explicitly documented in the code as known v1 limitations rather than silent gaps, which is a reasonable tradeoff for a benchmarking tool. No new bugs were found in this pass.
1 participant
Summary
eth_call/debug_traceCalldiscard writes, so they can't measure the storage-commit cost of opcodes likeSSTORE. This PR adds capture-and-replay for the gas-repricing work: capture an opcode's exact write set viadebug_traceCall+prestateTracer(diff mode), then replay the identical write set against both memiavl and FlatKV, timingApplyChangeSetsandCommitseparately.writeset.go: write-set format (per-block storage/code/nonce/codehash/raw entries, deletes) +ReplayWriteSet.Validateenforces FlatKV's fixed value widths up front so both backends always replay the same data. memiavl opens withAsyncCommitBuffer=0socommit_ns/keycompares synchronous WAL writes on both sides.writeset_convert.go: prestateTracer diff → write set. Handles storage writes/deletes (slot encoding normalized), nonce bumps, code deploys (code/codehash/codesize), and SELFDESTRUCT account-removal deletes. Balance changes are bank-module writes: counted, not converted (v1 scope).writeset_bench_test.go:BenchmarkWriteSetReplayreplaysTRACE_PATHorWRITESET_PATHon both backends, reportingapply_ns/key/commit_ns/key; optionalSNAPSHOT_PATHpre-population.wrappers/db_implementations.go: MemIAVL factory accepts an optional*memiavl.Config(nil keeps the existing bench default).Does not depend on #3770 (FlatKV path passes an explicit config).
Follow-ups (not in this PR)
Usage
Test plan
go test ./sei-db/state_db/bench/...: converter fixtures from a real pacific-1 trace; delete/encoding-mismatch/self-destruct/code-deploy/validation cases; replay round-trip + read-back on both backendsSSTOREbytecode traced on pacific-1, replayed on memiavl and FlatKVgofmt -s/goimportsclean