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
3e0f462 to
e2c5289
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0e269cb to
68b4b04
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
crates/store/src/state/mod.rs:1149
- In the loop starting at line 1109, if a
SlotData::Allrequest is followed by aSlotData::MapKeysrequest and the MapKeys request fails (e.g., returns None at line 1112 or an error at line 1123), the forest_guard that was re-acquired at line 1143 will still be held when the error is returned. This keeps the write lock on the forest unnecessarily.
While Rust's RAII will drop the guard eventually, it's better to explicitly drop it before returning errors to avoid holding the lock during error handling. Consider wrapping the MapKeys logic in a scope that ensures the guard is dropped, or use an early break pattern.
for StorageMapRequest { slot_name, slot_data } in storage_requests {
let details = match &slot_data {
SlotData::MapKeys(keys) => forest_guard
.get_storage_map_details_for_keys(
account_id,
slot_name.clone(),
block_num,
keys,
)
.ok_or_else(|| DatabaseError::StorageRootNotFound {
account_id,
slot_name: slot_name.to_string(),
block_num,
})?
.map_err(DatabaseError::MerkleError)?,
SlotData::All => {
// we don't want to hold the forest guard for a prolonged time
drop(forest_guard);
// we collect all storage items, if the account is small enough or
// return `AccountStorageMapDetails::LimitExceeded`
let details = self
.db
.reconstruct_storage_map_from_db(
account_id,
slot_name.clone(),
block_num,
Some(
// TODO unify this with
// `AccountStorageMapDetails::MAX_RETURN_ENTRIES`
// and accumulated the limits
<QueryParamStorageMapKeyTotalLimit as QueryParamLimiter>::LIMIT,
),
)
.await?;
forest_guard = self.forest.write().await;
details
},
};
storage_map_details.push(details);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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