fix: avoid cloning block headers in `validate_block` #6254

hanabi1224 · 2025-11-19T17:38:57Z

Summary of changes

Changes introduced in this pull request:

avoid cloning block headers in validate_block
avoid converting Address@fvm4 in miner_power

Reference issue to close (if applicable)

Closes

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

Refactor
- Simplified address handling throughout the codebase for improved code consistency and maintainability.
- Optimized miners retrieval logic to remove unnecessary conversion steps.
- Unified address conversion mechanisms across multiple components.

coderabbitai · 2025-11-19T17:39:04Z

Walkthrough

Address handling is simplified across validation, state RPC, and power actor modules by removing intermediate conversion steps and relying on Into trait implementations instead of explicit conversion function calls.

Changes

Cohort / File(s)	Summary
Address conversion implementations `src/shim/address.rs`	Added and restructured From trait implementations for Address-to-Address_v4 conversions (owned, borrowed, and lifetime-bound reference variants) to support unified .into() usage.
Power actor address handling `src/shim/actors/builtin/power/mod.rs`	Updated ADDRESS constant type to fully qualified fvm_shared2::address::Address; refactored miner list construction and power lookups to use .into() conversions instead of explicit from_address_v\* calls across all state variants.
State manager power lookups `src/state_manager/mod.rs`	Simplified miner power and consensus minimum checks by passing Address directly to miner_power and related functions instead of &addr.into(), across is_miner_slashed, get_power, eligible_to_mine, and miner_has_min_power.
Validation logic `src/fil_cns/validation.rs`	Changed validate_block to clone entire block into v_block and access header via v_block.header.miner_address; updated validate_miner call site to pass miner_addr directly instead of &miner_addr.into().
RPC state retrieval `src/rpc/methods/state.rs`	Simplified StateListMiners miners vector construction by directly assigning state.list_all_miners(ctx.store())? instead of iterating, converting with From::from, and collecting.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Focus areas:
- Verify the Address_v4 conversion implementations in src/shim/address.rs don't introduce circular or conflicting trait implementations
- Confirm all address .into() usages in src/shim/actors/builtin/power/mod.rs correctly resolve to the intended type across different power state variants
- Validate that passing Address directly (vs. &Address) maintains proper borrowing semantics in state manager power lookups
- Check that the RPC simplification in state.rs maintains type compatibility and doesn't accidentally skip necessary conversions

Suggested reviewers

sudo-shashank
LesnyRumcajs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately reflects a primary optimization in the changeset: avoiding block header cloning in validate_block. However, it omits the significant secondary objective of avoiding Address conversions in miner_power calls across multiple files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch hm/avoid-cloning-block-headers

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

hanabi1224 · 2025-11-19T17:40:08Z

src/fil_cns/validation.rs

    let v_state_manager = state_manager.clone();
    let v_base_tipset = base_tipset.clone();
-    let v_header = header.clone();
+    let v_block = block.clone();


block is an Arc pointer while header is a large struct

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/shim/address.rs (1)

346-362: Address ↔ Address_v4 conversions are correct; reference From impl is optional

The owned and borrowed conversions to Address_v4 are correct and zero‑cost since they just expose the inner field. The impl<'a> From<&'a Address> for &'a Address_v4 is also sound, but given Address already derefs to Address_v4, that extra From impl is mostly redundant and could be dropped to keep the conversion surface simpler if you want to reduce overlap.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b197e21 and 5e6e7d8.

📒 Files selected for processing (5)

