Skip to content

fix(prune): respect rocksdb storage settings for history pruning#21535

Closed
HuberyJulianay wants to merge 2 commits intoparadigmxyz:mainfrom
HuberyJulianay:pruning
Closed

fix(prune): respect rocksdb storage settings for history pruning#21535
HuberyJulianay wants to merge 2 commits intoparadigmxyz:mainfrom
HuberyJulianay:pruning

Conversation

@HuberyJulianay
Copy link
Contributor

History pruning was hardcoded to use MDBX cursors, ignoring account_history_in_rocksdb and storages_history_in_rocksdb settings. This meant RocksDB history data would never get cleaned up when pruning was enabled.
Added prune_account_history_up_to and prune_storage_history_up_to to RocksDBBatch, and updated the prune segments to check storage settings before choosing the backend.

@gakonst
Copy link
Member

gakonst commented Feb 2, 2026

Closing this PR after review. The approach has several issues:

  1. Trait bound leakage: The RocksDBProviderFactory bound appears to be required unconditionally in some code paths, which would break MDBX-only builds.

  2. Excessive cfg fragmentation: +842 lines across 8 files with duplicated cfg-gated implementations creates maintenance burden and increases risk of cfg mismatch bugs across the feature matrix.

  3. API stability: Having two different build() methods under different cfgs can cause downstream breakage when call sites need their own conditional compilation.

  4. Unclear runtime behavior: Not evident that the runtime paths actually use the new RocksDB provider factory when settings indicate RocksDB history storage.

If you'd like to pursue this fix, consider:

  • Centralizing backend selection behind a single HistoryBackendFactory abstraction
  • Ensuring all RocksDB-specific code is strictly behind cfg gates
  • Adding integration tests for both rocksdb and non-rocksdb build configurations

@gakonst gakonst closed this Feb 2, 2026
@github-project-automation github-project-automation bot moved this from Backlog to Done in Reth Tracker Feb 2, 2026
@yongkangc yongkangc self-assigned this Feb 2, 2026
HuberyJulianay added a commit to HuberyJulianay/reth that referenced this pull request Feb 2, 2026
…ackend selection

Add tests to ensure that when rocksdb feature is enabled but storage settings
specify MDBX (account_history_in_rocksdb=false, storages_history_in_rocksdb=false),
the pruner correctly uses the MDBX path and does not touch RocksDB data.

This addresses the integration testing suggestion from paradigmxyz#21535 review:
- Tests both account_history and storage_history pruning
- Verifies MDBX data is pruned
- Verifies RocksDB data is NOT modified when settings indicate MDBX backend

Also fixes typos CI by adding 'consts' and 'Consts' to the allowed words list
(std::env::consts is Rust stdlib, RethCliVersionConsts is a valid type name).

Amp-Thread-ID: https://ampcode.com/threads/T-019c1e9b-7b96-702c-8268-a603e741f97b
Co-authored-by: Amp <amp@ampcode.com>
HuberyJulianay added a commit to HuberyJulianay/reth that referenced this pull request Feb 2, 2026
…ackend selection

Add tests to ensure that when rocksdb feature is enabled but storage settings
specify MDBX (account_history_in_rocksdb=false, storages_history_in_rocksdb=false),
the pruner correctly uses the MDBX path and does not touch RocksDB data.

This addresses the integration testing suggestion from paradigmxyz#21535 review:
- Tests both account_history and storage_history pruning
- Verifies MDBX data is pruned
- Verifies RocksDB data is NOT modified when settings indicate MDBX backend

Amp-Thread-ID: https://ampcode.com/threads/T-019c1e9b-7b96-702c-8268-a603e741f97b
Co-authored-by: Amp <amp@ampcode.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants