refactor state manager mod into multiple sub mods#6864
Open
akaladarshi wants to merge 2 commits intomainfrom
Open
refactor state manager mod into multiple sub mods#6864akaladarshi wants to merge 2 commits intomainfrom
akaladarshi wants to merge 2 commits intomainfrom
Conversation
Contributor
WalkthroughThis PR refactors the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
akaladarshi
force-pushed
the
akaladarshi/refactor-stmgr
branch
from
a40c2d9 to
2c9f42a
Compare
hanabi1224
reviewed
Apr 14, 2026
akaladarshi
force-pushed
the
akaladarshi/refactor-stmgr
branch
from
259a36f to
7e82e1d
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/state_manager/message_search.rs (1)
99-102: Remove theunwrap()from this boundary check.The short-circuit makes this safe today, but Line 101 is still a production
unwrap()on a hot path. A small rewrite withis_some_andormatchkeeps the condition panic-free if this branch gets rearranged later.As per coding guidelines "Avoid `unwrap()` in production code; use `?` or `expect()` with descriptive messages instead".♻️ Suggested change
if parent_actor_state.is_none() || (current_actor_state.sequence > message_sequence - && parent_actor_state.as_ref().unwrap().sequence <= message_sequence) + && parent_actor_state + .as_ref() + .is_some_and(|state| state.sequence <= message_sequence)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state_manager/message_search.rs` around lines 99 - 102, The condition using parent_actor_state.unwrap() is unsafe; replace the unwrap by checking the Option directly (e.g., use Option::is_some_and(|p| p.sequence <= message_sequence) or a match on parent_actor_state) so the expression becomes panic-free while keeping the same logic comparing current_actor_state.sequence and message_sequence; update the conditional that currently references parent_actor_state.is_none() and parent_actor_state.as_ref().unwrap().sequence to use a safe Option check involving parent_actor_state and its sequence field (variables: parent_actor_state, current_actor_state.sequence, message_sequence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state_manager/actor_queries.rs`:
- Around line 171-174: The error message for the branch that loads
Address::DATACAP_TOKEN_ACTOR is wrong; update the Error::state call in the
get_actor result handling (where get_actor(&Address::DATACAP_TOKEN_ACTOR,
*ts.parent_state()) is used) to return a message indicating the datacap actor is
missing (e.g., "Datacap actor not found") instead of "Miner actor not found" so
logs and RPC responses correctly reflect the failing actor.
In `@src/state_manager/message_search.rs`:
- Around line 208-230: The code is recording applied tipsets into block_revert
instead of actual reverts, so search_back_poll never marks the backward-search
tipset as reverted; update the logic so that when observing head_changes.reverts
you insert the reverted tipset key into block_revert (use
tipset.key().to_owned()), and remove the insertion from the head_changes.applies
loop; adjust around candidate_tipset/candidate_receipt checks in the applies
loop so only applies drive acceptance while reverts populate block_revert; refer
to head_changes.reverts, head_changes.applies, candidate_tipset,
candidate_receipt, block_revert, search_back_poll, and wait_for_message to
locate the change.
- Around line 301-307: The code incorrectly continues the backward walk from
self.heaviest_tipset(), allowing a receipt that happened after the caller's
starting tipset; instead resume the search from the caller's provided starting
tipset (`from`). Update the call on the last line to continue from `from` (e.g.,
call `self.search_back_for_message(from, &message, look_back_limit,
allow_replaced)` or pass `Some(from)` into `search_for_message` as appropriate)
so the backward walk does not advance to `self.heaviest_tipset()`; keep the
existing fast-path `self.tipset_executed_message(&from, &message, ...)` check
intact.
---
Nitpick comments:
In `@src/state_manager/message_search.rs`:
- Around line 99-102: The condition using parent_actor_state.unwrap() is unsafe;
replace the unwrap by checking the Option directly (e.g., use
Option::is_some_and(|p| p.sequence <= message_sequence) or a match on
parent_actor_state) so the expression becomes panic-free while keeping the same
logic comparing current_actor_state.sequence and message_sequence; update the
conditional that currently references parent_actor_state.is_none() and
parent_actor_state.as_ref().unwrap().sequence to use a safe Option check
involving parent_actor_state and its sequence field (variables:
parent_actor_state, current_actor_state.sequence, message_sequence).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b62694d-597a-46eb-926a-41c3e75a3007
📒 Files selected for processing (8)
src/state_manager/actor_queries.rssrc/state_manager/address_resolution.rssrc/state_manager/execution.rssrc/state_manager/message_search.rssrc/state_manager/message_simulation.rssrc/state_manager/mining.rssrc/state_manager/mod.rssrc/state_manager/state_computation.rs
Comment on lines
+171
to
+174
| let act = self | ||
| .get_actor(&Address::DATACAP_TOKEN_ACTOR, *ts.parent_state()) | ||
| .map_err(Error::state)? | ||
| .ok_or_else(|| Error::state("Miner actor not found"))?; |
Contributor
There was a problem hiding this comment.
Fix the actor name in this error path.
Line 174 reports "Miner actor not found", but this branch is loading Address::DATACAP_TOKEN_ACTOR. That makes missing-datacap failures harder to diagnose in RPC logs and responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state_manager/actor_queries.rs` around lines 171 - 174, The error message
for the branch that loads Address::DATACAP_TOKEN_ACTOR is wrong; update the
Error::state call in the get_actor result handling (where
get_actor(&Address::DATACAP_TOKEN_ACTOR, *ts.parent_state()) is used) to return
a message indicating the datacap actor is missing (e.g., "Datacap actor not
found") instead of "Miner actor not found" so logs and RPC responses correctly
reflect the failing actor.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
There was a problem hiding this comment.
I see this is from the original code, but maybe we should correct it here
Comment on lines
+208
to
+230
| for tipset in head_changes.reverts { | ||
| if candidate_tipset | ||
| .as_ref() | ||
| .is_some_and(|candidate| candidate.key() == tipset.key()) | ||
| { | ||
| candidate_tipset = None; | ||
| candidate_receipt = None; | ||
| } | ||
| } | ||
| for tipset in head_changes.applies { | ||
| if candidate_tipset | ||
| .as_ref() | ||
| .map(|s| tipset.epoch() >= s.epoch() + confidence) | ||
| .unwrap_or_default() | ||
| { | ||
| return Ok((candidate_tipset, candidate_receipt)); | ||
| } | ||
| let poll_receiver = receiver.try_recv(); | ||
| if let Ok(Some(_)) = poll_receiver { | ||
| block_revert | ||
| .write() | ||
| .await | ||
| .insert(tipset.key().to_owned(), true); |
Contributor
There was a problem hiding this comment.
Track actual reverts before accepting the back-search result.
search_back_poll later interprets this map as “the backward-search tipset was reverted”. Lines 227-230 currently insert keys from head_changes.applies, so a real revert of back_tipset is never recorded and wait_for_message can return a receipt from a non-canonical chain during a reorg race.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state_manager/message_search.rs` around lines 208 - 230, The code is
recording applied tipsets into block_revert instead of actual reverts, so
search_back_poll never marks the backward-search tipset as reverted; update the
logic so that when observing head_changes.reverts you insert the reverted tipset
key into block_revert (use tipset.key().to_owned()), and remove the insertion
from the head_changes.applies loop; adjust around
candidate_tipset/candidate_receipt checks in the applies loop so only applies
drive acceptance while reverts populate block_revert; refer to
head_changes.reverts, head_changes.applies, candidate_tipset, candidate_receipt,
block_revert, search_back_poll, and wait_for_message to locate the change.
Comment on lines
+301
to
+307
| let current_tipset = self.heaviest_tipset(); | ||
| let maybe_message_receipt = | ||
| self.tipset_executed_message(&from, &message, allow_replaced.unwrap_or(true))?; | ||
| if let Some(r) = maybe_message_receipt { | ||
| Ok(Some((from, r))) | ||
| } else { | ||
| self.search_back_for_message(current_tipset, &message, look_back_limit, allow_replaced) |
Contributor
There was a problem hiding this comment.
Continue the backward walk from from, not the current head.
After the fast-path check against from, Line 307 switches to self.heaviest_tipset(). That lets search_for_message(Some(from), ...) return a receipt that only landed after the caller’s starting tipset.
🐛 Suggested change
- let current_tipset = self.heaviest_tipset();
let maybe_message_receipt =
self.tipset_executed_message(&from, &message, allow_replaced.unwrap_or(true))?;
if let Some(r) = maybe_message_receipt {
Ok(Some((from, r)))
} else {
- self.search_back_for_message(current_tipset, &message, look_back_limit, allow_replaced)
+ self.search_back_for_message(from, &message, look_back_limit, allow_replaced)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let current_tipset = self.heaviest_tipset(); | |
| let maybe_message_receipt = | |
| self.tipset_executed_message(&from, &message, allow_replaced.unwrap_or(true))?; | |
| if let Some(r) = maybe_message_receipt { | |
| Ok(Some((from, r))) | |
| } else { | |
| self.search_back_for_message(current_tipset, &message, look_back_limit, allow_replaced) | |
| let maybe_message_receipt = | |
| self.tipset_executed_message(&from, &message, allow_replaced.unwrap_or(true))?; | |
| if let Some(r) = maybe_message_receipt { | |
| Ok(Some((from, r))) | |
| } else { | |
| self.search_back_for_message(from, &message, look_back_limit, allow_replaced) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state_manager/message_search.rs` around lines 301 - 307, The code
incorrectly continues the backward walk from self.heaviest_tipset(), allowing a
receipt that happened after the caller's starting tipset; instead resume the
search from the caller's provided starting tipset (`from`). Update the call on
the last line to continue from `from` (e.g., call
`self.search_back_for_message(from, &message, look_back_limit, allow_replaced)`
or pass `Some(from)` into `search_for_message` as appropriate) so the backward
walk does not advance to `self.heaviest_tipset()`; keep the existing fast-path
`self.tipset_executed_message(&from, &message, ...)` check intact.
hanabi1224
reviewed
Apr 22, 2026
| pub fn miner_info(&self, addr: &Address, ts: &Tipset) -> Result<MinerInfo, Error> { | ||
| let actor = self | ||
| .get_actor(addr, *ts.parent_state())? | ||
| .ok_or_else(|| Error::state("Miner actor not found"))?; |
Contributor
There was a problem hiding this comment.
We could add addr to the info to make it more useful
hanabi1224
reviewed
Apr 22, 2026
| ) -> Result<BitField, Error> { | ||
| let actor = self | ||
| .get_actor(addr, *ts.parent_state())? | ||
| .ok_or_else(|| Error::state("Miner actor not found"))?; |
Contributor
There was a problem hiding this comment.
Suggested change
| .ok_or_else(|| Error::state("Miner actor not found"))?; | |
| .ok_or_else(|| Error::state(format!("Actor {addr} not found at epoch {}", ts.epoch())))?; |
hanabi1224
reviewed
Apr 22, 2026
| pub fn miner_info(&self, addr: &Address, ts: &Tipset) -> Result<MinerInfo, Error> { | ||
| let actor = self | ||
| .get_actor(addr, *ts.parent_state())? | ||
| .ok_or_else(|| Error::state("Miner actor not found"))?; |
Contributor
There was a problem hiding this comment.
Suggested change
| .ok_or_else(|| Error::state("Miner actor not found"))?; | |
| .ok_or_else(|| Error::state(format!("Miner actor {addr} not found")))?; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
mod.rsinto saperate file so all the modules are self containedtraitsinstead of concrete typeReference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Release Notes