fix: inspect and log errors in fvm consensus trait methods#7083
Merged
Conversation
Contributor
WalkthroughConsolidates FVM-specific extern logic into a shared ChangesConsensus and Extern Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
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.
🧹 Nitpick comments (1)
src/shim/consensus.rs (1)
6-10: 💤 Low valueConsider adding common derives for debugging and flexibility.
The
ConsensusFaultTypeandConsensusFaulttypes lackDebug,Clone, andCopy(for the enum) derives which would aid debugging and allow flexibility in usage patterns.♻️ Suggested derives
-pub enum ConsensusFaultType { +#[derive(Debug, Clone, Copy)] +pub enum ConsensusFaultType { DoubleForkMining, TimeOffsetMining, ParentGrinding, }-pub struct ConsensusFault { +#[derive(Debug, Clone)] +pub struct ConsensusFault { pub target: Address, pub epoch: ChainEpoch, pub fault_type: ConsensusFaultType, }Also applies to: 42-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shim/consensus.rs` around lines 6 - 10, Add common derive traits to improve debugging and usage: annotate the enum ConsensusFaultType with #[derive(Debug, Clone, Copy, PartialEq, Eq)] (at minimum Debug, Clone, Copy) and annotate the ConsensusFault struct with #[derive(Debug, Clone, PartialEq, Eq)] (at minimum Debug and Clone) so both types can be easily logged, cloned, and compared; update the derive lists near the declarations of ConsensusFaultType and ConsensusFault accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/shim/consensus.rs`:
- Around line 6-10: Add common derive traits to improve debugging and usage:
annotate the enum ConsensusFaultType with #[derive(Debug, Clone, Copy,
PartialEq, Eq)] (at minimum Debug, Clone, Copy) and annotate the ConsensusFault
struct with #[derive(Debug, Clone, PartialEq, Eq)] (at minimum Debug and Clone)
so both types can be easily logged, cloned, and compared; update the derive
lists near the declarations of ConsensusFaultType and ConsensusFault
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 94d2d4f6-8e9e-4a02-afc1-9d48800c31a0
📒 Files selected for processing (11)
src/interpreter/externs.rssrc/interpreter/fvm2.rssrc/interpreter/fvm3.rssrc/interpreter/fvm4.rssrc/interpreter/mod.rssrc/interpreter/vm.rssrc/shim/clock.rssrc/shim/consensus.rssrc/shim/externs.rssrc/shim/gas.rssrc/shim/mod.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
hanabi1224
commented
May 20, 2026
hanabi1224
commented
May 20, 2026
hanabi1224
commented
May 20, 2026
hanabi1224
commented
May 20, 2026
hanabi1224
commented
May 20, 2026
hanabi1224
commented
May 20, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/interpreter/externs.rs (2)
123-132: 💤 Low valueComment claims errors are not propagated, but the function returns
Result.The comment on line 123 states "errors are not propagated to the caller," but this function returns
anyhow::Result, so errors are indeed propagated. The error swallowing likely happens in the FVM adapter layer (e.g.,fvm2.rs,fvm3.rs,fvm4.rs), not here. Consider clarifying the comment to indicate where errors are ultimately swallowed.📝 Suggested comment clarification
- // Inspect and log errors as this is only called in `FVM` and errors are not propogated to the caller + // Inspect and log errors; FVM adapter layer swallows these errors and returns (None, gas) to FVM🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/interpreter/externs.rs` around lines 123 - 132, The comment above the call to self.verify_consensus_fault_impl currently says "errors are not propogated to the caller" but this function returns anyhow::Result and thus propagates errors; update the comment to accurately state that verify_consensus_fault_impl (and the surrounding method) return Result and that any swallowing of errors happens upstream in the FVM adapter layers (e.g., fvm2.rs, fvm3.rs, fvm4.rs), so callers should look there if errors are being suppressed.
36-37: 💤 Low valueThe
bailfield is never set totruein this file.The
bailfield is initialized tofalseand only read viabail(), but there's no visible code path that sets it totrue. If this is intended to be set by FVM adapters (as suggested by thepub(super)visibility pattern on other fields), consider adding aset_bail()method or documenting how this flag is meant to be triggered.Also applies to: 59-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/interpreter/externs.rs` around lines 36 - 37, The AtomicBool field `bail` is never set to true but is read via the `bail()` accessor; add a way for callers (e.g., FVM adapters) to trigger it by implementing a `set_bail()` (or `trigger_bail()`) method that sets `self.bail.store(true, Ordering::SeqCst)` (or appropriate ordering), or change visibility to allow supervised callers to set it; update any places that should abort to call this new method so the flag can actually be flipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/interpreter/externs.rs`:
- Around line 123-132: The comment above the call to
self.verify_consensus_fault_impl currently says "errors are not propogated to
the caller" but this function returns anyhow::Result and thus propagates errors;
update the comment to accurately state that verify_consensus_fault_impl (and the
surrounding method) return Result and that any swallowing of errors happens
upstream in the FVM adapter layers (e.g., fvm2.rs, fvm3.rs, fvm4.rs), so callers
should look there if errors are being suppressed.
- Around line 36-37: The AtomicBool field `bail` is never set to true but is
read via the `bail()` accessor; add a way for callers (e.g., FVM adapters) to
trigger it by implementing a `set_bail()` (or `trigger_bail()`) method that sets
`self.bail.store(true, Ordering::SeqCst)` (or appropriate ordering), or change
visibility to allow supervised callers to set it; update any places that should
abort to call this new method so the flag can actually be flipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9bcb3b27-8841-4b15-82bd-0ea68dce3672
📒 Files selected for processing (1)
src/interpreter/externs.rs
LesnyRumcajs
approved these changes
May 20, 2026
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
Follow-up of #7082
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Refactor
Tests