feat(seidb-bench): op-type sweep for insert/update/delete storage cost

[blindchaser]

Summary

Measurement substrate for the insert-vs-update-vs-delete storage-cost question raised in the gas-repricing discussion (Jeremy's new-key premium, the delete-refund idea). Builds on the write-set replay adapter (#3772).

• ReplayWriteSetSampled — applies+commits every block but times only blocks past a warm-up prefix, capturing one apply/commit sample per timed block so callers get p50/p95/p99 instead of a single mean.
• GenerateOpWriteSet — builds insert/update/delete scenarios over deterministic EVM storage keys. update/delete seed the exact keys in a warm-up (untimed) block so a timed block operates entirely on pre-existing keys; insert writes fresh keys.
• TestOpTypeSweep — env-gated (OP_SWEEP_CSV), skipped in CI; runs the grid on memiavl and FlatKV and writes a CSV of per-key apply/commit percentiles. Grid tunable via OP_SWEEP_KEYS_PER_BLOCK / OP_SWEEP_BLOCKS.

Scope / caveats

• Empty-DB runs measure the relative op asymmetry only. Absolute (gas-calibration) numbers require running against a copy of a mainnet-sized data directory — a cluster-side follow-up (wiring the opener to the real memiavl/flatkv paths).
• memiavl uses the synchronous-WAL replay config (from feat(seidb-bench): add write-set replay adapter for opcode storage-cost measurement #3772's OpenReplayWrapper) so commit timings are comparable to FlatKV.

Test plan

• go test ./sei-db/state_db/bench -run 'TestGenerateOpWriteSetShapes|TestReplayWriteSetSampledSkipsWarmup|TestPercentile'
• end-to-end sweep: OP_SWEEP_CSV=/tmp/opsweep.csv OP_SWEEP_KEYS_PER_BLOCK=50 OP_SWEEP_BLOCKS=10 go test ... -run TestOpTypeSweep produces a valid CSV on both backends
• gofmt -s / goimports clean

Made with Cursor

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Jul 24, 2026, 2:58 AM

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1277b829d

ℹ️ 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".

[chatgpt-codex-connector]

Use non-zero storage values in the sweep

When the op sweep runs for storage, this constructs every non-delete write as 32 zero bytes. In FlatKV, an all-zero storage value is interpreted as a deletion (StorageData.IsDelete() returns true and the commit path issues Delete), so insert never inserts slots and the update/delete warm-up block does not create the pre-existing keys those scenarios are intended to measure. This makes the generated CSV measure deletes/no-ops instead of insert/update/delete storage cost; seed and timed writes should use a non-zero storage value (and preferably a changed value for updates).

Useful? React with 👍 / 👎.

[seidroid]

A well-scoped, well-documented benchmark for insert/update/delete storage costs, but the write-set generator uses an all-zero storage value for every non-delete write, which FlatKV interprets as a deletion — collapsing all three scenarios to deletes on that backend and invalidating the core comparison this PR is built to produce.

Findings: 1 blocking | 4 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output; only the Codex pass contributed external findings, all of which are reflected here.
• Codex's High finding overlaps with the inline blocker below; the confirmed root cause is FlatKV's StorageData.IsDelete() treating an all-zero 32-byte value as a deletion (vtype/storage_data.go:111), which is stronger/more concrete than 'unchanged writes may be suppressed upstream'.
• 2 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] Using an all-zero value for every non-delete write invalidates the benchmark on FlatKV. FlatKV's StorageData.IsDelete() (sei-db/state_db/sc/flatkv/vtype/storage_data.go:111) returns true whenever the 32-byte value is all zeros, and prepareBatch then emits a batch.Delete(key) instead of a Set. Consequently on FlatKV: insert deletes fresh (absent) keys; update's warm-up block stores nothing (its writes are treated as deletes) so the timed blocks delete absent keys too; and delete deletes already-absent keys. All three ops collapse to essentially the same delete operation, so the insert-vs-update-vs-delete cost comparison — the entire purpose of this PR — is meaningless on FlatKV (and on memiavl update overwrites with an identical value rather than a changed one). Derive a non-zero, deterministic value per key (e.g. from the same index used for addr/slot) for non-delete writes so inserts/updates actually persist a slot.

[seidroid]

[suggestion] warmupBlocks is not validated. A negative value makes i >= warmupBlocks always true so warm-up blocks get sampled, and a value >= len(ws.Blocks) yields timed <= 0, silently returning empty ApplyNs/CommitNs (percentile then reports 0) with no error. Reject warmupBlocks < 0 || warmupBlocks >= len(ws.Blocks) up front so misuse fails loudly rather than producing misleading zero percentiles.

[seidroid]

[nit] defer w.Flush() doesn't check w.Error(). A disk-full or deferred write failure would be swallowed, yet the test still prints [OpSweep] wrote <path> and passes, leaving a truncated CSV that looks complete. Check w.Error() after the final flush (e.g. w.Flush(); require.NoError(t, w.Error())).

[claude]

🔴 GenerateOpWriteSet always fills non-delete write values with 32 zero bytes (hex.EncodeToString(make([]byte, spec.ValueBytes))), but FlatKV's StorageData.IsDelete() treats any all-zero 32-byte value as a deletion and prepareBatch issues a batch.Delete for it regardless of the entry's Delete flag. As a result, every FlatKV insert/update write in this benchmark — including the update/delete warm-up seed — is silently executed as a delete, invalidating the FlatKV insert/update numbers that are this PR's primary deliverable. Fix by using a non-zero value (e.g. the deterministic addr/slot bytes) for non-delete writes.

Extended reasoning...

GenerateOpWriteSet (opsweep.go:93) builds the value for every non-delete write as hex.EncodeToString(make([]byte, spec.ValueBytes)), i.e. 32 zero bytes, and this same zero value is used both for the OpInsert/OpUpdate timed writes and for the warm-up block that seeds keys ahead of the OpUpdate/OpDelete scenarios (opsweep.go:93-101).

On the FlatKV backend this collides with an existing invariant: vtype.StorageData.IsDelete() (sei-db/state_db/sc/flatkv/vtype/storage_data.go:109-121) defines a storage entry as a deletion whenever all 32 value bytes are zero — this mirrors real EVM SSTORE-to-zero semantics, independent of any explicit delete flag on the write-set entry. store_write.go's prepareBatch (lines 274-286) then checks w.IsDelete() and calls batch.Delete(key) instead of batch.Set(key, w.Serialize()) whenever that holds.

The consequence is that on FlatKV, none of the "insert" or "update" writes generated by this benchmark are actually persisted as writes — they are all executed as deletes:

1. Insert scenario: every timed write has Delete: false but value = 32 zero bytes → FlatKV treats it as IsDelete() == true → batch.Delete is issued against a key that was never present. The "insert" benchmark is actually measuring delete-of-nonexistent-key cost, not insert cost.
2. Update scenario: the warm-up block seeds the keys with the same all-zero value, so the warm-up itself is also converted to batch.Delete on FlatKV — the keys the scenario claims to pre-seed are never actually created. The subsequent timed "update" writes (also all-zero) are therefore also deletes of nonexistent keys, not updates of existing ones.
3. Only the OpDelete scenario happens to be correct, since its writes are already intended as deletes.

memiavl is unaffected because (per the comment in writeset.go's valueLenForKind) it stores raw bytes with no zero-value tombstone special-casing, so it accepts the all-zero value as an ordinary write. This means the two backends silently diverge in what they're actually measuring, and the FlatKV insert/update percentiles in the resulting CSV — the empirical input this PR exists to produce for the insert-vs-update-vs-delete gas-repricing question — are wrong without any error or panic to flag it.

Step-by-step proof:

• spec.ValueBytes = 32 → value := hex.EncodeToString(make([]byte, 32)) = "0000...00" (64 hex chars, all zero bytes).
• mkWrite(k, false) sets e.Value = value, e.Delete = false — this is used for the OpUpdate warm-up seed and for OpInsert/OpUpdate timed writes.
• WriteSet.BlockChangesets decodes e.Value back to 32 zero bytes and builds a KVPair{Key: ..., Value: []byte{0,0,...,0}, Delete: false}.
• On the FlatKV apply path, processStorageChanges parses this into a StorageData whose 32 value bytes are all zero via ParseStorageValue/SetValue.
• StorageData.IsDelete() iterates s.data[storageValueStart:storageDataLength] and returns true since every byte is zero — this check has no knowledge of the original Delete: false flag on the write-set entry.
• prepareBatch sees w.IsDelete() == true and calls batch.Delete(key), not batch.Set.
• Net effect: the "insert" and the warm-up "seed" writes for "update" never create a key in the FlatKV store at all; they're deletes of a key that was never there. The subsequent timed "update" write is therefore also operating on a nonexistent key, i.e. it's also really a delete-of-nonexistent-key, not an update.

The existing unit test TestReplayWriteSetSampledSkipsWarmup only asserts require.Positive(t, ns) on the timing samples, which a delete-of-nonexistent-key satisfies just as well as a real update would, so it does not catch this.

Fix: use a non-zero value for non-delete writes — e.g. a distinct sentinel byte pattern, or better, the deterministic addr/slot bytes already being generated for the key itself (guaranteed non-zero and unique per key), so insert/update measure real Set operations on FlatKV.

[claude]

🟡 The doc comment above type keyCursor struct{ next uint64 } refers to a nonexistent identifier keyIndexStride instead of keyCursor — a stale/copy-pasted comment from an earlier refactor. Purely cosmetic (Go convention is that a doc comment leads with the declared name), no behavioral impact; worth a quick fix to avoid confusing future readers.

Extended reasoning...

What's wrong

In sei-db/state_db/bench/opsweep.go, the doc comment directly above type keyCursor struct{ next uint64 } reads:

// keyIndexStride keeps generated addresses/slots from colliding across blocks
// and scenarios: each write gets a globally unique index.
type keyCursor struct{ next uint64 }

The comment names keyIndexStride, but the type it documents is keyCursor. There is no identifier named keyIndexStride anywhere else in the file (or in the rest of the PR diff) — grepping confirms it only appears in this one comment. This is almost certainly a leftover from an earlier draft/refactor where the type had a different name, and the rename to keyCursor was not propagated to its doc comment.

Why it matters (mildly)

Go convention is that a doc comment for a declaration should begin with the name of the thing it documents, specifically so that go doc output and IDE tooltips read coherently. Here, anyone running go doc on this package, or hovering over keyCursor in an editor, sees a comment that talks about a name that does not exist in the codebase — mildly confusing, and a paper cut for anyone extending this benchmark file later (e.g. wondering if keyIndexStride was renamed/removed and whether the described behavior still applies).

Step-by-step proof

1. Open sei-db/state_db/bench/opsweep.go at the type declaration (lines 44-46 in the current file).
2. Read the doc comment: // keyIndexStride keeps generated addresses/slots from colliding across blocks and scenarios: each write gets a globally unique index.
3. Read the declaration immediately below it: type keyCursor struct{ next uint64 }.
4. grep -n keyIndexStride sei-db/state_db/bench/opsweep.go returns only the comment line — no struct, function, variable, or constant of that name exists anywhere in the file or its test file.
5. The described behavior ("keeps generated addresses/slots from colliding ... each write gets a globally unique index") does match keyCursor's actual role (its take() method returns a monotonically increasing index used to derive unique deterministic addresses/slots) — confirming this is the right type, just with a stale name in the comment.

Fix

Simply rename the comment's subject to match the type:

// keyCursor keeps generated addresses/slots from colliding across blocks
// and scenarios: each write gets a globally unique index.
type keyCursor struct{ next uint64 }

Why nothing else prevents this

This is benchmark-only tooling (sei-db/state_db/bench), gated out of CI (OP_SWEEP_CSV-gated test), and doc comments aren't type-checked by the Go compiler or covered by the repo's golangci-lint config for this kind of name-mismatch, so nothing catches it automatically. It has zero effect on the generated write sets, the replay logic, or the CSV output — purely a documentation/readability issue.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit e1aa153. Configure here.}

[cursor]

Real dir reused across ops

Medium Severity

In TestOpTypeSweep, when OP_SWEEP_MEMIAVL_HOME or OP_SWEEP_FLATKV_DIR is set, the same on-disk path is reused for insert, update, and delete in the inner loop. Each OpenReplayWrapper continues from the version and state left by the previous op’s replay, so update and delete timings are not measured from the same baseline the insert row used and no longer isolate a single op type on a fresh mainnet-sized snapshot.

 

^{Reviewed by Cursor Bugbot for commit e1aa153. Configure here.}

[seidroid]

Benchmark-only addition (env-gated test + helper, skipped in CI) that generates controlled insert/update/delete write sets and replays them with per-block sampling to derive percentiles. Code is clean and well-tested; only minor test-quality nits, no correctness or security concerns.

Findings: 0 blocking | 4 non-blocking | 2 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output.
• Minor: in the update scenario the warm-up block seeds keys with a 32-byte zero value and the timed blocks overwrite them with the same value. Overwriting with an identical value could let a backend elide the write; if the intent is to measure a genuine value change, consider using a distinct value in the timed blocks. Non-blocking for a relative-cost benchmark.
• 2 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] defer w.Flush() discards the flush error (and csv.Writer.Error() is never checked). A failed/partial flush of the buffered writer would let the test report success while leaving a truncated or invalid CSV. Consider replacing the deferred flush with an explicit w.Flush(); require.NoError(t, w.Error()) before f.Close(). (Also raised by Codex.)

