Enable state pruning for archival nodes - #4764
Conversation
Archive mode disables the pruner so that consensus data is retained in full, which also leaves the merklized state tables, the largest in the database, growing without bound. Only recent state is needed to serve catchup, and any height can be derived again by replaying the leaves, so archive nodes now garbage collect state more than ARCHIVE_STATE_RETENTION heights behind the latest state height (a week of blocks by default). The newest version of every node is always kept, so the current state stays complete. Set ARCHIVE_FULL_STATE to retain everything as before. Nodes running the pruner are unaffected: it already prunes state alongside consensus data. Archive mode also no longer clears the state pruned height on startup, since the fetcher does not reconstruct merklized state.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
| update_state_storage_loop(ds.clone(), get_node_state), | ||
| ); | ||
|
|
||
| // Archive mode disables the pruner, so nothing else bounds the merklized state tables. |
There was a problem hiding this comment.
@imabdulbasit what's the point of making it configurable then? If it's useless we should just always delete it.
There was a problem hiding this comment.
I would like to keep it optional at first, so if anything goes wrong then we can just disable it via an env variable. We hav had issues before with pruning so I would prefer to keep it optional
The version at the pruned height itself is never superseded within the batch, so it survives collection. Update the config snapshot for the two new archive state retention options.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Claude finished @imabdulbasit's task in 9m 7s —— View job Code review
The design is sound and the reasoning in the doc comments is genuinely good — especially the delete + I verified the underlying deletion invariant holds: Six inline comments. The ones I'd act on before merge: 1. 2. Does this actually reclaim the disk it claims? ( 3. Fights the 4. User-facing behaviour change worth calling out explicitly. After collection, Testing gaps (existing tests are well constructed — the "newest version at the pruned height survives" assertion is exactly the right thing to pin):
CI: the two failures reported earlier ( Nits: stray blank line between the doc comment and No security concerns — no new external input reaches these queries, table names come from compile-time |
|
|
||
| tracing::info!(head, target, from, "collecting archived merklized state"); | ||
| while from < target { | ||
| let to = min(from + self.cfg.batch_size(), target) - 1; |
There was a problem hiding this comment.
Underflow / infinite loop / over-deletion when batch_size == 0.
ESPRESSO_NODE_PRUNER_BATCH_SIZE=0 is accepted here: PrunerCfg::validate() doesn't check batch_size, and ArchiveStateGc::new builds the cfg with PrunerCfg::from(opt.pruning) (line 141) without ever calling validate() — archive mode never goes through Config::pruner_cfg(), since --prune conflicts with --archive.
With batch_size == 0 and pruned == None (from == 0), min(0 + 0, target) - 1 underflows. In release builds that wraps to u64::MAX, and then:
delete_state_batch(tables, u64::MAX)deletes every row that isn't the newest at its path — collapsing all history including inside the retention window, which breaks catchup for any height below head;save_state_pruned_height(u64::MAX)records that;from = to + 1wraps back to0, so thewhile from < targetloop never terminates.
Cheap fix — clamp the batch and avoid the bare subtraction:
| let to = min(from + self.cfg.batch_size(), target) - 1; | |
| let batch_size = self.cfg.batch_size().max(1); | |
| let to = min(from + batch_size, target) - 1; |
| .prune_write() | ||
| .await | ||
| .context("opening transaction to delete state")?; | ||
| tx.delete_state_batch(self.cfg.state_tables(), to).await?; |
There was a problem hiding this comment.
Interaction with the in-flight *_bigint backfill.
cfg.state_tables() is ["block_merkle_tree_bigint", "fee_merkle_tree_bigint"] (persistence/sql.rs:664). The legacy fee_merkle_tree / block_merkle_tree tables are still populated on Postgres nodes that haven't finished BackfillFeeMerkleTree / BackfillBlockMerkleTree, and those backfills move legacy rows into *_bigint in ascending created (persistence/migrations.rs:104-155).
So on a node mid-backfill:
- the GC deletes old
*_bigintrows and advances the state pruned height; - the backfill then re-inserts rows for those same old
createdvalues (ON CONFLICT (path, created) DO NOTHINGno longer conflicts, since the row is gone); - those resurrected rows are below the pruned height, so no read will ever return them — they're invisible dead weight until the next GC interval deletes them again.
Not a correctness bug (reads at created <= pruned_height return NotFound), but it's wasted I/O and bloat, and no disk is actually reclaimed until the backfill passes the retention watermark. Worth either gating the GC on those two backfills being complete, or including the legacy tables in the delete list while they exist.
| } | ||
|
|
||
| /// Reclaim the space freed by the deleted rows. | ||
| async fn vacuum(&self, storage: &SqlStorage) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Orphaned hash_bigint rows cap the space actually reclaimed.
Merkle node rows reference hash_bigint(id) (FK ... REFERENCES hash_bigint(id), V1501__hash_bigint_expand.sql), and nothing in the codebase ever deletes from hash_bigint — it's append-only, deduplicated by value with a UNIQUE index. Since almost every node version has a distinct hash, hash_bigint holds roughly one row per deleted node version, so its row count and bytes (8-byte id + 32-byte value + PK + unique index on value) are in the same ballpark as the merkle rows this GC removes.
Net effect: the deletion is real but the disk win is likely a fraction of what the PR description implies. A batched sweep of orphans, e.g.
DELETE FROM hash_bigint AS h
WHERE h.id IN (SELECT id FROM hash_bigint WHERE id < $watermark LIMIT $batch)
AND NOT EXISTS (SELECT 1 FROM fee_merkle_tree_bigint WHERE hash_id = h.id)
AND NOT EXISTS (SELECT 1 FROM block_merkle_tree_bigint WHERE hash_id = h.id)would close the gap. Same gap exists for the time-based pruner, so it's arguably follow-up work — but it's worth stating explicitly in the PR whether the measured savings account for it.
Minor, in the same function: this duplicates SqlStorage::vacuum (hotshot-query-service/src/data_source/storage/sql.rs:1319); since prune_write was just made pub, making vacuum reusable (taking pages as an argument) would avoid the second copy.
| } | ||
|
|
||
| /// Delete all state older than the retention window. | ||
| async fn collect(&self, storage: &SqlStorage) -> anyhow::Result<()> { |
There was a problem hiding this comment.
On an existing archive node the first collect() call walks the entire history in one pass — with head in the millions and the default batch_size of 1000 that's thousands of delete transactions back to back, and on SQLite vacuum() only runs after the whole backlog finishes, so no space is reclaimed until then (and the freelist grows the whole time). The created index keeps each individual batch cheap, so this is a pacing/observability point rather than a query-plan one: consider vacuuming every N batches and logging progress (rows deleted / current from) so operators can see where a multi-hour first pass is.
Also, unlike the pruner (fetching.rs:443-470) there's no per-batch error backoff. Not a bug — a failed run resumes from the recorded pruned height on the next interval — but a transient error costs a full interval() (1.5h by default) of progress.
| /// Number of recent block heights of merklized state an archive node retains. | ||
|
|
There was a problem hiding this comment.
Stray blank line between the doc comment and the attribute (it compiles, but it's inconsistent with every other option here). Also worth documenting that the GC's pacing comes from the pruner options — ESPRESSO_NODE_PRUNER_INTERVAL (default 1.5h) and ESPRESSO_NODE_PRUNER_BATCH_SIZE — which is surprising given --archive conflicts with --prune:
| /// Number of recent block heights of merklized state an archive node retains. | |
| /// | |
| /// Collection runs on the pruner interval (PRUNER_INTERVAL) in batches of PRUNER_BATCH_SIZE | |
| /// heights, even though the pruner itself is disabled in archive mode. | |
| #[clap( |
| // reconstruct previously pruned data. | ||
| query("DELETE FROM pruned_height") | ||
| query("DELETE FROM pruned_height WHERE id = $1") | ||
| .bind(Transaction::<Write>::PRUNED_HEIGHT_ID) |
There was a problem hiding this comment.
Good change, and it also fixes a latent bug: previously an archive restart cleared the state pruned height too, but merklized state is never re-fetched from peers (only consensus data is), so get_path at those heights would stop returning NotFound and instead reconstruct from surviving nodes — yielding a commitment mismatch error, or wrong data if the commitment check were absent.
Worth a regression test alongside the existing pruning tests in this file: connect with archive, having seeded both pruned_height rows, and assert load_pruned_height() == None while load_state_pruned_height() == Some(h). Nothing currently pins the WHERE id = $1, so a future refactor could silently restore the old behaviour.
Clamp a zero configured batch size instead of underflowing, report progress and vacuum periodically during a long first collection pass, document that collection pacing comes from the pruner options, and pin that an archive restart clears only the data pruned height.
Currently the archival node store all the derived and underived data such as merklized state. A node only needs to keep the most recent merklized state data to serve catchup requests, and this data can be derived by replaying the leaves and applying the state transition function.
This PR adds a state garbage collector for the archival nodes that deletes state more than
ESPRESSO_NODE_ARCHIVE_STATE_RETENTIONheights behind the latest state height. SetESPRESSO_NODE_ARCHIVE_FULL_STATEto retain everything as before. Nodes running the pruner are unaffected, since it already prunes state alongside the other data, and archive startup no longer clears the state pruned height.