Baseline bug fix (FIP-0081) #1557

kkarrancsu · 2024-06-24T15:11:06Z

No description provided.

jennijuju · 2024-06-24T15:25:53Z

note for the maintainers: this FIP has not yet been accepted, so the PR should live in an integration/feat until then

anorth

I'm not intending to be the primary reviewer here, but just skimming the architecture.

anorth · 2024-06-24T20:05:38Z

actors/power/src/lib.rs

+    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)
+    }
+


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.

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

…n a separate function

kkarrancsu · 2024-06-25T00:03:17Z

actors/power/src/state.rs

@@ -68,6 +70,8 @@ pub struct State {
    /// Number of miners having proven the minimum consensus power.
    pub miner_above_min_power_count: i64,

+    pub epochs_since_ramp_start: u64,


If I recall our conversation correctly @lemmih - we said that this would be hardcoded to a particular value for each network?

please ignore this b/c it is incorrect, at least in the way I was thinking about it, epochs_since_ramp_start needs to be able to be positive or negative, so I've updated this to be i64

kkarrancsu · 2024-06-25T00:05:01Z

actors/power/src/state.rs

@@ -68,6 +70,8 @@ pub struct State {
    /// Number of miners having proven the minimum consensus power.
    pub miner_above_min_power_count: i64,

+    pub epochs_since_ramp_start: i64,


If I recall our convo correctly @lemmih - this will be hardcoded for each network at the next upgrade?

I was probably not thinking about everything when we discussed this, but this is a value that will be updated every epoch. My thinking was that this value would be negative before the ramp was scheduled to be initiated, but update every epoch, kind of like a countdown timer. Once it is greater than 0, the code to update pledge according to the ramp will be activated. See updated line 276 of monies.rs

So I think the main thing remaining is - where does the epochs_since_ramp_start parameter get updated? It seems there needs to be some way to access the current epoch of the network as well as a hardcoded value of the epoch at which the upgrade will take place.

kkarrancsu · 2024-06-25T00:11:22Z

actors/miner/src/monies.rs

@@ -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);
+
+    // Compute gamma based on current_epoch


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?

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

actors/miner/src/ext.rs

actors/power/src/state.rs

…ration, and refactored other variables to set the correct inputs for the pledge function

actors/miner/src/lib.rs

kkarrancsu · 2024-06-25T19:01:46Z

actors/miner/src/lib.rs

@@ -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 ?


@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?

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

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

rvagg · 2024-06-26T06:50:41Z

@kkarrancsu btw running make check will get you past the linting problems that get reported in CI here before you push

kkarrancsu · 2024-06-26T17:08:58Z

@kkarrancsu btw running make check will get you past the linting problems that get reported in CI here before you push

Sounds good, I will do that before any further pushes.

kkarrancsu · 2024-07-01T13:42:01Z

Hi @jennijuju, @anorth - zooming out a little bit - we initially wanted to prototype this for the purpose of making the FIP complete to be considered by core-devs and community. Are we at that stage, or do we need to get this to the finish line, with unit tests, etc?

jennijuju · 2024-07-01T13:58:53Z

Hi @jennijuju, @anorth - zooming out a little bit - we initially wanted to prototype this for the purpose of making the FIP complete to be considered by core-devs and community. Are we at that stage, or do we need to get this to the finish line, with unit tests, etc?

I think this is good enough for actor spec. For the FIP, you also need to specify where the ramp start epoch is set and when (IIUC values will be set at upgrade migration - @lemmih could you please confirm?

anorth · 2024-07-01T20:50:17Z

I agree this is probably enough to inform a spec in the FIP now, though failing tests might suggest that something is missed.

kkarrancsu · 2024-07-02T16:05:03Z

I agree this is probably enough to inform a spec in the FIP now, though failing tests might suggest that something is missed.

The remaining error results from updating the return value to return the additional two elements needed to compute the pledge based on how far along we've progressed in the ramped activation

pub epochs_since_ramp_start: i64,
pub ramp_duration_epochs: u64,

I'm looking at the pre_commit_requires_commd_test function in integration_tests/src/tests/batch_onboarding_deals_test.rs, but it isn't immediately clear how to fix this test error. If someone can point me in the right direction here, I can resolve it.

lemmih · 2024-07-04T09:43:33Z

I agree this is probably enough to inform a spec in the FIP now, though failing tests might suggest that something is missed.

The remaining error results from updating the return value to return the additional two elements needed to compute the pledge based on how far along we've progressed in the ramped activation
pub epochs_since_ramp_start: i64,
pub ramp_duration_epochs: u64,
I'm looking at the pre_commit_requires_commd_test function in integration_tests/src/tests/batch_onboarding_deals_test.rs, but it isn't immediately clear how to fix this test error. If someone can point me in the right direction here, I can resolve it.

Update CurrentTotalPowerReturnParams in actors/market/src/ext.rs to include the new fields:

diff --git a/actors/market/src/ext.rs b/actors/market/src/ext.rs
index 987d5068..0be8f9d8 100644
--- a/actors/market/src/ext.rs
+++ b/actors/market/src/ext.rs
@@ -162,5 +162,7 @@ pub mod power {
         pub quality_adj_power: StoragePower,
         pub pledge_collateral: TokenAmount,
         pub quality_adj_power_smoothed: FilterEstimate,
+        pub ramp_start_epoch: i64,
+        pub ramp_duration_epochs: u64,
     }
 }

lemmih · 2024-09-19T14:49:32Z

@anorth @rvagg This implementation proposal is now complete and is ready to be reviewed.

anorth · 2024-09-19T20:11:03Z

actors/market/tests/harness.rs

@@ -1150,6 +1150,9 @@ pub fn expect_query_network_info(rt: &MockRuntime) {
        quality_adj_power: power.clone(),
        pledge_collateral: TokenAmount::default(),
        quality_adj_power_smoothed: FilterEstimate::new(reward.atto().clone(), BigInt::zero()),
+        // TODO: flagging that this was set to zero, but this wouldn't be the actual return from the function


Please remove this TODO. Zero is a reasonable value for networks other than mainnet.

actors/power/src/state.rs

anorth · 2024-09-19T20:30:03Z

actors/miner/tests/precommit_deposit_and_initial_pledge_positive_test.rs

+        &qa_sector_power(),
+        &StoragePower::from(1u64 << 37),
+        &reward_estimate(),
+        &power_estimate(),
+        &TokenAmount::from_whole(1),


I found these a bit hard to mentally verify. Could you please expand them, e.g. by using named vals for the parameters, and spell out in words the expected result. I suggest not using the consts defined for the "clamping" test – mixing and matching those with inline values couples the tests together and makes it hard to trace what the values are intended for. You can probably combine all these new cases into a single test for re-use.

E.g. "With a fixed reward of zero and circulating supply of 1, a sector contributing 1/2 the current network QAP requires pledge of 1/2 * 30% of circulating supply => 15% of one token."

Please add tests for:

Another point part-way through the ramp, to demonstrate it's really linear

Before the ramp start, i.e. epochs_since_ramp_start is negative

Ramp start and duration of zero (i.e. no ramp for all new networks)

Ramp duration of zero, and the epochs -1, 0, and 1 around that ramp (i.e. step change)

I only have a rudimentary understanding of the reasons behind pledges and FIP0081. My understanding is limited to this: We previously used equation A to calculate initial pledges, and now we're using 70% A + 30% B. If you want a more detailed explanation of the rules, we'll have to pull in one of the authors of FIP0081.

Another point part-way through the ramp, to demonstrate it's really linear

Done.

Before the ramp start, i.e. epochs_since_ramp_start is negative

Done.

Ramp start and duration of zero (i.e. no ramp for all new networks)

Already there.

Ramp duration of zero, and the epochs -1, 0, and 1 around that ramp (i.e. step change)

Done.

Ramp start and duration of zero (i.e. no ramp for all new networks)

The existing test is not exercising the pledge calculation properly, it's just testing a boundary condition where there is zero reward and supply. Please add a test for (ramp, duration) = (0, 0) and with reward/supply inputs similar to the other tests you added.

Ramp duration of zero, and the epochs -1, 0, and 1 around that ramp (i.e. step change)

I don't see this test added. To be concrete, I'm looking to exercise where epochs_since_ramp start differs by zero and +/- 1 from the ramp duration.

Ramp start and duration of zero (i.e. no ramp for all new networks)

The existing test is not exercising the pledge calculation properly, it's just testing a boundary condition where there is zero reward and supply. Please add a test for (ramp, duration) = (0, 0) and with reward/supply inputs similar to the other tests you added.

Done.

Ramp duration of zero, and the epochs -1, 0, and 1 around that ramp (i.e. step change)

I don't see this test added. To be concrete, I'm looking to exercise where epochs_since_ramp start differs by zero and +/- 1 from the ramp duration.

Added a test with epochs_since_ramp={-1,0,1}.

actors/miner/src/monies.rs

anorth · 2024-09-19T20:38:30Z

actors/miner/src/monies.rs

+    // gamma = 1000 - 300 * (epochs_since_ramp_start / ramp_duration_epochs).max(0).min(1)
+    let skew = ((epochs_since_ramp_start * FIP_0081_ACTIVATION_PERMILLE)
+        / ramp_duration_epochs.max(1) as i64)
+        .max(0)


The uses of max and min are clever, but maybe too much so? I'm finding it too hard to mentally verify the expected behaviour around cases like negative epochs_since_start and zero ramp duration. Tests will help with confidence here, but also an if/match construction could take out some of those edge cases clearly.

This way is definitely easier to follow thanks

actors/market/tests/harness.rs

ZenGround0 · 2024-09-22T19:23:31Z

actors/miner/src/monies.rs

@@ -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: u64 = 1000; // 3 decimal places


nit: there's lots of other fixed point calculations floating around actors code so change this to something more precise either ONE_THOUSAND or GAMMA_FIXED_POINT_FACTOR

actors/miner/src/monies.rs

anorth

Thanks for the improvements

anorth · 2024-09-23T20:18:19Z

actors/miner/tests/precommit_deposit_and_initial_pledge_positive_test.rs

+        &qa_sector_power(),
+        &StoragePower::from(1u64 << 37),
+        &reward_estimate(),
+        &power_estimate(),
+        &TokenAmount::from_whole(1),


Ramp start and duration of zero (i.e. no ramp for all new networks)

The existing test is not exercising the pledge calculation properly, it's just testing a boundary condition where there is zero reward and supply. Please add a test for (ramp, duration) = (0, 0) and with reward/supply inputs similar to the other tests you added.

Ramp duration of zero, and the epochs -1, 0, and 1 around that ramp (i.e. step change)

I don't see this test added. To be concrete, I'm looking to exercise where epochs_since_ramp start differs by zero and +/- 1 from the ramp duration.

actors/miner/tests/precommit_deposit_and_initial_pledge_positive_test.rs

ZenGround0 · 2024-09-25T16:48:52Z

actors/miner/tests/fip0081_initial_pledge.rs

+    );
+}
+
+// On-ramp where 'baseline power' (85%).


