Skip to content

Fix history reads migration - #3348

Open
vicsn wants to merge 7 commits into
testnetfrom
fix_history_reads_migration_testnet
Open

Fix history reads migration#3348
vicsn wants to merge 7 commits into
testnetfrom
fix_history_reads_migration_testnet

Conversation

@vicsn

@vicsn vicsn commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Motivation

The previous migration logic in #3323 was insufficient. This PR introduces a one-time synchronous db migration to overcome the performance issue.

"Any key that already had an entry in MappingUpdateHeightsMap before this patch was deployed stays on the old path forever — every new write does a full read-modify-write of the growing vector. For an archive node with months of accumulated history, almost all actively-updated keys (staking/credits mappings) already fall into this category, so nearly all hot writes stay on the slow path permanently."

  • p2p_height - ledger_height lag grew monotonically (from ~39 to 138 blocks over 30 minutes) instead of converging to zero
  • The RocksDB LOG showed a constant sawtooth inestimated pending compaction bytes(0-24GB) that never drained to zero
  • Disk write throughput jumped to ~3.6-3.9 GB/s (vs ~7 MB/s on a healthy node) — roughly a 500x increase in write amplification
  • Hardware was nowhere near saturated (CPU ~2.5 of 32 cores, disk ~18-20% utilization) — so this isn't a hardware issue, it's purely algorithmic

Performance

Local benchmarking on macOS M2 Max shows we can do ~200k updates per second. Time scales roughly linearly with the total number of mapping keys and historical entries. In production we have 20M blocks (which each create a few entries) and 26M transactions (many of which create a few entries), which would predict at least ~5 minutes of migration time.

Test Plan

  • We should test this on a historical testnet node at tip.

vicsn and others added 5 commits July 31, 2026 12:27
Document the migration throughput harness and use the fastest tested batch size to reduce RocksDB startup migration time.

Co-authored-by: Cursor <cursoragent@cursor.com>
@vicsn
vicsn requested a review from ljedrz July 31, 2026 10:27
@vicsn vicsn mentioned this pull request Jul 31, 2026
1 task
Comment thread ledger/store/src/program/finalize.rs Outdated
@veria-ai

veria-ai Bot commented Jul 31, 2026

Copy link
Copy Markdown

PR overview

All previously flagged issues have been addressed. No open security concerns remain on this pull request.

Security review

No open security issues remain on this pull request.

Fixed/addressed: 1 · PR risk: 0/10

@ljedrz ljedrz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

While the overall approach is fine (with the caveat already noted by Veria AI), I'd suggest an alternative, lower-level solution for greater speed and future applicability:

  1. don't read the ledger using the snarkVM types, just open RocksDB "raw"
  2. dump the map of interest (based on the expected prefix) to a file
  3. delete the existing map in its entirety (solves the issue mentioned by Veria AI)
  4. translate the map from the external file into the new format (could be on the fly), and batch-write it to the database (still in the raw format)
  5. the ledger can now be loaded

That being said, I wouldn't want to block this, so approving the current approach (with the aforementioned issue fixed).

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.

2 participants