Skip to content

ethdb, triedb: tune pebble write path and add safe pathdb state carry-over#2170

Merged
marcello33 merged 4 commits intodevelopfrom
mardizzone/pebble-opt
Mar 31, 2026
Merged

ethdb, triedb: tune pebble write path and add safe pathdb state carry-over#2170
marcello33 merged 4 commits intodevelopfrom
mardizzone/pebble-opt

Conversation

@marcello33
Copy link
Copy Markdown
Collaborator

@marcello33 marcello33 commented Mar 30, 2026

Description

This PR implements some PebbleDB optimizations.
Others are on a config level, and will be managed in pos-ops.
The suggested configs are (tested on a 128GB RAM machine)

  [cache]
    cache = 24576 # 3x the original value
    gc = 5
    gomemlimit = "55GB"
    snapshot = 25
    database = 45
    trie = 25

The PR combines write-path tuning with pathdb buffer changes aimed at reducing write stalls and improving dirty-state reuse across flushes.
On the Pebble side, it tunes sync and compaction behavior. On the pathdb side, it splits the write buffer into node/state budgets, allows state carry-over when flushes are caused by trie-node pressure, and keeps that carry-over safe under async flushing via copy-on-write state snapshots. The copy-on-write carry-over means:

  • top-level state maps are copied
  • per-account storage submaps are shared initially
  • a storage submap is cloned only on first write
    This keeps the frozen flush snapshot immutable while still avoiding eager duplication of all storage submaps.

Pebble tuning:

  • increase BytesPerSync from the implicit default to 1 MiB
  • increase MemTableStopWritesThreshold to reduce write stalls during flushes
  • enable adaptive compaction settings:
    • L0CompactionConcurrency = 1
    • CompactionDebtConcurrency = 256 MiB

Pathdb buffer changes:

  • raise maxBufferSize to 2 GiB
  • add StateReservation to split the write buffer between:
    • trie nodes
    • flat state (accounts/storage)
  • update buffer fullness logic:
    • flush when trie nodes exceed their reservation
    • also flush when total buffer usage exceeds the hard limit
  • carry state forward only when:
    • the flush was caused by node pressure
    • state is still within its reserved budget
  • wire the new buffer constructor/config through generation and journal loading
  • clear carried-over state on revert when there are no uncommitted transitions left

Changes

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Changes only for a subset of nodes

Checklist

  • I have added at least 2 reviewer or the whole pos-v1 team
  • I have added sufficient documentation in code
  • I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply
  • Created a task in Jira and informed the team for implementation in Erigon client (if applicable)
  • Includes RPC methods changes, and the Notion documentation has been updated

Testing

  • I have added unit tests
  • I have added tests to CI
  • I have tested this code manually on local environment
  • I have tested this code manually on remote devnet using express-cli
  • I have tested this code manually on mainnet
  • I have created new e2e tests into express-cli

Copilot AI review requested due to automatic review settings March 30, 2026 15:37
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@marcello33
Copy link
Copy Markdown
Collaborator Author

@claude review

@claude
Copy link
Copy Markdown

claude bot commented Mar 30, 2026

Claude finished @marcello33's task in 1m 58s —— View job


Code Review: PebbleDB Optimizations & Pathdb State Carry-Over

  • Read and analyze all changed files
  • Review Pebble write-path tuning
  • Review pathdb buffer splitting and config changes
  • Review copy-on-write state carry-over implementation
  • Review test coverage
  • Post findings

Overall this is a well-structured PR with solid test coverage for the new carry-over logic. The copy-on-write approach is correctly implemented and the async flush isolation is properly handled. Below are my findings.


Pebble Tuning (ethdb/pebble/pebble.go)

Looks good. The changes are reasonable and well-commented:

  • BytesPerSync: 1 MiB — sensible increase from the 512 KB default; reduces sync frequency during compaction.
  • MemTableStopWritesThreshold: memTableLimit * 2 — doubles the threshold to absorb flush bursts and reduce write stalls. This is a common tuning knob.
  • L0CompactionConcurrency = 1 and CompactionDebtConcurrency = 256 MiB — adaptive compaction is a good choice over the previous "use all CPUs" approach, which can cause CPU contention with EVM execution.

No issues found here.


Pathdb Buffer Splitting (triedb/pathdb/buffer.go, triedb/pathdb/config.go)

No bugs found. The design is clean:

  • newBuffer correctly computes nodeLimit and stateLimit from the stateReservation percentage, with a fallback to defaultStateReservation for invalid values.
  • full() correctly triggers on either condition: nodes.size > nodeLimit OR total > limit.
  • shouldCarryStates() is a simple states.size <= stateLimit check — correct.

