feat: add miner deposit #1398

hunjixin · 2023-09-01T02:56:03Z

fip filecoin-project/FIPs#796
disccussion filecoin-project/FIPs#780

codecov-commenter · 2023-09-01T03:22:28Z

Codecov Report

Merging #1398 (b5cc6ad) into master (8076305) will decrease coverage by 0.08%.
The diff coverage is 82.75%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1398      +/-   ##
==========================================
- Coverage   91.06%   90.99%   -0.08%     
==========================================
  Files         145      145              
  Lines       27397    27420      +23     
==========================================
+ Hits        24950    24951       +1     
- Misses       2447     2469      +22

Files Changed	Coverage Δ
actors/miner/src/lib.rs	`84.69% <78.26%> (-0.05%)`	⬇️
actors/miner/src/monies.rs	`99.43% <100.00%> (+<0.01%)`	⬆️
actors/miner/src/state.rs	`80.97% <100.00%> (-0.05%)`	⬇️
actors/power/src/lib.rs	`84.04% <100.00%> (-0.72%)`	⬇️
runtime/src/runtime/policy.rs	`98.38% <100.00%> (+0.01%)`	⬆️

... and 6 files with indirect coverage changes

anorth

As discussed in the FIP, I think the deposit amount must be a function, not a constant. This will increase the impact this change has on test code, so I suggest we resolve things at the specification level before applying that impact to the code here.

anorth · 2023-09-18T00:07:37Z

actors/miner/src/lib.rs