[seidroid]

[nit] The PR description and the ReplaySamples doc comment state callers get p50/p95/p99, but the CSV header only emits p50 and p99. Add apply_p95_ns_per_key / commit_p95_ns_per_key columns (the samples support it), or adjust the stated contract to p50/p99. (Also raised by Codex.)

Superseded: latest AI review found no blocking issues.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.20%. Comparing base (1f1542e) to head (e1aa153).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3800      +/-   ##
==========================================
- Coverage   60.16%   59.20%   -0.96%     
==========================================
  Files        2327     2235      -92     
  Lines      194602   184102   -10500     
==========================================
- Hits       117076   109000    -8076     
+ Misses      66953    65382    -1571     
+ Partials    10573     9720     -853     

Flag	Coverage Δ
sei-db	70.41% <ø> (ø)
sei-db-state-db	?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 92 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

🟡 csv.NewWriter(f) buffers writes and defer w.Flush() discards the returned error, and w.Error() is never checked before the test exits. A disk-full or I/O failure during the deferred flush is silently swallowed, so the test can still print "[OpSweep] wrote " and pass while leaving a truncated/invalid CSV on disk.

Extended reasoning...

In TestOpTypeSweep (opsweep_test.go:127-129), the CSV writer is set up as:

w := csv.NewWriter(f)
defer w.Flush()

csv.Writer wraps a bufio.Writer internally, so individual w.Write(row) calls only append to an in-memory buffer — they don't necessarily hit the OS/disk until Flush runs. Flush does return an error, but here it's deferred, so the return value is discarded entirely (Go silently drops the return value of a deferred call unless you capture it). The only way to observe a flush failure afterward is to call w.Error(), which returns the first error encountered by Write or Flush — and that is never called anywhere in the test.

Code path: the test loops over backends/ops, calls w.Write(row) for each row (each of which only buffers), and after the loop ends the function returns, running the deferred w.Flush() (before the deferred f.Close(), since defers run LIFO). If that flush hits an I/O error — disk full, quota exceeded, remote/network filesystem hiccup, permission race, etc. — the error is thrown away, and the very next line executed is fmt.Printf("[OpSweep] wrote %s\n", csvPath), which is unconditional. The test then returns without failing.

Why nothing else catches this: every other file operation in the test is checked (os.Create via require.NoError, f.Close() via require.NoError in its own defer, every individual w.Write(row) via require.NoError). The flush is the one buffered operation whose error is dropped, so the existing checks give a false sense of completeness — someone skimming the test would reasonably assume all I/O is already verified.

Impact: this is opt-in benchmark tooling gated behind OP_SWEEP_CSV and skipped in CI, so it can't affect the CI signal or production. But because it's the sole test in the PR that persists the sweep's actual deliverable (the CSV of insert/update/delete storage-cost percentiles), the impact when it does trigger is exactly the wrong kind of silent failure: an operator running the sweep against a real/large data directory sees "[OpSweep] wrote /path/to/out.csv" and PASS, then later loads a truncated or incomplete CSV into analysis without any indication something went wrong.

Step-by-step proof:

1. Assume the last few w.Write(row) calls succeed (buffered, not yet on disk) but the underlying volume then fills up or hits a transient I/O error.
2. The loop ends normally — no error was raised by Write, so require.NoError passed every time.
3. fmt.Printf("[OpSweep] wrote %s\n", csvPath) executes unconditionally.
4. The function returns; deferred calls run LIFO: w.Flush() runs first and returns a non-nil error, which is discarded (no variable captures it); then f.Close() runs and typically succeeds since the file descriptor itself is fine even if the flush failed to write all buffered bytes.
5. The test reports PASS with the success message already printed, but out.csv on disk contains fewer bytes than intended — a truncated/invalid row or missing trailing rows.

Fix: replace the deferred flush with an explicit flush-and-check before f.Close():

w.Flush()
require.NoError(t, w.Error())

This surfaces any buffered write/flush failure as a test failure instead of a silent success.