This comment is wrong, this is past ramp

ZenGround0

Thanks for getting this done it looks great.

ZenGround0 · 2024-09-25T16:51:19Z

actors/miner/tests/fip0081_initial_pledge.rs

+        initial_pledge_on_ramp
+    );
+}
+


nit: it would be nice to confirm that when we are less than 1/1000th of the way through the ramp that gamma == 1. ramp duration == 2000, epochs since ramp == 1, pledge should be 1500

I think we're also missing a case for "at ramp", e.g. 10, 10, giving us 1950, and similar to @ZenGround0's case, 999/1000, what does that give us? 1950 or one step before that?

initial updates to integrate baseline bug fix

102ee19

jennijuju requested a review from lemmih June 24, 2024 15:24

anorth reviewed Jun 24, 2024

View reviewed changes

kkarrancsu added 3 commits June 24, 2024 15:51

Merge branch 'filecoin-project:master' into fip0081

7b582ad

moving ramp duration to be returned by call to total_power rather tha…

6da9f6a

…n a separate function

updated to use correct variable name

e76fd96

kkarrancsu commented Jun 25, 2024

View reviewed changes

changing type to i64 so that the ramp can be preplanned

ad45428

kkarrancsu commented Jun 25, 2024

View reviewed changes

lemmih reviewed Jun 25, 2024

