Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Baseline bug fix (FIP-0081) #1557

Merged
merged 27 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
102ee19
initial updates to integrate baseline bug fix
kkarrancsu Jun 21, 2024
7b582ad
Merge branch 'filecoin-project:master' into fip0081
kkarrancsu Jun 24, 2024
6da9f6a
moving ramp duration to be returned by call to total_power rather tha…
kkarrancsu Jun 24, 2024
e76fd96
updated to use correct variable name
kkarrancsu Jun 25, 2024
ad45428
changing type to i64 so that the ramp can be preplanned
kkarrancsu Jun 25, 2024
f4f11cd
updated state to store epoch at which upgrade will happen, and the du…
kkarrancsu Jun 25, 2024
63b7ecd
fixed syntax and type errors indicated by running make check
kkarrancsu Jun 26, 2024
adf43f0
Merge branch 'master' into fip0081
kkarrancsu Jun 26, 2024
6825568
fixed make check errors
kkarrancsu Jun 26, 2024
2d245ca
fixing rustfmt errors
kkarrancsu Jul 2, 2024
60d63d3
fixing additional rustfmt errors
kkarrancsu Jul 2, 2024
c19558e
fixed integration test errors
kkarrancsu Jul 5, 2024
1bd140e
Merge branch 'master' into fip0081
lemmih Sep 9, 2024
a140b68
write three tests, fix bug in pledge calculation
lemmih Sep 9, 2024
22cc3c8
remove unnecessary TODO items
lemmih Sep 10, 2024
330ef9d
remote yet another unnecessary TODO item
lemmih Sep 20, 2024
06ff09c
document the two new fields in power state
lemmih Sep 20, 2024
bceeb2c
test pre-ramp, test early on-ramp
lemmih Sep 20, 2024
417f9e1
check first step on ramp
lemmih Sep 20, 2024
37c442d
use if-then-else rather than min/max
lemmih Sep 20, 2024
252a358
avoid a division when merging baseline and simple pledges
lemmih Sep 24, 2024
8c969b4
validate pledges 1 epoch around FIP0081 activation
lemmih Sep 24, 2024
e1836c0
rename gamme constant, reword comments
lemmih Sep 24, 2024
7f09f47
test more FIP0081 cases
lemmih Sep 24, 2024
1611afe
move tests to separate file
lemmih Sep 25, 2024
5c361a5
Merge branch 'master' into fip0081
lemmih Sep 25, 2024
c5b2658
correct test comments
lemmih Sep 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions actors/miner/src/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ pub mod power {
pub quality_adj_power: StoragePower,
pub pledge_collateral: TokenAmount,
pub quality_adj_power_smoothed: FilterEstimate,
pub epochs_since_ramp_start: i64,
pub ramp_duration_epochs: u64,
}
#[derive(Serialize_tuple, Deserialize_tuple)]
pub struct EnrollCronEventParams {
Expand Down
13 changes: 13 additions & 0 deletions actors/miner/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,8 @@ impl Actor {
network_baseline: rew.this_epoch_baseline_power,
circulating_supply,
epoch_reward: rew.this_epoch_reward_smoothed,
epochs_since_ramp_start: pwr.ramp_start_epoch - rt.curr_epoch(),
lemmih marked this conversation as resolved.
Show resolved Hide resolved
ramp_duration_epochs: pwr.ramp_duration_epochs,
};

/*
Expand Down Expand Up @@ -1913,6 +1915,8 @@ impl Actor {
network_baseline: rew.this_epoch_baseline_power,
circulating_supply,
epoch_reward: rew.this_epoch_reward_smoothed,
epochs_since_ramp_start: pwr.ramp_start_epoch - rt.curr_epoch(),
ramp_duration_epochs: pwr.ramp_duration_epochs,
};
activate_new_sector_infos(
rt,
Expand Down Expand Up @@ -1981,11 +1985,14 @@ impl Actor {
activate_sectors_deals(rt, &data_activations, compute_commd)?;
let successful_activations = batch_return.successes(&precommited_sectors);

// TODO: total_power is not requested here, so how can we get ramp_start_epoch & ramp_duration_epochs ?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lemmih - flag here, do we need to make a call to get_total_power(), or is this function a special startup state where the inputs are noops?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You ran into some technical debt here. This whole method is legacy and never invoked on chain. It should be removed. It's used only for creating genesis state (e.g. for testnets). Just set both values to zero.

cc @ZenGround0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slated for removal #1556 so don't spend too much time on this. It's only ever called at genesis in trusted testing environments.

let pledge_inputs = NetworkPledgeInputs {
network_qap: params.quality_adj_power_smoothed,
network_baseline: params.reward_baseline_power,
circulating_supply: rt.total_fil_circ_supply(),
epoch_reward: params.reward_smoothed,
// TODO: set ramp_start_epoch
// TODO: set ramp_duration_epochs
};
activate_new_sector_infos(
rt,
Expand Down Expand Up @@ -3902,6 +3909,8 @@ where
network_baseline: rew.this_epoch_baseline_power,
circulating_supply,
epoch_reward: rew.this_epoch_reward_smoothed,
epochs_since_ramp_start: pwr.ramp_start_epoch - rt.curr_epoch(),
ramp_duration_epochs: pwr.ramp_duration_epochs,
};
let mut power_delta = PowerPair::zero();
let mut pledge_delta = TokenAmount::zero();
Expand Down Expand Up @@ -4098,6 +4107,7 @@ fn update_existing_sector_info(
&pledge_inputs.epoch_reward,
&pledge_inputs.network_qap,
&pledge_inputs.circulating_supply,
&pledge_inputs.ramp_duration_epochs,
),
);
new_sector_info
Expand Down Expand Up @@ -5219,6 +5229,7 @@ fn activate_new_sector_infos(
&pledge_inputs.epoch_reward,
&pledge_inputs.network_qap,
&pledge_inputs.circulating_supply,
&pledge_inputs.ramp_duration_epochs,
);

deposit_to_unlock += pci.pre_commit_deposit.clone();
Expand Down Expand Up @@ -5631,6 +5642,8 @@ struct NetworkPledgeInputs {
pub network_baseline: StoragePower,
pub circulating_supply: TokenAmount,
pub epoch_reward: FilterEstimate,
pub epochs_since_ramp_start: i64,
pub ramp_duration_epochs: u64,
}

// Note: probably better to push this one level down into state
Expand Down
30 changes: 27 additions & 3 deletions actors/miner/src/monies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ pub const TERMINATION_LIFETIME_CAP: ChainEpoch = 140;
// Multiplier of whole per-winner rewards for a consensus fault penalty.
const CONSENSUS_FAULT_FACTOR: u64 = 5;

const FIXED_POINT_FACTOR: i64 = 1000; // 3 decimal places

/// The projected block reward a sector would earn over some period.
/// Also known as "BR(t)".
/// BR(t) = ProjectedRewardFraction(t) * SectorQualityAdjustedPower
Expand Down Expand Up @@ -255,6 +257,8 @@ pub fn initial_pledge_for_power(
reward_estimate: &FilterEstimate,
network_qa_power_estimate: &FilterEstimate,
circulating_supply: &TokenAmount,
epochs_since_ramp_start: i64,
ramp_duration_epochs: u64
) -> TokenAmount {
let ip_base = expected_reward_for_power_clamped_at_atto_fil(
reward_estimate,
Expand All @@ -267,10 +271,30 @@ pub fn initial_pledge_for_power(
let lock_target_denom = LOCK_TARGET_FACTOR_DENOM;
let pledge_share_num = qa_power;
let network_qa_power = network_qa_power_estimate.estimate();
let pledge_share_denom = cmp::max(cmp::max(&network_qa_power, baseline_power), qa_power);

anorth marked this conversation as resolved.
Show resolved Hide resolved
// Compute gamma based on current_epoch
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where can we write a unittest for the updated initial_pledge_for_power so that we can be more certain that the computation is what we expect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just add a new file in actors/miner/tests/, you'll see it already being tested for a particular case in actors/miner/tests/precommit_deposit_and_initial_pledge_positive_test.rs so you can copy that pattern

let mut gamma = FIXED_POINT_FACTOR; // Initialize as 1000
if epochs_since_ramp_start > 0 {
if epochs_since_ramp_start as u64 < ramp_duration_epochs {
let reduction = (300 * FIXED_POINT_FACTOR) / ramp_duration_epochs; // 0.3 as fixed-point
gamma -= reduction;
} else {
gamma = 700; // 0.7 as fixed-point
}
}

let additional_ip_num = lock_target_num * pledge_share_num;
let additional_ip_denom = pledge_share_denom * lock_target_denom;
let additional_ip = additional_ip_num.div_floor(&additional_ip_denom);

let pledge_share_denom_baseline = cmp::max(cmp::max(&network_qa_power, baseline_power), qa_power);
let pledge_share_denom_simple = cmp::max(network_qa_power, qa_power);

let additional_ip_denom_baseline = pledge_share_denom_baseline * lock_target_denom;
let additional_ip_baseline = additional_ip_num.div_floor(&additional_ip_denom_baseline);
let additional_ip_denom_simple = pledge_share_denom_simple * lock_target_denom;
let additional_ip_simple = additional_ip_num.div_floor(&additional_ip_denom_simple);

// convex combination of simple and baseline pledge
let additional_ip = (additional_ip_baseline * gamma + additional_ip_simple * (FIXED_POINT_FACTOR - gamma)) / FIXED_POINT_FACTOR;

let nominal_pledge = ip_base + TokenAmount::from_atto(additional_ip);
let pledge_cap = TokenAmount::from_atto(INITIAL_PLEDGE_MAX_PER_BYTE.atto() * qa_power);
Expand Down
1 change: 1 addition & 0 deletions actors/power/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ impl Actor {
quality_adj_power: st.this_epoch_quality_adj_power,
pledge_collateral: st.this_epoch_pledge_collateral,
quality_adj_power_smoothed: st.this_epoch_qa_power_smoothed,
epochs_since_ramp_start: st.epochs_since_ramp_start,
})
}

Expand Down
3 changes: 3 additions & 0 deletions actors/power/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ pub struct State {
/// Number of miners having proven the minimum consensus power.
pub miner_above_min_power_count: i64,

pub ramp_start_epoch: i64,
pub ramp_duration_epochs: u64,
anorth marked this conversation as resolved.
Show resolved Hide resolved

/// A queue of events to be triggered by cron, indexed by epoch.
pub cron_event_queue: Cid, // Multimap, (HAMT[ChainEpoch]AMT[CronEvent]

Expand Down
Loading