feat(rpc): internal method Forest.BaseFeeByHeight for troubleshooting#7103
Merged
Conversation
Contributor
WalkthroughExtracts EthBaseFee::get_base_fee used by Filecoin.EthBaseFee and adds Forest.BaseFeeByHeight RPC that resolves a tipset at a given epoch and returns its upcoming base fee; registers the new RPC and updates ignored API snapshots. ChangesBase-fee RPC refactoring and new height-indexed method
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/rpc/methods/eth.rs (1)
896-900: ⚡ Quick winAdd context to tipset lookup failures.
load_required_tipset_by_heightcurrently bubbles raw errors; adding context here will make RPC troubleshooting much easier for bad/unsupported heights.Suggested patch
- let ts = ctx.chain_index().load_required_tipset_by_height( - height, - ctx.chain_store().heaviest_tipset(), - ResolveNullTipset::TakeOlder, - )?; + let ts = ctx + .chain_index() + .load_required_tipset_by_height( + height, + ctx.chain_store().heaviest_tipset(), + ResolveNullTipset::TakeOlder, + ) + .with_context(|| format!("failed to load tipset at height {height}"))?;As per coding guidelines, "Use
anyhow::Result<T>for most operations and add context with.context()when errors occur".🤖 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/rpc/methods/eth.rs` around lines 896 - 900, The call to ctx.chain_index().load_required_tipset_by_height(...) should add contextual error information instead of bubbling raw errors; update the expression that produces ts (the call to load_required_tipset_by_height on ctx.chain_index with ResolveNullTipset::TakeOlder and the height variable) to attach .context(...) (from anyhow::Context) with a clear message such as "failed to load tipset at height {height}" so RPC callers get meaningful diagnostics, and ensure the surrounding function returns anyhow::Result to propagate the contextualized error.
🤖 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/rpc/methods/eth.rs`:
- Around line 896-900: The call to
ctx.chain_index().load_required_tipset_by_height(...) should add contextual
error information instead of bubbling raw errors; update the expression that
produces ts (the call to load_required_tipset_by_height on ctx.chain_index with
ResolveNullTipset::TakeOlder and the height variable) to attach .context(...)
(from anyhow::Context) with a clear message such as "failed to load tipset at
height {height}" so RPC callers get meaningful diagnostics, and ensure the
surrounding function returns anyhow::Result to propagate the contextualized
error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5fea6f5b-181f-48bc-8a4e-4a2a8f4238d7
⛔ Files ignored due to path filters (3)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/rpc/methods/eth.rssrc/rpc/mod.rs
hanabi1224
force-pushed
the
hm/BaseFeeByHeight
branch
from
ae20044 to
ef891dc
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
896-901: ⚡ Quick winAdd context to height-based tipset resolution errors.
load_required_tipset_by_height(...)currently bubbles errors withoutheightcontext. Add.with_context(...)so troubleshooting failures are actionable.Suggested patch
- let ts = ctx.chain_index().load_required_tipset_by_height( - height, - ctx.chain_store().heaviest_tipset(), - ResolveNullTipset::TakeOlder, - )?; + let ts = ctx + .chain_index() + .load_required_tipset_by_height( + height, + ctx.chain_store().heaviest_tipset(), + ResolveNullTipset::TakeOlder, + ) + .with_context(|| format!("failed to resolve tipset for height {height}"))?;As per coding guidelines:
**/*.rs: Useanyhow::Result<T>for most operations and add context with.context()when errors occur.🤖 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/rpc/methods/eth.rs` around lines 896 - 901, The call to load_required_tipset_by_height in eth.rs (ctx.chain_index().load_required_tipset_by_height(...)) can return errors with no height context; update that invocation to add an anyhow context (using .context(...)) that includes the requested height so the error message shows which height failed, and propagate the updated Result into the subsequent EthBaseFee::get_base_fee(&ctx, &ts)? call; e.g., add .context(format!("loading tipset at height {}", height)) to the load_required_tipset_by_height call to provide actionable debug information.
🤖 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/rpc/methods/eth.rs`:
- Around line 896-901: The call to load_required_tipset_by_height in eth.rs
(ctx.chain_index().load_required_tipset_by_height(...)) can return errors with
no height context; update that invocation to add an anyhow context (using
.context(...)) that includes the requested height so the error message shows
which height failed, and propagate the updated Result into the subsequent
EthBaseFee::get_base_fee(&ctx, &ts)? call; e.g., add .context(format!("loading
tipset at height {}", height)) to the load_required_tipset_by_height call to
provide actionable debug information.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c393b0a-54b6-432b-801b-dc33ce32b170
⛔ Files ignored due to path filters (3)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/rpc/methods/eth.rssrc/rpc/mod.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
hanabi1224
force-pushed
the
hm/BaseFeeByHeight
branch
from
ef891dc to
2089aad
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
851-851: ⚡ Quick winMake helper error context method-agnostic.
Line 851 hardcodes
eth_baseFeein a shared helper, soForest.BaseFeeByHeightfailures will be mislabeled.Suggested patch
- compute_base_fee(ctx.db(), ts, smoke_height, firehorse_height) - .context("failed to compute base fee for eth_baseFee") + compute_base_fee(ctx.db(), ts, smoke_height, firehorse_height) + .context("failed to compute base fee")🤖 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/rpc/methods/eth.rs` at line 851, The helper in src/rpc/methods/eth.rs currently hardcodes the string "eth_baseFee" in its .context call, causing other callers like Forest.BaseFeeByHeight to be mislabeled; change the helper to be method-agnostic by removing the hardcoded name and either (A) accept a method_name parameter and use it in the context (e.g., format!("failed to compute base fee for {}", method_name)) or (B) use a generic context like "failed to compute base fee", then update callers (eth_baseFee and Forest.BaseFeeByHeight) to pass the appropriate label if you choose option A. Ensure the .context invocation is updated accordingly so errors reflect the correct RPC method.
🤖 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.
Inline comments:
In `@src/rpc/methods/eth.rs`:
- Around line 896-900: Forest.BaseFeeByHeight must reject requests for future
epochs instead of relying on ResolveNullTipset::TakeOlder; before calling
ctx.chain_index().load_required_tipset_by_height, obtain the current head epoch
from the chain head (via ctx.chain_store().heaviest_tipset().epoch() or
equivalent) and if the requested height is greater than that epoch return an
error (appropriate RPC/error variant) immediately; then call
load_required_tipset_by_height for valid heights (or switch ResolveNullTipset to
a non-fallback mode like Fail) so you never silently return an older tipset base
fee.
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Line 851: The helper in src/rpc/methods/eth.rs currently hardcodes the string
"eth_baseFee" in its .context call, causing other callers like
Forest.BaseFeeByHeight to be mislabeled; change the helper to be method-agnostic
by removing the hardcoded name and either (A) accept a method_name parameter and
use it in the context (e.g., format!("failed to compute base fee for {}",
method_name)) or (B) use a generic context like "failed to compute base fee",
then update callers (eth_baseFee and Forest.BaseFeeByHeight) to pass the
appropriate label if you choose option A. Ensure the .context invocation is
updated accordingly so errors reflect the correct RPC method.
🪄 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: 594a49e3-5aac-4b0b-9813-f0e515f77bf3
⛔ Files ignored due to path filters (3)
src/rpc/snapshots/forest__rpc__tests__rpc__v0.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v1.snapis excluded by!**/*.snapsrc/rpc/snapshots/forest__rpc__tests__rpc__v2.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
src/rpc/methods/eth.rssrc/rpc/mod.rssrc/tool/subcommands/api_cmd/test_snapshots_ignored.txt
✅ Files skipped from review due to trivial changes (1)
- src/tool/subcommands/api_cmd/test_snapshots_ignored.txt
LesnyRumcajs
approved these changes
May 25, 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
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