Observations:

  1. maxBufferSize raised to 2 GiB (config.go:43) — this is within the 4 GB Pebble batch limit (as noted in the comment). With async flushing enabled by default, this is fine. The comment correctly warns about disabling async flushing.

  2. defaultStateReservation = 80 — 80% for states, 20% for trie nodes. This is an aggressive state bias. With the default 64 MB buffer, that's only ~13 MB for trie nodes before triggering a flush. For nodes running with larger WriteBufferSize configs this is fine, but with defaultBufferSize (64 MB) this may cause frequent flushes during initial sync when trie node churn is high. Consider whether the default reservation makes sense for all node types (archive, full, snap-syncing).


Copy-on-Write State Carry-Over (triedb/pathdb/states.go)

Well implemented. The COW pattern is correct:

  • copy() (line 107-124): Clones top-level account and storage maps, shares storage submaps, sets storageShared = true. Account []byte values are treated as immutable (shared by reference) — this is safe because the merge/revert paths always replace values, never mutate them in-place.

  • writableStorage() (line 152-164): Correctly clones the submap on first write and marks it owned via markStorageOwned().

  • merge() (line 331): Uses writableStorage(accountHash) before mutating — correct COW integration.

  • revertTo() (line 375): Also uses writableStorage(accountHash) — correct.

  • reset() (line 500-508): Clears storageShared and ownedStorage — correct cleanup.

  • decode() (line 485-486): Correctly sets storageShared = false for deserialized data.

One minor observation:

  1. Low: copy() doesn't initialize ownedStoragestates.go:116-123

    The copy() method creates a new stateSet with storageShared = true but doesn't initialize ownedStorage. This is handled lazily in markStorageOwned() (line 144: if s.ownedStorage == nil { s.ownedStorage = make(...) }), so it's functionally correct. Just noting the lazy init pattern is intentional and safe.


Disk Layer Commit Integration (triedb/pathdb/disklayer.go)

Correct implementation with one important nuance handled well:

  1. Flush capture pattern (buffer.go:163-165): The flush() method captures flushNodes, flushStates, flushLayers before launching the goroutine. This is critical — it allows the caller to transfer b.states ownership (for carry-over) without racing with the background flush. Well done.

  2. Carry-over decision (disklayer.go:483-488):

    if !force && !flush && combined.shouldCarryStates() {
        carried := combined.states.copy()
        combined = newBuffer(...)
    }

    This correctly suppresses carry-over for force flushes and history-driven flushes. The copy() before creating the new buffer ensures the frozen buffer's state snapshot is immutable.

  3. Revert path (disklayer.go:549-551): When the buffer is empty (layers == 0) but may contain carried-over states, reset() is called to clear them before the persistent-state revert. This prevents stale carried state from serving reads after persistent state is rolled back. Correct.


Test Coverage

Excellent. The test suite covers all critical scenarios:

  • TestBufferLimitSplit — reservation arithmetic
  • TestBufferFullTriggeredByNodeLimit/TotalLimit — both flush triggers
  • TestShouldCarryStates — carry-over decision boundary
  • TestStateSetCopy — snapshot isolation via merge
  • TestDiskLayerCommitCarryOverIsolationthe critical test — uses a blockingAncientStore to hold the flush mid-flight while mutating and resetting the live buffer. Verifies the persisted data matches the original snapshot, not the mutated live state. This directly validates the COW correctness under async flushing.
  • TestDiskLayerCommitForceFlushDoesNotCarryStates / TestDiskLayerCommitHistoryFlushDoesNotCarryStates — carry-over suppression
  • TestRevertClearsCarriedStates / TestRevertToWithLayers — revert correctness
  • TestStateSetCopyUsesCopyOnWriteForStorageMaps — verifies pointer sharing and lazy detach
  • TestStateSetRevertToDetachesSharedStorageMaps — revert triggers COW correctly

One suggestion:

  1. Consider adding a -race test for the carry-over path

    TestDiskLayerCommitCarryOverIsolation tests correctness but not data-race safety. Running the test suite with -race would validate that the live buffer and frozen buffer don't share mutable state. The COW design should pass, but it would be valuable to run go test -race ./triedb/pathdb/ as part of CI for this change.


Summary

Area Verdict
Pebble tuning Good — reasonable, well-commented changes
Buffer split logic Good — correct arithmetic, clean API
COW state carry-over Good — correctly implemented with proper isolation
Async flush safety Good — snapshot capture + COW prevents races
Revert path Good — carried states correctly cleared
Test coverage Excellent — covers all critical paths including async isolation
Config wiring GoodStateReservation threaded through generate, journal, tests

