Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughImplements FIP-0115: adds premium-weighted base-fee computation with a weighted quick-select module, new helper paths/constants, message deduplication by (from, sequence), tests, and updates call sites to pass a next-upgrade activation height. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
📝 Coding Plan
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
LesnyRumcajs
force-pushed
the
fip-0115-basefee
branch
from
739f727 to
3d8575c
Compare
src/chain/store/base_fee.rs
Outdated
|
|
||
| /// 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
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 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
|
||
| #[test] | ||
| fn test_weighted_quick_select_deterministic() { | ||
| // Run multiple times to verify correctness despite randomness |
There was a problem hiding this comment.
If you want deterministic testing of the randomized algorithm, you can mock the randomness (forest_rng). and iterate through all possible responses like a depth-first-search, verifying correctness for all of them. As-is, if it failed in exactly one rare case, it might be difficult to reproduce and also flaky test failures tend to be ignored.
I implemented such a mock in this commit
LesnyRumcajs
force-pushed
the
fip-0115-basefee
branch
from
34fc466 to
a1b8946
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/chain/store/base_fee.rs (1)
118-118: Typo in function name: "utlilization" → "utilization".Consider fixing this typo for consistency and clarity. Since this is a private function, it won't cause breaking changes.
Suggested fix
-fn compute_next_base_fee_from_utlilization<DB>( +fn compute_next_base_fee_from_utilization<DB>(Also update the call site at line 72:
- compute_next_base_fee_from_utlilization(db, ts, smoke_height) + compute_next_base_fee_from_utilization(db, ts, smoke_height)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/base_fee.rs` at line 118, Rename the private function compute_next_base_fee_from_utlilization to compute_next_base_fee_from_utilization and update every call site that references compute_next_base_fee_from_utlilization (e.g., the place that currently invokes it from the base fee calculation code) to use the corrected name; ensure any unit tests or local references are updated as well to keep consistency since this is an internal/private function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/chain/store/base_fee.rs`:
- Line 118: Rename the private function compute_next_base_fee_from_utlilization
to compute_next_base_fee_from_utilization and update every call site that
references compute_next_base_fee_from_utlilization (e.g., the place that
currently invokes it from the base fee calculation code) to use the corrected
name; ensure any unit tests or local references are updated as well to keep
consistency since this is an internal/private function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 988112e5-5000-40df-859e-b27d947e45ea
📒 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/message/mod.rs
- src/message_pool/msgpool/provider.rs
- CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/chain/store/base_fee.rs (1)
83-94: Pin replacement-message tie-breaking with a regression test.This loop keeps the first
(from, sequence)seen in the tipset. If two blocks carry different replacements for the same sender/nonce, the derived base fee changes with iteration order, so a small fixture for that case would help protect this consensus path from future ordering refactors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chain/store/base_fee.rs` around lines 83 - 94, Add a regression test that pins the tie-breaking behavior of the replacement-message loop in base fee derivation: create a tipset with two blocks that both include different messages from the same sender and same sequence, then verify that computing the base fee (via the code path that calls crate::chain::block_messages and iterates over bls_msgs/secp_msgs, using seen.insert((m.from(), m.sequence()))) yields the same base fee regardless of block iteration order; ensure the test exercises effective_gas_premium and gas_limit accumulation and fails if a later-seen replacement overrides the first-seen message.
🤖 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 281-298: The property test prop_next_base_fee_change_bounded
allows negative premiums which the real code never uses; constrain or clamp the
test premium to non-negative before calling compute_next_base_fee_from_premium.
Specifically, in prop_next_base_fee_change_bounded (which calls
compute_next_base_fee_from_premium) either skip cases where premium is negative
or replace premium with its non-negative equivalent (e.g., premium =
premium.max(&TokenAmount::from_atto(0)) or check premium.is_negative() and
return true) so the test mirrors production's effective_gas_premium clamping.
---
Nitpick comments:
In `@src/chain/store/base_fee.rs`:
- Around line 83-94: Add a regression test that pins the tie-breaking behavior
of the replacement-message loop in base fee derivation: create a tipset with two
blocks that both include different messages from the same sender and same
sequence, then verify that computing the base fee (via the code path that calls
crate::chain::block_messages and iterates over bls_msgs/secp_msgs, using
seen.insert((m.from(), m.sequence()))) yields the same base fee regardless of
block iteration order; ensure the test exercises effective_gas_premium and
gas_limit accumulation and fails if a later-seen replacement overrides the
first-seen message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa13c311-d55c-4552-b652-6f18f38d8f7c
📒 Files selected for processing (5)
src/chain/store/base_fee.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 (2)
- src/chain_sync/tipset_syncer.rs
- src/rpc/methods/miner.rs
|
Upstream merged |
@wjmelements Thanks for reviewing, much appreciated! |
3 participants
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
New Features
Behavioral Change
Tests / Changelog