refactor(deps): replace RocksDB package - #12853
Conversation
|
Claude finished @rubo's task in 10m 27s —— View job Review:
|
EVM Opcode Benchmark DiffAggregated runs: base=1, pr=1 No significant regressions or improvements detected. |
|
@claude re-review. Ignore nix failures and unpinned package version, the latter is intentional.
Not true. The feed is public, just not the production one. See The following findings are rejected:
|
|
Claude finished @rubo's task in 7m 41s —— View job Re-review:
|
| # | Status |
|---|---|
| 1 Nix deps | excluded per your instruction |
2 Code Lint IDE0005 |
✅ fixed — Code Lint green |
| 3 buffer guard | ✅ withdrawn |
| 4 pinned slice | ✅ withdrawn |
| 5 test coverage | ✅ addressed — two new tests cover the empty-value round trip and the undersized buffer |
| 6 version pin | intentional, dropped |
7 HyperClockCacheWrapper |
mostly resolved; see the new finding below |
8 dead cf parameter |
✅ dropped |
9 AllowUnsafeBlocks |
✅ justification comment added |
| 10 asymmetric stats logging | ✅ else-warn added (nit inline) |
| 11 mixed iterator projections | ✅ both use GetValueSpan().ToArray() |
| 12 version log | ✅ confirmed |
| 13 ctor surface change | still a release-note item |
New findings
🟠 Medium — HyperClockCacheWrapper now throws on a zero capacity. The rewrite dropped the if (_capacity > 0) guard around GC.AddMemoryPressure, which throws ArgumentOutOfRangeException for bytesAllocated <= 0. Db.SharedBlockCacheSize (DbModule.cs:73) and FlatDb.BlockCacheSizeBudget (FlatRocksDbConfigAdjuster.cs:46,55) are both operator-settable ulongs, so 0 — the obvious way to disable the shared cache — goes from "works, pressure accounting skipped" on master to a startup crash out of the DI factory. No test covers capacity 0, so CI won't catch it. Details + suggestion · Fix this →
Same comment covers two smaller points: _disposed is a plain bool, so concurrent Dispose() can double-call GC.RemoveMemoryPressure (SafeHandle used to guarantee exactly-once); and there's no finalizer, so an undisposed wrapper never removes its pressure — harmless today since both call sites dispose, and the native handle is still covered by CacheHandle's critical finalizer.
🔵 Low — the new logger.Warn at DbMetricsUpdater.cs:46 is missing the if (logger.IsWarn) guard used elsewhere in this layer, and it fires every StatsDumpPeriodSec per updater (one per column family). Details
🔵 Low — src/Nethermind/Nethermind.Runner/packages.lock.json lost its trailing newline (\ No newline at end of file at the end of the diff). Regeneration artifact, worth restoring so the next dotnet restore --force-evaluate doesn't churn the line back.
🔵 Low — RepairDb is a new protected virtual reached from the constructor path (Init → RepairIfCorrupted), and RepairTrackingDbOnTheRocks overrides it using a primary-constructor parameter. That works only because Roslyn assigns primary-ctor captures before the base constructor call; a hand-written derived constructor assigning a field the normal way would observe null here. The existing // ReSharper disable once VirtualMemberCallInConstructor already flags the pattern, so this is a note rather than a request — it's a reasonable test seam and net less surface than the old Native injection.
Observation, not a finding — BuildOptions runs once per column family (DbOnTheRocks.cs:172), so with RowCacheSize > 0 on a ColumnsDb each call overwrites _rowCache and only the last Cache is disposed in ReleaseUnmanagedResources. On master this was an outright IntPtr leak; now the orphans are reclaimed by CacheHandle's critical finalizer, so the PR strictly improves it. Mentioning only because you're in the file.
CI
Code Lint, Code formatting, Build solutions, Integration tests (E2E), Stateless execution tests, Trivy, spell check, dependency review all ✅. Nix ❌ as expected (excluded). Flat DB tests, extra test variants, EF tests and the Hoodi sync gate were still running when I looked.
Manual-testing checklist (native v10 → v11)
Still the part the diff can't answer, unchanged from last round: codec parity (kSnappyCompression / kLZ4Compression / format_version=5 in Config/DbConfig.cs), native assets for all five RIDs in the lockfile, opening an existing v10 datadir under v11 and what a rollback to a v10 build does with v11-written SSTs, and a multi-hour sync watching native memory plus _allocatedSpan now that the FlushOptions / ReadOptions / row-Cache finalizer safety nets all changed shape.
Verdict: the migration itself holds up — I was wrong on both of the substantive findings, and the follow-up commit closed the rest. One Medium (zero-capacity crash) and three Lows remain; none is deep, and the Medium is a one-line guard.
· branch feature/rocksdb-bindings
|
@claude re-review, ignoring nix and unpinned package. Some context: A zero capacity makes rocksdb request a zero-length anonymous mapping for the Also make Dispose exactly-once via Interlocked, which SafeHandle used to |
Re-review:
|
| Point | Status |
|---|---|
| Zero capacity rejected as a configuration error | ✅ HyperClockCacheWrapper.cs:29-34, <exception>-documented, covered by SharedCacheRejectsZeroCapacity |
Dispose exactly-once via Interlocked |
✅ Interlocked.Exchange(ref _disposed, 1), bool → int; SharedCacheCanBeCreatedAndDisposed double-disposes |
Every DbMetricsUpdater log call guarded |
✅ all four Warn + the Error now guarded (lines 46, 52, 102, 163, 226) — one unguarded Info remains, see below |
Also confirmed while in there: Nethermind.Config (ExitCodes) and Nethermind.Db (IFlatDbConfig) both resolve for Nethermind.Db.Rocks, and Code Lint is green, so no repeat of the IDE0005 regression; packages.lock.json's trailing newline is restored; and DestroyReadOptions / RocksDbSharp are gone from the tree entirely (only a stale comment mentions the old package now).
Previous findings — status
| # | Status |
|---|---|
| 1 Nix deps | excluded per your instruction |
| 2–5, 8–12 | ✅ closed in earlier rounds (3 and 4 were my errors) |
| 6 version pin | intentional, excluded |
| 7 zero-capacity crash (last round's Medium) | ✅ fixed |
_disposed race |
✅ fixed |
unguarded Warn (last round's Low) |
✅ fixed |
packages.lock.json newline |
✅ fixed |
| 13 ctor surface change | still a release-note item (DbOnTheRocks' public ctor lost Native? rocksDbNative) |
RepairDb virtual-from-ctor |
informational, unchanged |
New findings — all Low
🔵 Three comments in ColumnsDb.cs went stale with this change. :167-168 still says "ReadOptions in RocksDbSharp has a finalizer but no IDisposable" — names the removed package, and the claim is now false (the bindings' ReadOptions is a SafeHandle, which is why line 260 can Dispose() it). :174-175 says each GetViewBetween ReadOptions is finalizer-only and "not addressed by this PR" — RocksdbSortedView.Dispose() now disposes it, and per AGENTS.md comments shouldn't reference the PR. Details
🔵 The guard rejects exactly zero — is zero the whole unsafe range? Cache.CreateHyperClock passes estimatedEntryCharge = 0, so rocksdb auto-sizes the handle table from capacity alone. FlatRocksDbConfigAdjuster.cs:46 computes BlockCacheSizeBudget * 0.3, so a small budget yields a handful of bytes; if the auto-sizing rounds down, a tiny-but-nonzero capacity hits the same mapping. The new test only proves 10 KiB clears the floor. Same comment also covers: the message renders bare key names (SharedBlockCacheSize) rather than the Db. / FlatDb. prefixes operators actually type, and it's worth one manual --Db.SharedBlockCacheSize 0 run to confirm exit code 107 actually reaches the process — the throw is wrapped in Autofac's DependencyResolutionException, and only EthereumStepsManager.TryUnwrapException unwraps that, on the step-construction path only. Details
🔵 logger.Info at DbMetricsUpdater.cs:213 is the one call left unguarded, and the priciest — LogMemoryProfile() runs ~7 + numLevels native GetProperty calls per tick before formatting. An early if (!logger.IsInfo) return; at line 178 skips the queries, not just the string. Pre-existing on master. Details
🔵 Test coverage stops at the wrapper. SharedCacheRejectsZeroCapacity pins the constructor, but not the operator-facing wiring. A case in the existing FlatRocksDbConfigAdjusterTests with BlockCacheSizeBudget = 0 would pin that the config path reports it as a configuration error rather than something Autofac-shaped.
CI
Build solutions, Code Lint, Code formatting, Stateless execution tests, Trivy, spell check, dependency review, Build tools, PR labeler — all ✅. Nix ❌ as expected (excluded). I couldn't build or run tests locally (dotnet build needs approval in this environment), so the compile/test signal here is CI's.
Manual-testing checklist (native v10 → v11)
Unchanged, and still the part the diff can't answer: codec parity (kSnappyCompression / kLZ4Compression / format_version=5 in Config/DbConfig.cs), native assets for all five RIDs in the lockfile, opening an existing v10 datadir under v11 and what a rollback to a v10 build does with v11-written SSTs, and a multi-hour sync watching native memory plus _allocatedSpan. Add to that: one deliberate zero-capacity startup to confirm the new error path prints cleanly and exits 107.
Verdict: mergeable from my side once Nix is regenerated on the production feed. The Medium from last round is fixed, the two smaller points with it, and nothing new above Low turned up — the four remaining items are comment hygiene, a floor question for the new guard, one pre-existing log guard, and an optional test.
View job run · branch feature/rocksdb-bindings
|
@claude review ignoring nix and unpinned package |
|
Claude finished @rubo's task in 9m 13s —— View job Re-review:
|
| # | Status |
|---|---|
| 1 Nix deps | excluded per your instruction |
| 2–5, 8, 10–12 | ✅ closed in earlier rounds (3 and 4 were my errors) |
| 6 version pin | intentional, excluded |
7 zero-capacity crash + _disposed race |
✅ fixed in b1fcaed |
9 AllowUnsafeBlocks |
✅ better than asked — the unsafe is gone |
stale ColumnsDb comments |
✅ fixed |
| zero-capacity floor question | ✅ answered by the budget = 2 test case |
unguarded Info |
❌ my error, withdrawn |
packages.lock.json newline |
cc14ece, see below |
| 13 ctor surface change | still a release-note item (DbOnTheRocks' public ctor lost Native? rocksDbNative) |
RepairDb virtual-from-ctor |
informational, unchanged |
New findings — both Low
🔵 packages.lock.json lost its trailing newline again. cc14ece re-introduced \ No newline at end of file; it had been restored last round. Rides along with the regeneration that flips the feed. Details
🔵 Eight per-DB native option handles are still finalizer-only. ReleaseUnmanagedResources now disposes _db, _iteratorManager, _rowCache and _reader, but WriteOptions, _noWalWrite, _lowPriorityWriteOptions, _lowPriorityAndNoWalWrite, _defaultReadOptions, _hintCacheMissOptions, _readAheadReadOptions and DbOptions are left to their critical finalizers. Identical to master in effect (no leak — SafeHandle reclaims them), so this is purely a consistency note: they're all IDisposable now, and disposing them after _db.Dispose() would finish the deterministic-cleanup theme the rest of the PR follows. Entirely optional.
CI (all on 66fea0c)
Build solutions, Build tools, Code Lint, Code formatting, Integration tests (E2E), Nethermind tests (Flat DB), Nethermind extra test variants, Nethermind/Ethereum tests, Stateless execution tests, EVM Opcode Benchmark Diff, Sync PR Gate (Hoodi), spell check, dependency review, PR labeler — all ✅.
Two ❌:
- Nix — excluded per your instruction.
- Trivy scanner — the
Scanstep succeeded; onlyUpload scan resultsfailed, with##[error]No server is currently available to service your request(GitHub code-scanning outage at 17:47). Transient infra, not this PR — a rerun should clear it.
Worth noting the EXPB benchmark signal is still missing: the "No metrics were produced" comment and the failed Run EXPB Reproducible Benchmarks / Publish Docker image runs were all triggered on f4122b0, i.e. the commit that didn't compile. Since the PR carries a performance is good label and lists Optimization as a change type, re-triggering EXPB on 66fea0c is the cheapest way to get an actual number.
Manual-testing checklist (native v10 → v11)
Unchanged and still the part the diff can't answer: codec parity (kSnappyCompression / kLZ4Compression / format_version=5 in Config/DbConfig.cs), native assets for all five RIDs in the lockfile, opening an existing v10 datadir under v11 and what a rollback to a v10 build does with v11-written SSTs, a multi-hour sync watching native memory plus _allocatedSpan, and one deliberate --Db.SharedBlockCacheSize 0 startup to confirm the new error prints cleanly and exits 107 (the throw is still wrapped in Autofac's DependencyResolutionException, and the new adjuster test calls GetForDatabase directly, so it doesn't cover the container path).
Verdict: mergeable from my side once Nix is regenerated on the production feed. Everything from the previous rounds is closed, one of my own Lows was wrong, and the two remaining items are a missing newline and an optional consistency cleanup.
· branch feature/rocksdb-bindings
|
@claude review, ignoring nix, unpinned version, and new line in packages.lock.json (it's autogenerated) On your last finding
Doing it would be a correctness risk for no benefit. Rejected. Forward- and backward-compatibility between v10 and v11 has been tested successfully. |
|
Claude finished @rubo's task in 3m 42s —— View job Re-review:
|
| Item | Status |
|---|---|
| Nix deps regeneration | excluded (rides along with the production-feed flip) |
| Version pin | intentional |
packages.lock.json newline |
autogenerated, ignore |
| Eight finalizer-only option handles | ✅ withdrawn — rejection is correct |
DbOnTheRocks public ctor lost Native? rocksDbNative |
release-note item for out-of-tree subclasses (net surface reduction, no objection) |
RepairDb virtual-from-ctor |
informational |
CI — all on the merge commit 649da20
Build solutions, Build tools, Code Lint, Code formatting, Integration tests (E2E), Nethermind tests (Flat DB), Nethermind extra test variants, Nethermind/Ethereum tests, Stateless execution tests, Trivy, spell check, dependency review, PR labeler — ✅. Nix ❌ (excluded). EVM Opcode Benchmark Diff and Sync PR Gate (Hoodi) still running.
Two notes on the perf signal, since the PR carries performance is good:
- The earlier MULMOD +360% comment is noise, not a regression — the bot itself listed MULMOD under "Noisy rerun opcodes", PR CV is 19% against a 1.1% base and the uncertainty band (±53%) swamps the ±5% threshold. A pure-EVM opcode has no path through this diff.
- EXPB still has no numbers. The "No metrics were produced" comment came from a run triggered on
f4122b0, the commit that didn't compile (WriteOptionslackedSetDisableWalbeforepreview.71). Re-triggering on649da20is the cheapest way to get a real figure for an Optimization-labelled PR.
Verdict: mergeable from my side once Nix is regenerated on the production feed. Nothing new this round, the last open finding was correctly rejected, and the v10↔v11 compat result retires the riskiest untestable item.
· branch feature/rocksdb-bindings
Benchmark summary — EXPB + RPC (eth_call corpus, EthCallChaos, multicall)Tested:
This is three changes at onceBinding layer + native engine 10.10.1 → 11.8.1 + RocksDB's allocator glibc → jemalloc. The numbers say "this is faster", not "jemalloc bought us X". jemalloc is confirmed live, statically linked and internalized (0 of 1749 exported dynsyms are allocator entry points → serves RocksDB only, does not interpose the process allocator). Verified via RocksDB itself: with Correctness — clean
EXPB (n=3 paired, flat)
9/9 pairs favour the branch on AVG (sign test p=0.004), in both arm orders. An earlier campaign on the previous baseline gave −2.2 / −4.4 / −9.2% AVG — reproduced, marginally larger. eth_call
A warm-only eth_call A/B reports "neutral" here — the effect lives in the cold path. Disable the warmup cell to see it. multicall (order-balanced, both arm orders) — updated with the re-run on the current baseline
Per-scenario on the current baseline @250 rps: high-gas#1/#2/#3 −1.3 / −2.8 / −1.2%, large#1 +0.1%, med#1 −3.3%, small#1 −1.3%, small#2 −4.1%. The earlier −8% at 250 rps came from the pre-refactor image; on the current baseline the multicall effect is neutral-to-slightly-better, not a win. Both arms moved (master @250 high-gas#1 1626.9 → 1586.9 ms, branch 1540.4 → 1565.3), and per-order spreads are wide today (e.g. high-gas#1 @100: +5.8% / −9.2%), so position noise dominates an effect of this size. Scenarios under ~2 ms ( Shape of the winMonotonic in how much DB work the request does: warm eth_call ~0 → cold eth_call −2% → fusaka −3% → realblocks −5% → saturated multicall −8% → superblocks −11%. Consistent with the allocator/engine acting on the RocksDB read + compaction path rather than on EVM execution. Worth resolving before merge
|
Changes
Replaces the
RocksDBpackage with our in-house Nethermind.RocksDbBindings, and moves from RocksDB v10 to v11.New interop
[assembly: DisableRuntimeMarshalling]: blittable signatures, no marshalling stubs, noNative.Instancesingleton hop per call.SafeHandle. Children (iterators, snapshots, checkpoints, WAL iterators) hold a ref-counted lease on the database handle, so a leaked iterator can no longer keep the DB open and its LOCK held.HandleLease, closing the check-then-use races the old code hadGone
RocksDbExtensions.cs, and all hand-rolledrocksdb_get/rocksdb_free/ pinnable-slice interop.unsafeblocks inDbOnTheRocks, bothMarshal.StringToHGlobalAnsi/FreeHGlobalpairs, and the privateFlushOptionsfinalizer class.AllowUnsafeBlocksfromNethermind.Db.Test.DbOnTheRocks'Native? rocksDbNativeconstructor parameter: source-breaking for out-of-tree subclasses; repair is now aprotected virtual RepairDb.Correctness
GetSpanhanded back a zero-length span over a liverocksdb_getallocation that no caller ever released; the bindings free it.Db.SharedBlockCacheSize=0is now a config error. It used toabort()inside RocksDB.WarmupFiletolerates null live-file metadata;HyperClockCacheWrapper.GetUsageno longer races disposal.Remarks
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Requires thorough manual testing