No CRITICAL or HIGH issues found. The PR is well-designed and thoroughly tested. The COW carry-over is the most complex part and it's correctly implemented — the flush() snapshot capture pattern combined with copy() ensures the frozen buffer remains immutable while the live buffer evolves independently.


|

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tunes the PebbleDB write path and refactors PathDB’s write buffer to reduce write stalls while safely reusing dirty flat-state across flushes (via copy-on-write state snapshots).

Changes:

  • Pebble: adjust sync/flush/compaction knobs (BytesPerSync, MemTableStopWritesThreshold, adaptive compaction concurrency settings).
  • PathDB: split write-buffer budgeting between trie nodes and flat state, flush on node-pressure or hard total limit, and carry over state only when safe/within budget.
  • Add extensive unit tests covering buffer budgeting, carry-over semantics, and copy-on-write isolation.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
ethdb/pebble/pebble.go Pebble option tuning for sync cadence, write-stall reduction, and adaptive compaction.
triedb/pathdb/config.go Introduces StateReservation config and raises maximum allowed buffer size.
triedb/pathdb/buffer.go Adds node/state budgeting, new flush trigger logic, and carry-over eligibility helper.
triedb/pathdb/disklayer.go Implements safe state carry-over on node-pressure flushes; clears carried state on revert with no transitions.
triedb/pathdb/states.go Adds copy-on-write stateSet.copy() and detaching logic for shared per-account storage submaps.
triedb/pathdb/buffer_test.go New tests for buffer split, flush triggers, carry-over behavior, and async-flush isolation.
triedb/pathdb/states_test.go New tests validating copy-on-write behavior for storage submaps and revert detaching.
triedb/pathdb/journal.go Wires StateReservation into buffer construction during journal loading.
triedb/pathdb/generate.go Wires StateReservation into buffer construction during snapshot generation.
triedb/pathdb/difflayer_test.go Updates buffer construction to new signature.
triedb/pathdb/layertree_test.go Updates buffer construction to new signature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread triedb/pathdb/config.go Outdated
Comment thread triedb/pathdb/buffer.go Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 30, 2026 16:04
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread triedb/pathdb/config.go
Comment thread triedb/pathdb/buffer.go
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026

Codecov Report

❌ Patch coverage is 85.84906% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.92%. Comparing base (4d3fff5) to head (a178c20).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
ethdb/pebble/pebble.go 0.00% 10 Missing ⚠️
triedb/pathdb/states.go 94.33% 2 Missing and 1 partial ⚠️
triedb/pathdb/buffer.go 92.85% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (85.84%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2170      +/-   ##
===========================================
+ Coverage    51.88%   51.92%   +0.03%     
===========================================
  Files          884      884              
  Lines       155324   155412      +88     
===========================================
+ Hits         80593    80693     +100     
+ Misses       69522    69512      -10     
+ Partials      5209     5207       -2     
Files with missing lines Coverage Δ
triedb/pathdb/config.go 81.57% <100.00%> (+2.79%) ⬆️
triedb/pathdb/disklayer.go 80.31% <100.00%> (+3.04%) ⬆️
triedb/pathdb/generate.go 83.30% <100.00%> (ø)
triedb/pathdb/journal.go 46.84% <100.00%> (ø)
triedb/pathdb/buffer.go 86.82% <92.85%> (+4.67%) ⬆️
triedb/pathdb/states.go 88.91% <94.33%> (+1.21%) ⬆️
ethdb/pebble/pebble.go 17.79% <0.00%> (-0.23%) ⬇️

... and 20 files with indirect coverage changes

Files with missing lines Coverage Δ
triedb/pathdb/config.go 81.57% <100.00%> (+2.79%) ⬆️
triedb/pathdb/disklayer.go 80.31% <100.00%> (+3.04%) ⬆️
triedb/pathdb/generate.go 83.30% <100.00%> (ø)
triedb/pathdb/journal.go 46.84% <100.00%> (ø)
triedb/pathdb/buffer.go 86.82% <92.85%> (+4.67%) ⬆️
triedb/pathdb/states.go 88.91% <94.33%> (+1.21%) ⬆️
ethdb/pebble/pebble.go 17.79% <0.00%> (-0.23%) ⬇️

... and 20 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.

@marcello33 marcello33 requested a review from a team March 30, 2026 18:27
@marcello33 marcello33 merged commit 393dd21 into develop Mar 31, 2026
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants