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 1 commit
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
6 changes: 6 additions & 0 deletions actors/miner/src/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ pub mod power {
pub const UPDATE_PLEDGE_TOTAL_METHOD: u64 = 6;
pub const SUBMIT_POREP_FOR_BULK_VERIFY_METHOD: u64 = 8;
pub const CURRENT_TOTAL_POWER_METHOD: u64 = 9;
pub const MINER_RAMP_PARAMS_METHOD: u64 = 10; // TODO: how to set this value properly??

#[derive(Serialize_tuple, Deserialize_tuple)]
pub struct CurrentTotalPowerReturn {
Expand All @@ -116,6 +117,11 @@ pub mod power {
pub quality_adjusted_delta: StoragePower,
}

#[derive(Serialize_tuple, Deserialize_tuple)]
pub struct MinerRampParamsReturn {
pub ramp_duration_epochs: u64
}

pub const MAX_MINER_PROVE_COMMITS_PER_EPOCH: usize = 200;
}

Expand Down
24 changes: 24 additions & 0 deletions actors/miner/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -811,11 +811,13 @@ impl Actor {
let rew = request_current_epoch_block_reward(rt)?;
let pwr = request_current_total_power(rt)?;
let circulating_supply = rt.total_fil_circ_supply();
let ramp_duration_epochs = request_miner_ramp_params(rt)?.ramp_duration_epochs;
let pledge_inputs = NetworkPledgeInputs {
network_qap: pwr.quality_adj_power_smoothed,
network_baseline: rew.this_epoch_baseline_power,
circulating_supply,
epoch_reward: rew.this_epoch_reward_smoothed,
ramp_duration_epochs: ramp_duration_epochs,
};

/*
Expand Down Expand Up @@ -1908,11 +1910,13 @@ impl Actor {
let rew = request_current_epoch_block_reward(rt)?;
let pwr = request_current_total_power(rt)?;
let circulating_supply = rt.total_fil_circ_supply();
let ramp_duration_epochs = request_miner_ramp_params(rt)?.ramp_duration_epochs;
let pledge_inputs = NetworkPledgeInputs {
network_qap: pwr.quality_adj_power_smoothed,
network_baseline: rew.this_epoch_baseline_power,
circulating_supply,
epoch_reward: rew.this_epoch_reward_smoothed,
ramp_duration_epochs: ramp_duration_epochs
};
activate_new_sector_infos(
rt,
Expand Down Expand Up @@ -3844,11 +3848,13 @@ where
let rew = request_current_epoch_block_reward(rt)?;
let pow = request_current_total_power(rt)?;
let circulating_supply = rt.total_fil_circ_supply();
let ramp_duration_epochs = request_miner_ramp_params(rt)?.ramp_duration_epochs;
let pledge_inputs = NetworkPledgeInputs {
network_qap: pow.quality_adj_power_smoothed,
network_baseline: rew.this_epoch_baseline_power,
circulating_supply,
epoch_reward: rew.this_epoch_reward_smoothed,
ramp_duration_epochs: ramp_duration_epochs
};
let mut power_delta = PowerPair::zero();
let mut pledge_delta = TokenAmount::zero();
Expand Down Expand Up @@ -4045,6 +4051,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 @@ -4803,6 +4810,21 @@ fn request_current_total_power(
)
}

/// Request the ramp duration from the power actor
fn request_miner_ramp_params(
rt: &impl Runtime,
) -> Result<ext::power::MinerRampParamsReturn, ActorError> {
deserialize_block(
extract_send_result(rt.send_simple(
&STORAGE_POWER_ACTOR_ADDR,
ext::power::MINER_RAMP_PARAMS_METHOD,
Default::default(),
TokenAmount::zero(),
))
.map_err(|e| e.wrap("failed to check miner ramp params"))?,
)
}

/// Resolves an address to an ID address and verifies that it is address of an account actor with an associated BLS key.
/// The worker must be BLS since the worker key will be used alongside a BLS-VRF.
fn resolve_worker_address(rt: &impl Runtime, raw: Address) -> Result<ActorID, ActorError> {
Expand Down Expand Up @@ -5166,6 +5188,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 @@ -5578,6 +5601,7 @@ struct NetworkPledgeInputs {
pub network_baseline: StoragePower,
pub circulating_supply: TokenAmount,
pub epoch_reward: FilterEstimate,
pub ramp_duration_epochs: u64
}

// Note: probably better to push this one level down into state
Expand Down
29 changes: 26 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,7 @@ pub fn initial_pledge_for_power(
reward_estimate: &FilterEstimate,
network_qa_power_estimate: &FilterEstimate,
circulating_supply: &TokenAmount,
epochs_since_ramp_start: i64,
) -> TokenAmount {
let ip_base = expected_reward_for_power_clamped_at_atto_fil(
reward_estimate,
Expand All @@ -267,10 +270,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
8 changes: 8 additions & 0 deletions actors/power/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,13 @@ impl Actor {
Ok(MinerConsensusCountReturn { miner_consensus_count: st.miner_above_min_power_count })
}

fn ramp_duration(rt: &impl Runtime) -> Result<u64, ActorError> {
rt.validate_immediate_caller_accept_any()?;
let st: State = rt.state()?;

Ok(st.ramp_duration_epochs)
}

Copy link
Member

Choose a reason for hiding this comment

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

I think the ramp configuration should be rolled into the return value from the existing request for total power. All the call sites above that called this function also invoked the power actor for the total power just a few lines earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - that makes sense and reduces the number of overall changes, I've updated accordingly.

fn process_deferred_cron_events(
rt: &impl Runtime,
rewret: ThisEpochRewardReturn,
Expand Down Expand Up @@ -438,5 +445,6 @@ impl ActorCode for Actor {
MinerRawPowerExported => miner_raw_power,
MinerCountExported => miner_count,
MinerConsensusCountExported => miner_consensus_count,
RampDurationExported => ramp_duration,
}
}
4 changes: 4 additions & 0 deletions actors/power/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ pub const CRON_QUEUE_AMT_BITWIDTH: u32 = 6;
pub type ClaimsMap<BS> = Map2<BS, Address, Claim>;
pub const CLAIMS_CONFIG: Config = DEFAULT_HAMT_CONFIG;

const RAMP_DURATION_EPOCHS: u64 = 365 * 2880; // 1Y
lemmih marked this conversation as resolved.
Show resolved Hide resolved

/// Storage power actor state
#[derive(Default, Serialize_tuple, Deserialize_tuple, Clone, Debug)]
pub struct State {
Expand All @@ -68,6 +70,8 @@ pub struct State {
/// Number of miners having proven the minimum consensus power.
pub miner_above_min_power_count: 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