Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements periodic pruning of the InnerForest in-memory data structures to bound memory growth, addressing issue #1175 about pruning account storage maps and vault tables.
Changes:
- Added pruning mechanism that retains only the last 50 blocks (configurable via
HISTORICAL_BLOCK_RETENTIONconstant) of account vault and storage map data - Introduced block-indexed tracking structures (
vault_roots_by_blockandstorage_slots_by_block) to efficiently identify entries eligible for pruning - Refactored
state/mod.rsto cacheblock.body()calls and improve code readability
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/store/src/inner_forest/mod.rs | Added pruning logic with new tracking data structures, HISTORICAL_BLOCK_RETENTION constant, and helper methods to prune old vault and storage map roots |
| crates/store/src/inner_forest/tests.rs | Added comprehensive test coverage for pruning functionality, including edge cases like empty forest, young chains, boundary conditions, and multiple accounts/slots |
| crates/store/src/state/mod.rs | Refactored to cache block.body() result and renamed block_data to block_bytes for clarity; split forest write lock acquisition into separate line |
| crates/store/src/db/tests.rs | Renamed test function to follow consistent naming convention using db_roundtrip_ prefix |
| CHANGELOG.md | Added enhancement entry for periodic cleanup feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 81 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
72dad48 to
a3be45b
Compare
|
Found one more bug, when using RPC with response The large changeset is mostly due to tests added. |
igamigo
left a comment
There was a problem hiding this comment.
Overall LGTM, but left a couple of comments. Not sure if they are fully correct (or if they land within the scope of this PR exactly), but worth reviewing for the long term at least.
|
@bobbinth it looks like the |
35b80c3 to
098c805
Compare
| .map(|((cached_block, ..), snapshot)| (*cached_block, snapshot)) | ||
| .max_by_key(|(cached_block, _)| cached_block.as_u32()) | ||
| .map(|(_, snapshot)| snapshot.entries.clone()) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Is this correct? Seems like the entry that you are trying to retrieve here could be evicted and then the new delta would be built from a default base.
| let values = self | ||
| .select_storage_map_sync_values( | ||
| account_id, | ||
| BlockNumber::GENESIS..=block_num, | ||
| entries_limit, | ||
| ) | ||
| .await?; | ||
| if values.last_block_included != block_num { | ||
| return Ok(AccountStorageMapDetails::limit_exceeded(slot_name)); | ||
| } |
There was a problem hiding this comment.
Should this loop until all values are retrieved instead?
| ) | ||
| .await?; | ||
| if values.last_block_included != block_num { | ||
| return Ok(AccountStorageMapDetails::limit_exceeded(slot_name)); |
There was a problem hiding this comment.
nit: the query does not filter per slot, right? So really the limit is exceeded as a whole
Important
Targeting
mainWhat
Does cleanup the
InnerForest, both the lookup tables/maps and the actualSmtForest.Required for bounding the in-memory size growth.
Context
Related #1175
How
There are a few requirements:
*_refcounttablesCaveats
::AllEntries, theSmtProofcontains the relevantSmtLeafitselfSmtForestfor a specific key