View reviewed changes

actors/miner/src/ext.rs Outdated Show resolved Hide resolved

lemmih reviewed Jun 25, 2024

View reviewed changes

actors/power/src/state.rs Outdated Show resolved Hide resolved

updated state to store epoch at which upgrade will happen, and the du…

f4f11cd

…ration, and refactored other variables to set the correct inputs for the pledge function

kkarrancsu commented Jun 25, 2024

View reviewed changes

actors/miner/src/lib.rs Outdated Show resolved Hide resolved

kkarrancsu commented Jun 25, 2024

View reviewed changes

kkarrancsu added 3 commits June 26, 2024 10:56

fixed syntax and type errors indicated by running make check

63b7ecd

Merge branch 'master' into fip0081

adf43f0

fixed make check errors

6825568

kkarrancsu added 2 commits July 2, 2024 08:05

fixing rustfmt errors

2d245ca

fixing additional rustfmt errors

60d63d3

kkarrancsu mentioned this pull request Jul 2, 2024

Fip0081 gradual activation filecoin-project/FIPs#967

Merged

fixed integration test errors

c19558e

remove unnecessary TODO items

22cc3c8

jennijuju mentioned this pull request Sep 19, 2024

implement the migration for FIP0081 filecoin-project/go-state-types#313

Closed

BigLep mentioned this pull request Sep 19, 2024

Pledge updates for FIP-0081 filecoin-project/lotus#12494

Closed

anorth changed the title ~~initial updates to integrate baseline bug fix~~ Baseline bug fix (FIP-0081) Sep 19, 2024

anorth requested a review from ZenGround0 September 19, 2024 20:09

anorth reviewed Sep 19, 2024

View reviewed changes

lemmih added 5 commits September 20, 2024 10:11

remote yet another unnecessary TODO item

330ef9d

document the two new fields in power state

06ff09c

test pre-ramp, test early on-ramp

bceeb2c

check first step on ramp

417f9e1

use if-then-else rather than min/max

37c442d

ZenGround0 reviewed Sep 22, 2024

View reviewed changes

anorth reviewed Sep 23, 2024

View reviewed changes

lemmih added 4 commits September 24, 2024 14:40

avoid a division when merging baseline and simple pledges

252a358

validate pledges 1 epoch around FIP0081 activation

8c969b4

rename gamme constant, reword comments

e1836c0

test more FIP0081 cases

7f09f47

ZenGround0 reviewed Sep 24, 2024

View reviewed changes

actors/miner/tests/precommit_deposit_and_initial_pledge_positive_test.rs Show resolved Hide resolved

lemmih added 2 commits September 25, 2024 13:38

move tests to separate file

1611afe

Merge branch 'master' into fip0081

5c361a5

ZenGround0 reviewed Sep 25, 2024

View reviewed changes

ZenGround0 approved these changes Sep 25, 2024

View reviewed changes

anorth approved these changes Sep 25, 2024

View reviewed changes

correct test comments

c5b2658

This was referenced Sep 27, 2024

chore(power): add CurrentTotalPowerReturn serialization tests #1580

Merged

feat(fip0081): initialPledge calculation & ramp, plus migration tests filecoin-project/lotus#12526

Merged

ZenGround0 added this pull request to the merge queue Sep 30, 2024

Merged via the queue into filecoin-project:master with commit 01a05fc Sep 30, 2024
10 checks passed

Baseline bug fix (FIP-0081) #1557

Baseline bug fix (FIP-0081) #1557

Conversation

kkarrancsu commented Jun 24, 2024

jennijuju commented Jun 24, 2024

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkarrancsu Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Jun 26, 2024

kkarrancsu commented Jun 26, 2024

kkarrancsu commented Jul 1, 2024

jennijuju commented Jul 1, 2024

anorth commented Jul 1, 2024

kkarrancsu commented Jul 2, 2024

lemmih commented Jul 4, 2024

lemmih commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkarrancsu Jun 25, 2024 •

edited

Loading