src/fil_cns/validation.rs (2 hunks)
src/rpc/methods/state.rs (1 hunks)
src/shim/actors/builtin/power/mod.rs (9 hunks)
src/shim/address.rs (1 hunks)
src/state_manager/mod.rs (4 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.

📚 Learning: 2025-09-02T10:05:34.350Z

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.

Applied to files:

src/shim/actors/builtin/power/mod.rs
src/state_manager/mod.rs
src/fil_cns/validation.rs

🧬 Code graph analysis (1)

src/shim/actors/builtin/power/mod.rs (2)

src/shim/address.rs (15)

new_id (142-144)

from (311-313)

from (317-319)

from (323-325)

from (329-331)

from (335-337)

from (341-343)

from (347-349)

from (353-355)

from (359-361)

from (365-371)

from (375-377)

from (381-387)

from (391-393)

from (397-401)

src/shim/actors/convert.rs (1)

from_policy_v13_to_v9 (156-221)

🔇 Additional comments (10)

src/rpc/methods/state.rs (1)

1836-1844: StateListMiners now directly returns miner addresses from power state

Using state.list_all_miners(ctx.store())? directly matches the Ok = Vec<Address> signature and removes an unnecessary conversion layer; behavior stays the same and is simpler.

src/fil_cns/validation.rs (2)

99-110: Miner validation now clones the block Arc instead of the header

Cloning Arc<Block> for the spawned miner validation task avoids cloning the header, keeps lifetimes simple, and is cheap; passing &v_block.header.miner_address is safe and avoids extra allocations.

223-237: validate_miner correctly uses the new miner_power address API

Using miner_addr: &Address directly in state.miner_power(state_manager.blockstore(), miner_addr) aligns with the refactored power state API and removes now‑unnecessary address conversions without changing semantics.

src/shim/actors/builtin/power/mod.rs (3)

6-18: Power actor imports and ADDRESS constant are now explicit and consistent

Pulling in shim::address::Address alongside FVM2 TokenAmount/StoragePower and defining ADDRESS as the FVM2 power actor ID clarifies the types in play and matches how other modules talk to the power actor.

97-157: list_all_miners now returns shim Addresses uniformly for all versions

For V12–V17, pushing addr.into() from the claims map keys into the Vec<Address> unifies miner address handling with earlier versions and with RPC/state callers; conversions are centralized via the Address From/Into impls, avoiding bespoke helpers.

223-261: miner_power and nominal power checks consistently take shim::Address

Switching miner_power to accept &Address and using miner.into() per actor version, plus passing &miner.into() / miner.id()? in miner_nominal_power_meets_consensus_minimum, removes scattered address-conversion code and keeps all power checks driven by the shim address type, while preserving the underlying actor API expectations.

src/state_manager/mod.rs (4)

386-395: is_miner_slashed now uses the shim Address directly for miner_power

Calling spas.miner_power(self.blockstore(), addr)?.is_none() is consistent with the new power state API and keeps the slashed check’s semantics unchanged.

420-432: get_power uses shim::Address for both miner power and consensus-minimum check

Passing maddr directly into miner_power and miner_nominal_power_meets_consensus_minimum removes redundant conversions while keeping the error paths and “has minimum power” behavior identical.

858-865: eligible_to_mine reads miner power via the updated Address-based API

Using power_state.miner_power(self.blockstore(), address)? matches the rest of the refactor and still enforces the same non-empty power-claim requirement before further eligibility checks.

1488-1492: miner_get_base_info and miner_has_min_power are aligned with the new power API

Both the get_power(&lb_state_root, Some(&addr))? call in miner_get_base_info and ps.miner_nominal_power_meets_consensus_minimum(policy, self.blockstore(), addr) in miner_has_min_power now use shim::Address consistently, simplifying address handling without changing the underlying power semantics.

Also applies to: 1515-1526

fix: avoid cloning block headers in validate_block

5e6e7d8

hanabi1224 marked this pull request as ready for review November 19, 2025 17:39

hanabi1224 requested a review from a team as a code owner November 19, 2025 17:39

hanabi1224 requested review from LesnyRumcajs and removed request for a team November 19, 2025 17:39

hanabi1224 commented Nov 19, 2025

View reviewed changes

LesnyRumcajs approved these changes Nov 19, 2025

View reviewed changes

coderabbitai bot reviewed Nov 19, 2025

View reviewed changes

hanabi1224 enabled auto-merge November 19, 2025 17:53

hanabi1224 added this pull request to the merge queue Nov 19, 2025

Merged via the queue into main with commit 06e744d Nov 19, 2025
47 of 48 checks passed

hanabi1224 deleted the hm/avoid-cloning-block-headers branch November 19, 2025 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: avoid cloning block headers in `validate_block` #6254

fix: avoid cloning block headers in `validate_block` #6254

Uh oh!

hanabi1224 commented Nov 19, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 19, 2025 •

edited

Loading

Uh oh!

hanabi1224 Nov 19, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: avoid cloning block headers in validate_block #6254

fix: avoid cloning block headers in validate_block #6254

Uh oh!

Conversation

hanabi1224 commented Nov 19, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

hanabi1224 Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: avoid cloning block headers in `validate_block` #6254

fix: avoid cloning block headers in `validate_block` #6254

hanabi1224 commented Nov 19, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 19, 2025 •

edited

Loading