fix: remove unique events logic from the ChainGetEvents API
#6286
Conversation
WalkthroughRemoved IndicesStore and its usages, shortened ChainStore::new (dropped indices/db arg), switched async chain-event collection to a non-async get_events_by_event_root via StampedEvent::get_events, added event AMT storage/validation in StateManager, and expanded event tests for V3/V4, order, duplicates. Changes
Sequence Diagram(s)sequenceDiagram
participant RPC as RPC Handler
participant SM as StateManager
participant Exec as Executor / StampedEvent
participant BS as BlockStore
participant AMT as AMT
Note over SM,Exec: Apply — build and store events AMTs
RPC->>SM: apply_block_messages(messages, events)
SM->>Exec: compute state & receipts
SM->>AMT: build receipts AMT (Amtv0)
SM->>AMT: derive events AMT(s) (Amt with EVENTS_AMT_BITWIDTH)
SM->>SM: ensure(derived_root == provided_event_root)
SM->>BS: put events AMT root and nodes
Note over RPC,SM: Retrieve — load by events root or compute fallback
RPC->>SM: tipset_state_events(tipset) or get_events_by_event_root(root)
alt events stored
SM->>BS: load events AMT by root
BS->>Exec: AMT node data
Exec->>Exec: StampedEvent::get_events(root) (try V4, fallback V3)
Exec-->>SM: Vec<Event> (order & duplicates preserved)
else compute fallback
SM->>Exec: compute state & extract events
Exec-->>SM: Vec<Event>
end
SM-->>RPC: StateEvents / events
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
akaladarshi
force-pushed
the
akaladarshi/fix-get-events-api
branch
3 times, most recently
from
dda3266 to
a15e4e9
Compare
akaladarshi
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/shim/executor.rs (1)
340-415: StampedEvent AMT loader looks good; watch serde wire format & small perf nitThe new
StampedEvent::get_eventshelper correctly handles both v4-first and v3-fallback AMT loading and preserves storage order, which aligns with the new state-manager tests. Two minor follow‑ups to consider:
#[serde(untagged)]onStampedEventchanges its serialized shape (no variant tag). Please double‑check that nothing persists or exchanges the old tagged representation; if there is such usage, a compatibility note or migration test would help.- If you expect large event AMTs, you can avoid reallocations by reserving
eventscapacity from the AMT’s element count beforefor_each.src/state_manager/tests.rs (1)
341-406: Cache tests are correct; consider aligning events_root construction with productionThe receipt/events cache tests behave correctly:
update_cache_with_state_outputonly uses the providedeventsvector and treatsevents_rootas an opaque identifier, so building the events root with Amtv0 is safe here. For future clarity (especially now that production writes events into a specific-bitwidth AMT), it may be worth switching this test’s events_root construction to the same AMT flavor/bitwidth used in the main code, or adding a comment noting that the root isn’t dereferenced in this test.src/state_manager/mod.rs (1)
99-105: StateEvents and cache update logic look coherent; consider asserting vector invariantsMaking
StateEventspublic and wiringupdate_cache_with_state_outputto insert both events and their roots into the receipt/event cache handler matches howtipset_state_eventsand the RPC layer consume this data. One implicit assumption is thatevents.len()andevents_roots.len()always stay equal; given other code zips these two vectors, it may be worth adding a debug/assert (either when creatingStateEventsor before caching) to catch any future mismatch early.Also applies to: 511-528
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/chain/store/chain_store.rs(1 hunks)src/rpc/methods/chain.rs(1 hunks)src/rpc/methods/eth/filter/mod.rs(1 hunks)src/shim/executor.rs(3 hunks)src/state_manager/mod.rs(4 hunks)src/state_manager/tests.rs(4 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-02T14:23:53.808Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/tool/subcommands/api_cmd/test_snapshots.txt:206-252
Timestamp: 2025-09-02T14:23:53.808Z
Learning: In the Forest project, .rpcsnap.json.zst snapshot files listed in src/tool/subcommands/api_cmd/test_snapshots.txt are not stored in the repository itself but are downloaded from a DigitalOcean (DO) bucket when needed. The manifest file serves as an index/catalog of available snapshots.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshots.txt
📚 Learning: 2025-08-25T14:11:10.790Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5978
File: .github/workflows/unit-tests.yml:86-96
Timestamp: 2025-08-25T14:11:10.790Z
Learning: hanabi1224's RPC snapshot tests include a checksum validation mechanism that compares against HTTP HEAD headers, automatically re-downloading snapshots if the cached version doesn't match, providing protection against corrupted cached files.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshots.txt
📚 Learning: 2025-07-17T15:21:40.753Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5841
File: src/utils/get_size/mod.rs:10-10
Timestamp: 2025-07-17T15:21:40.753Z
Learning: The get_size2 crate's GetSize trait provides default implementations: get_stack_size() uses std::mem::size_of, get_heap_size() returns 0, and get_size() returns their sum. An empty impl like `impl GetSize for MyType {}` is valid and uses these defaults, making it suitable for simple wrapper types that don't allocate heap memory.
Applied to files:
src/shim/executor.rs
📚 Learning: 2025-08-08T12:11:55.266Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.
Applied to files:
src/rpc/methods/chain.rs
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/state_manager/tests.rssrc/state_manager/mod.rs
🧬 Code graph analysis (2)
src/chain/store/chain_store.rs (2)
src/shim/executor.rs (1)
key(305-310)src/blocks/tipset.rs (2)
key(337-340)key(512-515)
src/state_manager/tests.rs (1)
src/shim/executor.rs (4)
events_root(180-186)event(383-388)events(118-124)get_events(392-414)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
🔇 Additional comments (7)
src/tool/subcommands/api_cmd/test_snapshots.txt (1)
7-7: The snapshot filefilecoin_chaingetevents_1764864316078100.rpcsnap.json.zstis correctly listed in the manifest and is already available in the DigitalOcean bucket (https://forest-snapshots.fra1.cdn.digitaloceanspaces.com/rpc_test/). The change is valid and the test suite will be able to download this snapshot at runtime.src/rpc/methods/eth/filter/mod.rs (1)
379-395: Preserving AMT event duplicates and order is correctThe updated
collect_chain_eventsflow (filter by root, then flatten without dedup) now faithfully reflects the AMT contents, including duplicates and order, which is consistent with the new events tests and the API goal of returning all emitted events.src/chain/store/chain_store.rs (1)
201-208: Renamed tipset-key lookup clarifies events-root usageThe new
get_tipset_key_by_events_rootwrapper keeps the previous behavior while making the intent (lookup by events root) explicit, which matches its use fromChainGetEvents.src/rpc/methods/chain.rs (1)
254-263: ChainGetEvents now correctly resolves via events-root indexSwitching to
get_tipset_key_by_events_rootand updating the error message to reference the tipset for the given events root aligns the RPC with the new indexing logic; the subsequent tipset load and event collection remain consistent.src/state_manager/mod.rs (3)
471-595: Event-root indexing and blockstore fallback path are sensible; confirm all relevant tipsets pass through hereThe new logic in
tipset_state_outputandtipset_state_eventsgives a clear pipeline:
- When computing state, you index each non‑
Noneevents_root to itsTipsetKey.- When serving events, you first try the cache, then load from the events AMT in the blockstore (when an
events_rootis provided), and only fall back to recomputing state if that fails.This is a good layering and matches how
ChainGetEventsnow resolves root → tipset. One thing to verify is that, for all tipsets whose events you expect clients to query by root, there is at least one code path that callstipset_state_output(and thus writes these indices). If some validation or background paths only ever callapply_block_messagesdirectly without going throughtipset_state_output, their events_root mappings won’t be written and ChainGetEvents for those particular roots will correctly, but perhaps unexpectedly, return the “can't find tipset for events root …” error.
1898-2034: Receipt and events AMT construction/verification in apply_block_messages is well-structuredWithin
apply_block_messages, constructing the receipts root from the VM‑returned receipts and then, separately, materializing each non‑empty events AMT and asserting that its derived root matches the VM‑provided root gives you:
- A receipts root consistent with what
Receipt::get_receiptsexpects to read.- Events AMTs that are actually persisted, with an explicit equality check guarding against any divergence between how the VM computes roots and how the node rebuilds them (including bitwidth configuration).
The error path on an events‑root mismatch is clear and will surface early during tipset execution, which is appropriate for a consensus‑sensitive invariant.
1769-1819: validate_tipsets now bails cleanly on state mismatchSwitching
validate_tipsetsto usebail!("state mismatch")when the expected and actual state/receipt roots differ simplifies the error path without changing behavior, and the surrounding logging still records all the relevant details for debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/state_manager/tests.rs (1)
437-625: Event AMT tests comprehensively cover duplicates, order, empties, and v3/v4 compatibilityThe new helpers and tests look solid:
create_raw_event_v3/v4generate deterministic RAW IPLD events, which is ideal for CID equality checks.test_events_store_and_retrieve_basic,test_events_entries_are_preserved_when_duplicates_are_stored, andtest_events_preserve_orderdirectly exerciseStampedEvent::get_eventsand confirm the “no dedup, preserve insertion order” behavior that this PR is aiming for.test_events_same_content_same_cidandtest_identical_events_produce_same_roottogether validate that identical event sequences yield identical AMT roots and remain readable throughget_events.test_events_empty_listandtest_events_v3_store_and_retrievenicely cover the empty‑root and v3‑compat paths.One small, purely optional nit:
test_events_same_content_same_cidandtest_identical_events_produce_same_rootoverlap quite a bit; you could fold them into a single test that asserts both CID equality and successful retrieval to reduce duplication, but it's not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/tests/api_compare/filter-list(0 hunks)scripts/tests/api_compare/filter-list-offline(0 hunks)src/state_manager/tests.rs(4 hunks)
💤 Files with no reviewable changes (2)
- scripts/tests/api_compare/filter-list-offline
- scripts/tests/api_compare/filter-list
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/state_manager/tests.rs
🧬 Code graph analysis (1)
src/state_manager/tests.rs (1)
src/shim/executor.rs (4)
events_root(180-186)event(383-388)events(118-124)get_events(392-414)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: All lint checks
🔇 Additional comments (2)
src/state_manager/tests.rs (2)
14-14: Importing bothAmtandAmtv0keeps tests aligned with runtime AMT variantsUsing
Amtfor the newEVENTS_AMT_BITWIDTH‑backed event AMTs andAmtv0for legacy roots (receipts / earlier tests) makes the intent explicit and avoids alias confusion; usage below is consistent with this split. No changes needed here, just noting the clarity.
351-352:Amtv0::new_from_itercorrectly mirrors v0‑formatted roots in cache testsSwitching both the mock
events_rootandreceipt_rootconstruction toAmtv0::new_from_iterensures these tests only pass when the blockstore actually contains a valid v0 AMT, matching how roots are stored/loaded in the state manager. This gives the cache tests more realistic coverage without overcomplicating them.Also applies to: 388-389
akaladarshi
force-pushed
the
akaladarshi/fix-get-events-api
branch
from
b1ae8f4 to
ad77eee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/state_manager/mod.rs (2)
511-528: tipset_state_events fast path currently poisons the events cache with partial dataWhen
events_rootis provided, the new logic:
- Checks the cache via
get_events_or_else.- Inside the supplier closure, tries to load only the events for that root via
StampedEvent::get_events, and if found, returns aStateEventscontaining a singleVec<StampedEvent>and a singleSome(root).Because this
StateEventsis then cached under the tipset key, later callers that expect a full tipset view (e.g.,collect_eventswithevents_root = None) will:
- Pull this truncated
StateEventsfrom the cache.- Hit the new length check in
collect_events(messages.len() == events.len()), and fail with a length mismatch.This can manifest whenever
Filecoin.ChainGetEventsis called for a tipset before any filter-based event queries.A safer pattern would be:
- Use the blockstore fast path outside of
get_events_or_elseand do not insert that partialStateEventsinto the cache, or- Restrict the cache to always store the full per-tipset
events/rootsproduced bycompute_tipset_state, regardless of whether anevents_roothint is present.For example, you could restructure
tipset_state_eventsroughly as:@@ - let key = tipset.key(); - let ts = tipset.clone(); - let this = Arc::clone(self); - let cids = tipset.cids(); - let events_root = events_root.cloned(); - self.receipt_event_cache_handler - .get_events_or_else( - key, - Box::new(move || { - Box::pin(async move { - // Try to load events directly from the blockstore - if let Some(stamped_events) = events_root - .as_ref() - .and_then(|root| StampedEvent::get_events(this.blockstore(), root).ok()) - .filter(|events| !events.is_empty()) - { - return Ok(StateEvents { - events: vec![stamped_events], - roots: vec![events_root], - }); - } - - // Fallback: compute the tipset state if events not found in the blockstore - let state_out = this - .compute_tipset_state(ts, NO_CALLBACK, VMTrace::NotTraced) - .await?; - trace!("Completed tipset state calculation {:?}", cids); - Ok(StateEvents { - events: state_out.events, - roots: state_out.events_roots, - }) - }) - }), - ) - .await + let events_root = events_root.cloned(); + + // Fast path: if a specific events root is requested and the AMT exists, + // serve it directly without mutating the per‑tipset cache. + if let Some(root) = events_root.as_ref() { + if let Ok(stamped_events) = StampedEvent::get_events(self.blockstore(), root) { + if !stamped_events.is_empty() { + return Ok(StateEvents { + events: vec![stamped_events], + roots: vec![events_root], + }); + } + } + } + + let key = tipset.key(); + let ts = tipset.clone(); + let this = Arc::clone(self); + let cids = tipset.cids(); + self.receipt_event_cache_handler + .get_events_or_else( + key, + Box::new(move || { + Box::pin(async move { + let state_out = this + .compute_tipset_state(ts, NO_CALLBACK, VMTrace::NotTraced) + .await?; + trace!("Completed tipset state calculation {:?}", cids); + Ok(StateEvents { + events: state_out.events, + roots: state_out.events_roots, + }) + }) + }), + ) + .awaitThis keeps the cache invariant (“cached StateEvents represent the full tipset”) while still leveraging the AMT-backed fast path for
ChainGetEvents.Also applies to: 555-595
199-205: Verify that event index insertion is not bypassed in future state computation paths
tipset_state_outputindexes events roots viaput_indexafter computing state, but this indexing only occurs in two paths:
- Main RPC path via
tipset_state_output(line 497)- Backfill utility via
process_tsin db_util.rsDirect calls to
compute_tipset_stateorapply_block_messagesoutside these two paths—such as inexecution_trace, validation, or tool commands—do not trigger indexing. While current production paths (sync viatipset_syncer, RPC viatipset_state) route throughtipset_state_outputproperly, future code that calls these functions directly risks creating persisted state without index entries, breakingChainGetEventsresolution.Consider either consolidating all state computations through
tipset_state_output, or moving theput_indexcall to the point where events roots are produced (e.g., inapply_block_messagesaftervm.flush()), to guarantee indexing regardless of caller.
🧹 Nitpick comments (3)
src/rpc/methods/eth/filter/mod.rs (1)
281-291: Stricter message/events length check is appropriateThe added
ensure!with concrete lengths will fail fast instead of silently zipping mismatchedmessagesandevents, which is much safer for debugging and prevents misaligned event attribution.src/state_manager/tests.rs (1)
437-625: Event AMT tests give good coverage of storage, order, duplicates, and v3/v4 handlingThe new helpers (
create_raw_event_v3/v4) and tests comprehensively cover:
- Basic store/retrieve via
EVENTS_AMT_BITWIDTHandStampedEvent::get_events.- Duplicate events (consecutive and non‑consecutive) preserving count and position.
- Strict order preservation across multiple entries.
- Identical content producing identical root CIDs.
- Empty-event lists round‑tripping correctly.
- V3-encoded events still being readable through
StampedEvent::get_events.The slight overlap between
test_events_same_content_same_cidandtest_identical_events_produce_same_rootis acceptable and keeps intent clear.src/state_manager/mod.rs (1)
1899-2035: Receipt and events AMT construction in apply_block_messages is consistent and well-guardedThe updated
apply_block_messagespipeline:
- Uses
Amtv0::new_from_iterto buildreceipt_rootfrom the receipts vector, aligning with the tests that now construct receipts usingAmtv0.- Iterates
(events, events_roots)and, when anevents_rootis present, reconstructs the AMT viaAmt::new_from_iter_with_bit_widthandEVENTS_AMT_BITWIDTH, thenensure!s that the derived root matches the FVM-provided root.That
ensure!gives a clear, early failure if the bitwidth or serialization diverge from what the VM produced, which is valuable for catching subtle incompatibilities at state-application time.As long as
EVENTS_AMT_BITWIDTHstays coordinated with the bitwidth used when events roots are produced, this looks sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/chain/store/chain_store.rs(1 hunks)src/rpc/methods/chain.rs(1 hunks)src/rpc/methods/eth/filter/mod.rs(3 hunks)src/shim/executor.rs(3 hunks)src/state_manager/mod.rs(4 hunks)src/state_manager/tests.rs(4 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tool/subcommands/api_cmd/test_snapshots.txt
- src/shim/executor.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/state_manager/tests.rssrc/state_manager/mod.rs
📚 Learning: 2025-08-08T12:11:55.266Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.
Applied to files:
src/rpc/methods/chain.rs
🧬 Code graph analysis (2)
src/chain/store/chain_store.rs (2)
src/shim/executor.rs (1)
key(305-310)src/blocks/tipset.rs (2)
key(337-340)key(512-515)
src/state_manager/mod.rs (2)
src/shim/executor.rs (5)
events_root(180-186)key(305-310)new(254-266)get_events(392-414)events(118-124)src/state_manager/cache.rs (3)
get_events(152-152)get_events(206-208)get_events(263-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
🔇 Additional comments (5)
src/chain/store/chain_store.rs (1)
201-208: Method rename to clarify events-root index usage looks goodRenaming to
get_tipset_key_by_events_rootbetter reflects how the indices store is used here; behavior remains a straight passthrough toindices.read_objand is consistent with the ChainGetEvents call site.src/rpc/methods/chain.rs (1)
248-265: ChainGetEvents now resolves tipset explicitly via events-root indexSwitching to
get_tipset_key_by_events_rootwith the updated error context accurately describes the failure mode while keeping the rest of the flow (loading the tipset and collecting events) unchanged.src/rpc/methods/eth/filter/mod.rs (1)
369-403: ChainGetEvents now preserves duplicate events and order as stored in AMTThe new
ensure!onstate_events.roots.len() == state_events.events.len()plus removal of any deduplication ensures:
- You only proceed when roots and event vectors are structurally consistent.
- All events under the selected root (including intentional duplicates) are returned in AMT order.
This aligns with the stated goal of matching Lotus’ behavior for
ChainGetEvents.src/state_manager/tests.rs (1)
340-407: Receipt/events cache tests align with new AMTv0 usageUsing
Amtv0::new_from_iterto buildreceipt_rootand the mockevents_rootmatches the production change to useAmtv0for receipts while treating event roots as opaqueCids in the cache. The tests correctly assert that empty events do not populate the cache and that a valid AMT-backedreceipt_rootyields one cached receipt.src/state_manager/mod.rs (1)
93-105: StateEvents layout and sizing helpers look consistentMaking
StateEventspublic withevents: Vec<Vec<StampedEvent>>androots: Vec<Option<Cid>>, plus theGetSizeannotations, matches how it’s consumed by the cache layer and RPC code. The explicit heap-size helpers for nested vectors should keep cache accounting accurate.
akaladarshi
force-pushed
the
akaladarshi/fix-get-events-api
branch
from
17bdc74 to
51357fe
Compare
akaladarshi
force-pushed
the
akaladarshi/fix-get-events-api
branch
from
51357fe to
35ac348
Compare
|
DB migration check in CI is failing because I've removed the I was thinking, if I can get some comments on the PR, to finalise before the migration, it would be helpful 🙏 @hanabi1224 @LesnyRumcajs |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/state_manager/mod.rs (1)
1985-2006: Consider adding a length check before the zip operation.The
zipat line 1989 will silently truncate ifeventsandevents_rootshave different lengths. Whileapply_block_messagesshould return consistent lengths, adding an explicit check would catch bugs early.// step 5: construct receipt root from receipts let receipt_root = Amtv0::new_from_iter(chain_index.db(), receipts)?; // step 6: store events AMTs in the blockstore + ensure!( + events.len() == events_roots.len(), + "Events count ({}) and events_roots count ({}) mismatch", + events.len(), + events_roots.len() + ); for (msg_events, events_root) in events.iter().zip(events_roots.iter()) {src/rpc/methods/eth/filter/mod.rs (1)
368-383: LGTM onget_events_by_event_rootimplementation.The function correctly loads events via
StampedEvent::get_eventsfrom the blockstore without deduplication, aligning with Lotus behavior. The error message at line 377 provides useful context.Consider simplifying the error handling pattern:
- let state_events = - match StampedEvent::get_events(ctx.chain_store().blockstore(), events_root) { - Ok(e) => e, - Err(e) => { - return Err(anyhow::anyhow!("load events amt: {}", e)); - } - }; + let state_events = StampedEvent::get_events(ctx.chain_store().blockstore(), events_root) + .context("load events amt")?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/benchmark_private/tipset_validation.rs(0 hunks)src/chain/store/chain_store.rs(3 hunks)src/chain_sync/chain_follower.rs(0 hunks)src/daemon/context.rs(0 hunks)src/daemon/db_util.rs(1 hunks)src/db/car/many.rs(1 hunks)src/db/memory.rs(1 hunks)src/db/mod.rs(0 hunks)src/db/parity_db.rs(1 hunks)src/libp2p/chain_exchange/provider.rs(0 hunks)src/rpc/methods/chain.rs(1 hunks)src/rpc/methods/eth/filter/mod.rs(3 hunks)src/rpc/methods/sync.rs(1 hunks)src/state_manager/mod.rs(4 hunks)src/state_manager/tests.rs(4 hunks)src/tool/offline_server/server.rs(0 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(0 hunks)src/tool/subcommands/api_cmd/generate_test_snapshot.rs(1 hunks)src/tool/subcommands/api_cmd/test_snapshot.rs(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)src/tool/subcommands/index_cmd.rs(0 hunks)src/tool/subcommands/state_compute_cmd.rs(0 hunks)
💤 Files with no reviewable changes (9)
- src/tool/offline_server/server.rs
- src/libp2p/chain_exchange/provider.rs
- src/tool/subcommands/state_compute_cmd.rs
- src/daemon/context.rs
- src/db/mod.rs
- src/tool/subcommands/index_cmd.rs
- src/benchmark_private/tipset_validation.rs
- src/tool/subcommands/api_cmd/api_compare_tests.rs
- src/chain_sync/chain_follower.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tool/subcommands/api_cmd/test_snapshots.txt
- src/state_manager/tests.rs
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-17T14:24:47.046Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6167
File: src/tool/subcommands/state_compute_cmd.rs:89-91
Timestamp: 2025-10-17T14:24:47.046Z
Learning: In `src/tool/subcommands/state_compute_cmd.rs`, when using `ReadOpsTrackingStore` to generate minimal snapshots, `HEAD_KEY` should be written to `db.tracker` (not `db` itself) before calling `export_forest_car()`, because the export reads from the tracker MemoryDB which accumulates only the accessed data during computation.
Applied to files:
src/db/memory.rssrc/daemon/db_util.rssrc/tool/subcommands/api_cmd/generate_test_snapshot.rssrc/tool/subcommands/api_cmd/test_snapshot.rs
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/state_manager/mod.rssrc/db/car/many.rssrc/tool/subcommands/api_cmd/generate_test_snapshot.rs
📚 Learning: 2025-08-08T12:11:55.266Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.
Applied to files:
src/rpc/methods/chain.rssrc/daemon/db_util.rssrc/rpc/methods/eth/filter/mod.rssrc/tool/subcommands/api_cmd/test_snapshot.rs
📚 Learning: 2025-08-25T13:35:24.230Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5969
File: src/tool/subcommands/snapshot_cmd.rs:412-412
Timestamp: 2025-08-25T13:35:24.230Z
Learning: In src/tool/subcommands/snapshot_cmd.rs, the +1 in `last_epoch = ts.epoch() - epochs as i64 + 1` fixes an off-by-1 bug where specifying --check-stateroots=N would validate N+1 epochs instead of N epochs, causing out-of-bounds errors when the snapshot contains only N recent state roots.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshot.rs
📚 Learning: 2025-08-11T14:00:47.060Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: src/daemon/db_util.rs:236-259
Timestamp: 2025-08-11T14:00:47.060Z
Learning: In Forest's snapshot import (`src/daemon/db_util.rs`), when F3 CID is present in snapshot metadata but the actual F3 data is missing, this should cause a hard error as it indicates snapshot corruption. Only errors during the F3 import process itself (after data is successfully retrieved) should be non-fatal and logged.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshot.rs
🧬 Code graph analysis (8)
src/db/memory.rs (3)
src/chain/store/chain_store.rs (1)
db(503-503)src/chain/store/index.rs (1)
db(48-50)src/rpc/methods/state.rs (1)
db(1443-1444)
src/rpc/methods/chain.rs (3)
src/rpc/methods/eth/filter/mod.rs (1)
get_events_by_event_root(369-383)src/tool/subcommands/api_cmd/generate_test_snapshot.rs (1)
ctx(99-151)src/tool/subcommands/api_cmd/test_snapshot.rs (1)
ctx(122-169)
src/db/car/many.rs (3)
src/db/memory.rs (2)
SettingsStoreExt(32-32)SettingsStoreExt(156-156)src/db/parity_db.rs (1)
SettingsStoreExt(177-177)src/chain/mod.rs (1)
SettingsStoreExt(45-45)
src/rpc/methods/sync.rs (1)
src/chain/store/chain_store.rs (4)
new(117-142)new(567-571)db(503-503)chain_config(247-249)
src/tool/subcommands/api_cmd/generate_test_snapshot.rs (5)
src/db/car/many.rs (1)
SettingsStoreExt(256-256)src/db/memory.rs (2)
SettingsStoreExt(32-32)SettingsStoreExt(156-156)src/db/parity_db.rs (1)
SettingsStoreExt(177-177)src/chain/mod.rs (1)
SettingsStoreExt(45-45)src/db/mod.rs (1)
open_db(283-285)
src/chain/store/chain_store.rs (3)
src/chain/store/index.rs (2)
db(48-50)new(42-46)src/state_manager/mod.rs (2)
new(181-183)chain_config(359-361)src/daemon/context.rs (1)
chain_config(65-67)
src/rpc/methods/eth/filter/mod.rs (1)
src/shim/executor.rs (3)
events(118-124)events_root(180-186)get_events(392-414)
src/tool/subcommands/api_cmd/test_snapshot.rs (1)
src/chain/store/chain_store.rs (6)
db(503-503)eth_mappings(180-182)eth_mappings(196-198)new(117-142)new(567-571)chain_config(247-249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
🔇 Additional comments (25)
src/db/car/many.rs (1)
14-15: LGTM - Import cleanup aligns with IndicesStore removal.The consolidated import statement correctly removes
IndicesStorewhile retaining all other necessary traits forManyCarfunctionality.src/db/memory.rs (1)
6-6: LGTM - Clean removal of IndicesStore import.The import change correctly reflects the removal of the
IndicesStoretrait implementation fromMemoryDB.src/chain/store/chain_store.rs (3)
9-9: LGTM - Import cleanup consistent with IndicesStore removal.The import correctly removes
IndicesStoreandIndicesStoreExtwhile retainingEthMappingsStoreandEthMappingsStoreExtwhich are still used for Ethereum mapping functionality.
743-744: Test correctly updated for new ChainStore::new signature.The test now passes 5 arguments matching the updated constructor:
(db, heaviest_tipset_key_provider, eth_mappings, chain_config, genesis_block_header).
758-758: LGTM - Consistent with other test updates.src/rpc/methods/sync.rs (1)
189-191: LGTM - Test correctly updated for new ChainStore::new signature.The
ctx()function now constructsChainStorewith the updated 5-argument signature, consistent with the removal of theIndicesStoreparameter.src/tool/subcommands/api_cmd/test_snapshot.rs (2)
55-64: Simplified backfill logic using let-chains.The refactored function cleanly chains the conditionals. Note that
try_write()will silently skip the backfill if the lock is already held elsewhere. This is likely acceptable for test scenarios but worth being aware of.
134-140: Improved error handling with?operator.Good change - using
?instead ofunwrap()properly propagates errors through theanyhow::Resultreturn type.src/daemon/db_util.rs (1)
332-334: Event root indexing removed from tipset processing.The
compute_tipset_stateresult is no longer unpacked to extract and indexevents_roots. This aligns with the PR objective of serving events directly from the blockstore rather than maintaining a separate index. Events are now stored as part of state computation and retrieved viaStampedEvent::get_eventswhen needed.src/rpc/methods/chain.rs (2)
254-256: Simplified event retrieval aligned with Lotus behavior.The handler now directly fetches events from the blockstore via
EthEventHandler::get_events_by_event_root. This removes the previous tipset-resolution flow and duplicate-event logic that caused issue #6271. If events aren't present in the blockstore, the call will error out with an AMT load error, matching Lotus's behavior.
1610-1618: Test helper correctly updated for new ChainStore signature.The
_loadhelper now constructsChainStorewith the updated 5-argument signature, using separateMemoryDBinstances forheaviest_tipset_key_providerandeth_mappings.src/tool/subcommands/api_cmd/generate_test_snapshot.rs (3)
13-14: LGTM on import updates.The imports are correctly updated to remove
IndicesStorewhile retaining the necessaryEthMappingsStore,SettingsStore, and related types for the tracking store functionality.
83-97: LGTM on simplifiedbuild_indexfunction.The function now only builds the
eth_mappingsindex from the tracker, which aligns with the removal of the indices store column. The logic correctly returnsNonewhen no mappings are present.
112-121: VerifyChainStore::newsignature consistency.The constructor takes 5 parameters:
db(Arc),heaviest_tipset_key_provider(Arc<dyn HeaviestTipsetKeyProvider + Sync + Send>),eth_mappings(Arc<dyn EthMappingsStore + Sync + Send>),chain_config, andgenesis_block_header. All call sites consistently pass the same db instance for the first three parameters, which is valid since the concrete db type implements all required traits.src/db/parity_db.rs (3)
4-4: LGTM on import simplification.The
IndicesStoreimport is correctly removed, leaving only the necessary store traits.
23-42: LGTM onDbColumnenum update.The
Indicesvariant is cleanly removed. The remaining columns (GraphDagCborBlake2b256,GraphFull,Settings,EthMappings,PersistentGraph) are properly ordered and documented.
44-81: LGTM oncreate_column_optionsupdate.The match arms correctly handle all remaining column variants with appropriate options (preimage, btree_index, compression settings).
src/state_manager/mod.rs (6)
64-64: LGTM on import update.Adding
ensurefromanyhowis appropriate for the new validation logic in step 6.
99-105: LGTM onStateEventsstruct.The struct appropriately holds paired
eventsandrootsvectors with proper derives for debugging, cloning, and memory tracking.
550-576: LGTM ontipset_state_eventsfallback logic.The method now properly falls back to computing the tipset state when events are not found in cache, returning a
StateEventsstruct with both events and roots.
1992-2003: Good defensive check for AMT root consistency.The
ensure!macro correctly validates that the derived event root matches the FVM-computed root. This is essential for detecting any discrepancies in event serialization or AMT construction.
94-94: EVENTS_AMT_BITWIDTH value is verified against FVM at runtime.The bitwidth of 5 is already validated through the equality check at lines 2000-2001, where the derived event root is compared against the FVM-computed root. If this constant were misaligned with FVM's event AMT construction, the
ensure!()check would fail. No additional verification needed.
71-71: Confirm the intentionality of usingAmtv0for receipt roots andAmtfor event roots.The code uses
Amtv0::new_from_iterfor receipt construction andAmt::new_from_iter_with_bit_widthfor events. WhileAmtv0is documented as the legacy AMT encoding (for backward compatibility) andAmtis the current encoding, the codebase lacks explicit documentation explaining why receipts specifically requireAmtv0. Consider adding a comment referencing the Lotus behavior or actor specification that mandates this version distinction.src/rpc/methods/eth/filter/mod.rs (2)
40-40: LGTM onStampedEventimport.The import is correctly added to support the new
get_events_by_event_rootimplementation that loads events directly viaStampedEvent::get_events.
283-290: LGTM on improved error messages.The error message now includes the concrete lengths for better debugging when there's a mismatch between messages and events.
Before moving forward without the indices column, I'd like to first see some numbers showing the performance impact for affected methods. |
| match StampedEvent::get_events(ctx.chain_store().blockstore(), events_root) { | ||
| Ok(e) => e, | ||
| Err(e) => { | ||
| return Err(anyhow::anyhow!("load events amt: {}", e)); |
There was a problem hiding this comment.
nit
| return Err(anyhow::anyhow!("load events amt: {}", e)); | |
| bail!("load events amt: {e}"); |
There was a problem hiding this comment.
Actually, probably let state_events = bla.context("failed getting events")?; would make more sense.
There was a problem hiding this comment.
The current error doesn't make much sense but I was trying to match lotus API level error , hence this
| ) -> anyhow::Result<Vec<StampedEvent>> { | ||
| let mut events = Vec::new(); | ||
|
|
||
| // Try StampedEvent_v4 first (StampedEvent_v4 and StampedEvent_v3 are identical, use v4 here) |
There was a problem hiding this comment.
are there other versions as well?
There was a problem hiding this comment.
No, for events there is only two versions , V3 and V4,
Line 342 in f4008dd
src/shim/executor.rs
Outdated
|
|
||
| // Try StampedEvent_v4 first (StampedEvent_v4 and StampedEvent_v3 are identical, use v4 here) | ||
| if let Ok(amt) = Amt::<StampedEvent_v4, _>::load(events_root, db) { | ||
| amt.for_each(|_, event| { |
There was a problem hiding this comment.
for_each_cacheless is a better option yielding faster iteration if the AMT is not iterated multiple times.
| // step 5: construct receipt root from receipts and flush the state-tree | ||
| let receipt_root = Amt::new_from_iter(chain_index.db(), receipts)?; | ||
| // step 5: construct receipt root from receipts | ||
| let receipt_root = Amtv0::new_from_iter(chain_index.db(), receipts)?; |
There was a problem hiding this comment.
Earlier it was V0 only, but we renamed it to Amt .
It was being used like this
use fil_actors_shared::fvm_ipld_amt::Amtv0 as Amt;
| // step 5: construct receipt root from receipts | ||
| let receipt_root = Amtv0::new_from_iter(chain_index.db(), receipts)?; | ||
|
|
||
| // step 6: store events AMTs in the blockstore |
There was a problem hiding this comment.
is there corresponding Lotus logic?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/shim/executor.rs (1)
390-414:get_eventsimplementation matches goals (v4‑first, v3 fallback, ordered, no dedup)The new
StampedEvent::get_eventscorrectly:
- Loads from the events AMT root via
Amt<StampedEvent_v4>first, then cleanly falls back toAmt<StampedEvent_v3>on error.- Uses
for_each_cacheless, which is appropriate for single‑pass iteration and preserves AMT index order, so events are returned in the stored order with duplicates intact (aligning with the new ChainGetEvents behavior).If you expect very large event sets, you could optionally pre‑allocate
eventswithevents.reserve(amt.count() as usize)before iteration for a minor perf win, but that’s not required for correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/shim/executor.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-17T15:21:40.753Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5841
File: src/utils/get_size/mod.rs:10-10
Timestamp: 2025-07-17T15:21:40.753Z
Learning: The get_size2 crate's GetSize trait provides default implementations: get_stack_size() uses std::mem::size_of, get_heap_size() returns 0, and get_size() returns their sum. An empty impl like `impl GetSize for MyType {}` is valid and uses these defaults, making it suitable for simple wrapper types that don't allocate heap memory.
Applied to files:
src/shim/executor.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: tests-release
- GitHub Check: cargo-publish-dry-run
- GitHub Check: tests
- GitHub Check: All lint checks
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Build MacOS
🔇 Additional comments (2)
src/shim/executor.rs (2)
10-10: Amt import aligns with new events AMT usagePulling in
AmtalongsideAmtv0is consistent with using the new AMT API for events while keeping receipts onAmtv0. Please just double‑check that the version ofAmthere matches how events are actually stored and how Lotus reads them, so ChainGetEvents parity is guaranteed end‑to‑end.
340-346: UntaggedStampedEventserialization looks appropriate for RPC parityAdding
#[serde(untagged)]toStampedEventmatches howReceiptis handled and should make the serialized shape match the underlying FVM event types (and Lotus) instead of introducing an enum wrapper. Since we only deriveSerialize, this shouldn’t introduce deserialization ambiguity. Please confirm that no external clients relied on the previously tagged JSON shape ofStampedEvent(if it was ever exposed directly).
Summary of changes
Changes introduced in this pull request:
blockstoreevents root->tipsetKeymappingblockstoreto serve theChainGetEventsAPI, and if events are not available we just error out, same as lotusReference issue to close (if applicable)
Closes #6271
Other information and links
The issue was: A message generated two identical events and
forestalways used to compute the state and get the events for serving theChainGetEventsAPI call request, so we were always getting two identical events, but thelotuswas serving directly from the blockstore so they were getting single event.Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.