@@ -188,6 +188,10 @@ impl Actor {
            .collect::<Result<_, _>>()?;

        let policy = rt.policy();
+        if rt.current_balance() < policy.new_miner_deposit {
+            return Err(actor_error!(illegal_argument, "not enough miner deposit"));


Insufficient funds is probably a better error code

yes, but this may exit two case:
case1: value in message is not enough for new miner deposit
case2: the from address dont have enough funds to deposit this error return before exec create miner.

so i think argument error should be resonable

anorth · 2023-09-18T00:08:31Z

actors/miner/src/lib.rs

+            .map_err(|e| {
+                actor_error!(
+                    illegal_state,
+                    "failed to lock new miner deposit in vesting table: {}",
+                    e
+                )
+            })?;


Please use the newer .with_context_code form here.

anorth · 2023-09-18T00:09:23Z

actors/miner/src/monies.rs

@@ -288,6 +288,11 @@ pub fn locked_reward_from_reward(reward: TokenAmount) -> (TokenAmount, &'static
    (lock_amount, &REWARD_VESTING_SPEC)
 }

+/// Lock amount of deposit of creating new miner
+pub fn locked_new_miner_deposit(deposit: TokenAmount) -> (TokenAmount, &'static VestSpec) {
+    (deposit, &REWARD_VESTING_SPEC)


Why is the deposit a parameter here if it's just passed straight back out?

just copy a copy code from locked_reward_from_reward. In fact, I do tend to use a certain ratio of sector collateral as new miners deposit.

anorth · 2023-09-18T00:10:15Z

actors/miner/src/state.rs

-        !self.pre_commit_deposits.is_zero()
-            || !self.initial_pledge.is_zero()
-            || !self.locked_funds.is_zero()
+        !self.pre_commit_deposits.is_zero() || !self.initial_pledge.is_zero()


I don't think this change is reasonable. A miner needs to keep vesting if they have locked funds - we can't tell how they reached this state.

you are right removing this will cause problems with the circulation calculations.

i have a progessing version without this code, but there are too much test case to fix. and had time to handle this since I quit my job recently.

hmm, check_state_invariants behavior need change.

builtin-actors/actors/miner/src/testing.rs

Line 306 in b81da94

if state.continue_deadline_cron() {

anorth · 2023-09-18T00:11:41Z

actors/miner/tests/apply_rewards.rs

    assert_eq!(expected, h.get_locked_funds(&rt));
 }

 #[test]
 fn funds_vest() {
    let h = ActorHarness::new(PERIOD_OFFSET);
-    let rt = h.new_runtime();
+    let mut rt = h.new_runtime();
+    rt.policy.new_miner_deposit = TokenAmount::zero();


I think it's a problem that this test is touched at all, and that an unrealistic value is used for the deposit. Can you explain why (in code comments) ?

just too much test to fix, this is a quick fix

anorth · 2023-09-18T00:12:12Z

actors/miner/tests/batch_method_network_fees_test.rs

@@ -175,7 +175,8 @@ fn insufficient_funds_for_batch_precommit_in_combination_of_fee_debt_and_network
 #[test]
 fn enough_funds_for_fee_debt_and_network_fee_but_not_for_pcd() {
    let actor = ActorHarness::new(*PERIOD_OFFSET);
-    let rt = actor.new_runtime();
+    let mut rt = actor.new_runtime();
+    rt.policy.new_miner_deposit = TokenAmount::default();


Why setting this unrealistic amount? Tests need to be updated to use a real deposit.

Also default is a rather opaque way of saying zero. Please use zero() everywhere so it's more obvious that this is not a default value, but a fake one.

anorth · 2023-09-18T00:16:55Z

actors/power/src/lib.rs

@@ -129,7 +129,7 @@ impl Actor {
                },
            )?;
            st.miner_count += 1;
-
+            st.add_pledge_total(rt.policy().new_miner_deposit.clone());


This code is a long way from the other code that it has to stay in sync with. This has a high chance of introducing an error over time. Instead, I think the miner should notify the power actor of the locked tokens the same way it does for every other locking: by invoking the update pledge method. We might need to re-arrange this method to ensure the state is ready for the re-entrant call and will not be overwritten.

It was initially intended to be written this way, but the current method doesn't support it. The behavior of the existing method had to be modified.

fn update_pledge_total( rt: &impl Runtime, params: UpdatePledgeTotalParams, ) -> Result<(), ActorError> { rt.validate_immediate_caller_type(std::iter::once(&Type::Miner))?; rt.transaction(|st: &mut State, rt| { st.validate_miner_has_claim(rt.store(), &rt.message().caller())?; st.add_pledge_total(params.pledge_delta); if st.total_pledge_collateral.is_negative() { // NOTICE need allow negative value in this line return Err(actor_error!( illegal_state, "negative total pledge collateral {}", st.total_pledge_collateral )); } Ok(()) }) }

anorth · 2023-09-18T00:18:18Z

integration_tests/src/tests/commit_post_test.rs

+                            Expect::power_update_pledge(
+                                id_addr.id().unwrap(),
+                                Some(TokenAmount::from_atto(BigInt::from_signed_bytes_be(&[
+                                    255, 17, 91, 207, 234, 13, 8, 99, 78,


This is completely opaque to me. Please use de/serialization code to produce this value so we can see where it comes from.

tediou5 · 2024-03-28T06:52:41Z

I've finished a new implementation, please close this pr, thanks
@anorth
FIP-0077

anorth · 2024-04-04T19:12:36Z

Closing as requested

hunjixin and others added 2 commits September 1, 2023 10:50

feat: add miner deposit

de7357a

Merge branch 'master' into feat/add_miner_deposit

2806c1b

hunjixin requested review from anorth, alexytsu and ZenGround0 and removed request for alexytsu September 1, 2023 05:05

Merge branch 'master' into feat/add_miner_deposit

b5cc6ad

anorth reviewed Sep 18, 2023

View reviewed changes

anorth closed this Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add miner deposit #1398

feat: add miner deposit #1398

hunjixin commented Sep 1, 2023 •

edited

Loading

codecov-commenter commented Sep 1, 2023 •

edited

Loading

anorth left a comment

anorth Sep 18, 2023

hunjixin Oct 10, 2023

anorth Sep 18, 2023

anorth Sep 18, 2023

hunjixin Oct 10, 2023

anorth Sep 18, 2023

hunjixin Oct 10, 2023

hunjixin Oct 10, 2023

hunjixin Oct 10, 2023

anorth Sep 18, 2023

hunjixin Oct 10, 2023 •

edited

Loading

anorth Sep 18, 2023

anorth Sep 18, 2023

hunjixin Oct 10, 2023

anorth Sep 18, 2023

tediou5 commented Mar 28, 2024

anorth commented Apr 4, 2024

feat: add miner deposit #1398

feat: add miner deposit #1398

Conversation

hunjixin commented Sep 1, 2023 • edited Loading

codecov-commenter commented Sep 1, 2023 • edited Loading

Codecov Report

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

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

hunjixin Oct 10, 2023 • 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

tediou5 commented Mar 28, 2024

anorth commented Apr 4, 2024

hunjixin commented Sep 1, 2023 •

edited

Loading

codecov-commenter commented Sep 1, 2023 •

edited

Loading

hunjixin Oct 10, 2023 •

edited

Loading