Open
Conversation
Contributor
WalkthroughImplements FIP-0115: base fee computation now branches at a specified upgrade epoch to compute next base fee from sender premiums (using a weighted quick-select) instead of utilization; adds premium-based helpers, a weighted_quick_select module, constants, and updates call sites and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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
🤖 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/chain/store/base_fee.rs`:
- Around line 13-17: The placeholder constant PLACEHOLDER_NEXT_UPGRADE_HEIGHT
(in src/chain/store/base_fee.rs) is disabling the FIP-0115 branch; replace this
compile-time MAX sentinel with a real activation epoch sourced from the network
configuration (e.g., add a field to your network/chain config struct or
ChainConfig and use that value instead of PLACEHOLDER_NEXT_UPGRADE_HEIGHT),
update all callers that currently import PLACEHOLDER_NEXT_UPGRADE_HEIGHT to read
the configured activation epoch (or accept it via function parameters) so the
premium-based branch can activate at runtime, and adjust any tests/fixtures to
inject the desired activation epoch for deterministic behavior.
In `@src/chain/store/weighted_quick_select.rs`:
- Around line 347-363: The property test prop_result_respects_weight_ordering is
vacuous because it sets target = pair.weight_high so the conditional `if target
< pair.weight_high` never runs; change how target is chosen (in the prop or
before calling weighted_quick_select) to produce a value strictly less than
pair.weight_high (for example pick target = pair.weight_low or generate target =
pair.weight_high - 1 ensuring non-negative), then call weighted_quick_select
with that target and assert the result equals
TokenAmount::from_atto(pair.high_premium) when target < pair.weight_high; adjust
logic around OrderedPremiumPair and TokenAmount conversions to match the chosen
target generation.
- Around line 30-31: Replace the panic-prone unwrap() call on
premiums.into_iter().next() with expect(...) that provides context; the
surrounding check uses limits.first().is_some_and(|&limit| limit > target_index)
so change premiums.into_iter().next().unwrap() to
premiums.into_iter().next().expect("expected at least one premium when
limits.first() indicates a valid target index") to document why next() is safe
and give a helpful error if it ever fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c5ed8d65-73fa-4ce4-aebd-af197db579f3
📒 Files selected for processing (7)
src/chain/store/base_fee.rssrc/chain/store/mod.rssrc/chain/store/weighted_quick_select.rssrc/chain_sync/tipset_syncer.rssrc/message/mod.rssrc/message_pool/msgpool/provider.rssrc/rpc/methods/miner.rs
4 tasks
LesnyRumcajs
force-pushed
the
fip-0115-basefee
branch
from
739f727 to
3d8575c
Compare
LesnyRumcajs
commented
Mar 9, 2026
|
|
||
| /// Placeholder for the FIP-0115 activation height. | ||
| /// Replace with the actual network upgrade height once finalized. | ||
| pub const PLACEHOLDER_NEXT_UPGRADE_HEIGHT: ChainEpoch = ChainEpoch::MAX; |
LesnyRumcajs
requested review from
akaladarshi and
hanabi1224
and removed request for
a team
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/chain/store/weighted_quick_select.rs (1)
21-25: Assert thepremiums/limits1:1 contract.
zipwill silently drop trailing entries if those vectors ever diverge, which would skew the selected premium without failing fast. A small guard here would make the invariant explicit in this consensus path.♻️ Suggested guard
pub fn weighted_quick_select( mut premiums: Vec<TokenAmount>, mut limits: Vec<u64>, mut target_index: u64, ) -> TokenAmount { + debug_assert_eq!( + premiums.len(), + limits.len(), + "premiums and limits must stay 1:1", + ); loop {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/weighted_quick_select.rs` around lines 21 - 25, Ensure the 1:1 invariant between premiums and limits in weighted_quick_select by adding an explicit guard that checks premiums.len() == limits.len() (and fails with a clear message if not) at the start of the function so zip cannot silently drop entries; refer to the function weighted_quick_select and the parameters premiums and limits when adding this assertion.
🤖 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/chain/store/base_fee.rs`:
- Around line 58-72: The activation check in compute_base_fee is off-by-one:
since compute_base_fee receives the parent tipset (ts), change the condition
that selects compute_next_base_fee_from_premiums so it compares the child epoch
to next_upgrade_height by using ts.epoch() + 1 instead of ts.epoch(); update the
if in compute_base_fee to use ts.epoch() + 1 >= next_upgrade_height so the
premium rule calls compute_next_base_fee_from_premiums at the correct epoch,
otherwise continue to compute_next_base_fee_from_utlilization.
---
Nitpick comments:
In `@src/chain/store/weighted_quick_select.rs`:
- Around line 21-25: Ensure the 1:1 invariant between premiums and limits in
weighted_quick_select by adding an explicit guard that checks premiums.len() ==
limits.len() (and fails with a clear message if not) at the start of the
function so zip cannot silently drop entries; refer to the function
weighted_quick_select and the parameters premiums and limits when adding this
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0675fdf2-09e1-46da-ad53-c63fe9e97969
📒 Files selected for processing (8)
CHANGELOG.mdsrc/chain/store/base_fee.rssrc/chain/store/mod.rssrc/chain/store/weighted_quick_select.rssrc/chain_sync/tipset_syncer.rssrc/message/mod.rssrc/message_pool/msgpool/provider.rssrc/rpc/methods/miner.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/rpc/methods/miner.rs
- src/message/mod.rs
- src/message_pool/msgpool/provider.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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
1 participant
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 #6686
Other information and links
Don't merge before the reference is merged to Lotus master. There might be some additional changes there.
Change checklist
Outside contributions
Summary by CodeRabbit