fix: load full changeset range and add regression test#3945
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
There was a problem hiding this comment.
Pull request overview
Fixes incorrect loading of state changeset ranges by switching from RocksDB iterator-based scanning (which breaks with little-endian BCS-encoded u64 keys) to a numeric-order multi_get batching strategy, and adds a regression unit test.
Changes:
- Replace iterator-based range loading with batched
multi_getover the numeric[from_order, to_order)span. - Add a unit test intended to ensure contiguous changeset spans are fully returned.
- Add
tempfileas a dev-dependency to support RocksDB-backed tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/rooch-store/src/state_store/mod.rs | Removes RocksDB iterator logic and always uses batched numeric multi_get; adds RocksDB-backed regression test helper and test. |
| crates/rooch-store/Cargo.toml | Adds tempfile dev-dependency for the new test setup. |
| fn temp_state_store() -> StateDBStore { | ||
| let dir = TempDir::new().expect("create tempdir"); | ||
| let registry = Registry::new(); | ||
| let db = RocksDB::new( | ||
| dir.path(), | ||
| crate::StoreMeta::get_column_family_names().to_vec(), | ||
| RocksdbConfig::default(), | ||
| ) | ||
| .expect("create rocksdb"); | ||
| let db_metrics = DBMetrics::new(®istry); | ||
| let instance = StoreInstance::new_db_instance(db, Arc::new(db_metrics)); | ||
| StateDBStore::new(instance) | ||
| } |
There was a problem hiding this comment.
temp_state_store() drops TempDir at the end of the function, but the returned StateDBStore still holds an open RocksDB instance pointing at that path. This can cause cleanup failures (notably on Windows) and makes the test rely on implementation details of tempfile/filesystem semantics. Keep the temp directory alive for the whole test (e.g., return a struct holding both TempDir and StateDBStore, or create the TempDir in the test and pass dir.path() into a helper).
| // Insert contiguous changesets keyed by numeric order | ||
| store.insert_dummy_changesets(110, 30); | ||
|
|
||
| // Fetch the span and ensure none are skipped | ||
| let got = store | ||
| .get_changesets_range_with_limit(110, 140, usize::MAX) | ||
| .expect("load range"); | ||
|
|
||
| assert_eq!(got.len(), 30, "should load all changesets in range"); | ||
| assert_eq!(got.first().unwrap().0, 110); | ||
| assert_eq!(got.last().unwrap().0, 139); | ||
| } |
There was a problem hiding this comment.
This test range (110..140) does not reproduce the little-endian RocksDB iterator ordering bug; for many small contiguous values, lexicographic order still matches numeric order, so an iterator-based implementation could still pass this test. To make this a true regression test, choose a span that crosses an endian boundary (e.g., 250..270 crossing 255/256, or 65_530..65_550 crossing 65_535/65_536) so the old iterator approach would deterministically miss entries.
| [dev-dependencies] | ||
| tempfile = "3" |
There was a problem hiding this comment.
The workspace already pins tempfile (see root Cargo.toml) and other crates depend on it via tempfile = { workspace = true }. Using tempfile = "3" here can lead to version drift across the workspace and duplicate semver requirements. Prefer tempfile = { workspace = true } for consistency.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/rooch-store/src/state_store/mod.rs:145
- The doc comment says this is a "Fallback implementation for non-RocksDB stores", but
get_changesets_range_with_limitnow unconditionally calls this path even for RocksDB. Update the comment (and consider renaming the helper) so it reflects the current behavior.
/// Fallback implementation for non-RocksDB stores using batch queries
fn get_changesets_range_batch_fallback(
&self,
| fn temp_state_store() -> StateDBStore { | ||
| let dir = TempDir::new().expect("create tempdir"); | ||
| let registry = Registry::new(); | ||
| let db = RocksDB::new( | ||
| dir.path(), | ||
| crate::StoreMeta::get_column_family_names().to_vec(), | ||
| RocksdbConfig::default(), | ||
| ) | ||
| .expect("create rocksdb"); | ||
| let db_metrics = DBMetrics::new(®istry); | ||
| let instance = StoreInstance::new_db_instance(db, Arc::new(db_metrics)); | ||
| StateDBStore::new(instance) | ||
| } |
There was a problem hiding this comment.
temp_state_store() creates a TempDir but drops it at the end of the function, which can delete the underlying directory while the returned StateDBStore/RocksDB is still in use (can lead to flaky failures, especially on Windows). Keep the TempDir alive for the duration of the test (e.g., return (TempDir, StateDBStore) or wrap both in a struct).
|
Docker images for this PR are available:
Pull commands:
|
Summary
Testing