Feature: move partition #1326

zhiqiangxu · 2023-06-30T01:29:31Z

This PR implements this FIP.

…ion_verify

anorth

Thanks for this. I haven't looked at the tests much until we align on the implementation.

The big outstanding question is whether this should more literally just move the partition, intact, to a new deadline, or remove a partition and add the sectors individually to the new deadline. You've done the latter, but I think the former will be better, definitely cheaper. But it does require adding a new method on Deadline state to accept a new partition.

Also there seems to be some confusion over which partitions to move. Only prove and move those the caller specified.

anorth · 2023-07-17T21:59:00Z

actors/miner/src/deadlines.rs

+    deadline_distance(policy, current_deadline, to_deadline)
+        < deadline_distance(policy, current_deadline, from_deadline)
+}
+


These look correct, but please add tests to demonstrate all the cases.

actors/miner/src/lib.rs

anorth · 2023-07-17T22:56:09Z

actors/miner/src/lib.rs

+            state.delete_sectors(store, &dead).map_err(|e| {
+                e.downcast_default(ExitCode::USR_ILLEGAL_STATE, "failed to delete dead sectors")
+            })?;


We don't want to do this if keeping the partition intact (including its reference to terminated sectors) while moving. I can see below that's not what you're doing, but please leave this comment until we resolve that.

I'm looking into the "moving partitions intact" approach, but Deadline.expirations_epochs seems quite complicated to maintain while moving. I guess it's the main reason current compact_partitions was implemented by removing then adding sectors.

@anorth Not sure whether it's worth to load sector infos just to calculate an accurate epoch for Deadline.expirations_epochs. Does it matter if the epoch is a period later? If that's the case, we can simply use the source epoch to calculate the target epoch with a different quant.

Hmm, I see. I think we should avoid loading sector info if at all possible.

If we say that a partition with faulty sectors can't be moved, then I think we only need to worry about on-time expirations. In that case I suggest rounding up, i.e. find the next epoch quantized to the new deadline that is not sooner than epoch in the queue. Then the SP might need to maintain a sector up to 1 proving period longer than committed.

Don't worry about removing anything from the old deadline's queue. It will have redundant entries, but they're harmless.

What about other sectors: faulty, unproven, terminated? I was considering to allow all sectors for this "moving partitions intact" approach.

Also, I found there is a test case which requires the epoch to be exactly quant.quantize_up(sector.expiration), should we modify this check?

I've temporarily pushed the latest commit here in case you want to try the test cases like ok_to_move_partitions_from_safe_epoch.

I would like to consider allowing any partition to be moved too, but at the moment I don't think it's worth the work/risk of changing how remove_partitions works at the moment. Let's require non-faulty, fully proven sectors for now.

Yes I think we should probably remove that test case. The quantisation is mentioned elsewhere too but I think we can probably remove it, and update associated code to handle arbitrary quantisation. This can be an independent change ahead of what you're doing here – for now just comment out that state assertion.

Please do push that commit to this branch - I think it's the right approach.

Also, I found there is a test case which requires the epoch to be exactly quant.quantize_up(sector.expiration), should we modify this check?

I looked into this and I think we can just make a simple change to make it epoch >= target.

I changed it a bit more restrictive: epoch >= target && (epoch-target)%quant.unit ==0

actors/miner/src/lib.rs

…ion_verify

jennijuju · 2023-09-28T12:09:23Z

could you please point me to where are we checking the amount of partitions after of to deadline doesn't exceed max_partitions_per_deadline after the moving?

jennijuju · 2023-09-28T12:27:28Z

can we add some tests for dispute scenarios as well, please?

Stebalien · 2023-09-28T12:38:07Z

could you please point me to where are we checking the amount of partitions after of to deadline doesn't exceed max_partitions_per_deadline after the moving?

That's a good point, I don't think we're doing that.

2. add check for `max_partitions_per_deadline`

zhiqiangxu · 2023-09-28T12:59:27Z

could you please point me to where are we checking the amount of partitions after of to deadline doesn't exceed max_partitions_per_deadline after the moving?

Good point, just added this check!

zhiqiangxu · 2023-09-28T13:11:31Z

can we add some tests for dispute scenarios as well, please?

OK, this may take a bit longer though.

jennijuju · 2023-09-28T13:45:09Z

can we add some tests for dispute scenarios as well, please?

OK, this may take a bit longer though.

Can be added in a follow up pr too!

zhiqiangxu · 2023-09-28T13:46:53Z

can we add some tests for dispute scenarios as well, please?

OK, this may take a bit longer though.

Can be added in a follow up pr too!

I'll add it in a follow up pr then, and we'll also manually test this case!

anorth

LGTM with more tests to come in follow-up

anorth · 2023-09-29T09:15:24Z

actors/miner/src/lib.rs

+            let mut orig_deadline =
+                deadlines.load_deadline(store, params.orig_deadline).context_code(
+                    ExitCode::USR_ILLEGAL_STATE,
+                    format!("failed to load deadline {}", params.orig_deadline),


Nit: use with_context_code when doing a format! operation for the message

Done, I guess this is to avoid alloc when no error happens?

anorth · 2023-10-10T00:04:41Z

actors/miner/tests/move_partitions_test.rs

+        || {
+            let current_deadline = h.current_deadline(&rt);
+
+            let from_deadline = new_deadline_info(
+                rt.policy(),
+                if current_deadline.index < orig_deadline_id {
+                    current_deadline.period_start - rt.policy().wpost_proving_period
+                } else {
+                    current_deadline.period_start
+                },
+                orig_deadline_id,
+                *rt.epoch.borrow(),
+            );
+
+            let from_ddl = h.get_deadline(&rt, orig_deadline_id);
+
+            let entropy = RawBytes::serialize(h.receiver).unwrap();
+            rt.expect_get_randomness_from_beacon(
+                DomainSeparationTag::WindowedPoStChallengeSeed,
+                from_deadline.challenge,
+                entropy.to_vec(),
+                TEST_RANDOMNESS_ARRAY_FROM_ONE,
+            );
+
+            let post = h.get_submitted_proof(&rt, &from_ddl, 0);
+
+            let all_ignored = BitField::new();
+            let vi = h.make_window_post_verify_info(
+                &sectors_info,
+                &all_ignored,
+                sectors_info[1].clone(),
+                Randomness(TEST_RANDOMNESS_ARRAY_FROM_ONE.into()),
+                post.proofs,
+            );
+            rt.expect_verify_post(vi, ExitCode::OK);
+        },


This window post expectation-setting is long and duplicated in other tests. Can you follow up to factor it out into either a flag on move_partitions, or another method on the harness that returns this closure?

Just saw the comments here, I'll do it tomorrow.

anorth · 2023-10-10T00:09:41Z

test_vm/tests/move_partitions_test.rs

+use vm_api::util::{apply_ok, get_state, DynBlockstore};
+use vm_api::VM;
+
+#[test]


In order to export tests to be run in other frameworks, test entry points no longer live here. Instead, they go in the test_vm package with a simple wrapper that invokes a method here with a single VM parameter. Please follow up to use the same pattern as other integration tests.

zhiqiangxu · 2023-10-11T02:33:20Z

@anorth I just pushed the suggested modifications to the follow-up PR: #1430

This reverts commit 5ec2a6b.

* Revert "Feature: move partition (#1326)" This reverts commit 5ec2a6b. * ci * run CI

* Revert "Feature: move partition (#1326)" This reverts commit 5ec2a6b. * update amt deps

zhiqiangxu added 15 commits June 28, 2023 09:41

init

924722e

avoid recompute current_deadline

da95dbb

treat empty bitfield as all

e502709

rm useless quote

eaf5370

add verify

2fa9693

combine option1 & option2

8725420

fix

d534373

fix

36caece

nit

badb691

mod error

fd39aac

nit

daab781

fmt

0271369

fix ci

c046cc6

fix bug

3eac91e

add test

181dc2e

zhiqiangxu force-pushed the feature/move_partition_verify branch from 23d838a to 181dc2e Compare July 4, 2023 02:52

zhiqiangxu added 2 commits July 5, 2023 14:15

add more test

8eb2243

Merge remote-tracking branch 'origin/master' into feature/move_partit…

394ea44

…ion_verify

ZenGround0 mentioned this pull request Jul 13, 2023

Filecoin storage capacity modeling - Miner cron analysis filecoin-project/lotus#11034

Closed

4 tasks

anorth mentioned this pull request Jul 13, 2023

First draft of FIP-0070 move partitions filecoin-project/FIPs#740

Merged

anorth reviewed Jul 17, 2023

View reviewed changes

zhiqiangxu added 9 commits July 18, 2023 22:13

partial fix for review

d1fdb0f

Merge remote-tracking branch 'origin/master' into feature/move_partit…

0804565

…ion_verify

adjust test

0353a28

use .context_code

bc9e6a1

fix for test

c1d88a2

disallow empty partitions

37d7d03

refactor deadline_available_for_move

89bcb74

fix for clippy

79ab15d

minor opt

39d6447

1. review fix

7619065

2. add check for `max_partitions_per_deadline`

fix comment

6e91261

anorth approved these changes Sep 29, 2023

View reviewed changes

use with_context_code

9964257

Stebalien approved these changes Sep 29, 2023

View reviewed changes

Stebalien removed request for lemmih, ZenGround0, arajasek and LesnyRumcajs September 29, 2023 13:12

Stebalien added this pull request to the merge queue Sep 29, 2023

Merged via the queue into filecoin-project:master with commit 5ec2a6b Sep 29, 2023
11 checks passed

zhiqiangxu mentioned this pull request Oct 7, 2023

add testcase for dispute after move partition #1430

Merged

anorth reviewed Oct 10, 2023

View reviewed changes

aarshkshah1992 added a commit that referenced this pull request Nov 10, 2023

Revert "Feature: move partition (#1326)"

e3f8661

This reverts commit 5ec2a6b.

aarshkshah1992 mentioned this pull request Nov 10, 2023

Revert "Feature: move partition (#1326)" #1480

Merged

aarshkshah1992 added a commit that referenced this pull request Nov 10, 2023

Revert "Feature: move partition (#1326)"

d1ca74e

This reverts commit 5ec2a6b.

This was referenced Nov 10, 2023

Revert "Feature: move partition (#1326)" from nv12 #1481

Merged

feat: chain: light-weight patch to fix calibrationnet again by removing move_partitions from built-in actors filecoin-project/lotus#11409

Merged

arajasek pushed a commit that referenced this pull request Nov 14, 2023

Revert "Feature: move partition (#1326)" from nv12 (#1481)

1b8c528

* Revert "Feature: move partition (#1326)" This reverts commit 5ec2a6b. * ci * run CI

arajasek pushed a commit that referenced this pull request Nov 14, 2023

Revert "Feature: move partition (#1326)" from nv12 (#1481)

de366e3

* Revert "Feature: move partition (#1326)" This reverts commit 5ec2a6b. * ci * run CI

github-merge-queue bot pushed a commit that referenced this pull request Dec 12, 2023

Revert "Feature: move partition (#1326)" (#1480)

81595c2

* Revert "Feature: move partition (#1326)" This reverts commit 5ec2a6b. * update amt deps

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: move partition #1326

Feature: move partition #1326

zhiqiangxu commented Jun 30, 2023 •

edited

Loading

anorth left a comment

anorth Jul 17, 2023

anorth Jul 17, 2023

zhiqiangxu Jul 21, 2023 •

edited

Loading

anorth Jul 21, 2023

anorth Jul 21, 2023

zhiqiangxu Jul 21, 2023 •

edited

Loading

anorth Jul 23, 2023

anorth Jul 23, 2023

anorth Jul 23, 2023

zhiqiangxu Jul 25, 2023 •

edited

Loading

jennijuju commented Sep 28, 2023 •

edited

Loading

jennijuju commented Sep 28, 2023

Stebalien commented Sep 28, 2023

zhiqiangxu commented Sep 28, 2023

zhiqiangxu commented Sep 28, 2023 •

edited

Loading

jennijuju commented Sep 28, 2023

zhiqiangxu commented Sep 28, 2023

anorth left a comment

anorth Sep 29, 2023

zhiqiangxu Sep 29, 2023

Stebalien Sep 29, 2023

anorth Oct 10, 2023

zhiqiangxu Oct 10, 2023

anorth Oct 10, 2023

zhiqiangxu commented Oct 11, 2023 •

edited

Loading

Feature: move partition #1326

Feature: move partition #1326

Conversation

zhiqiangxu commented Jun 30, 2023 • edited Loading

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

zhiqiangxu Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiqiangxu Jul 21, 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

zhiqiangxu Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

jennijuju commented Sep 28, 2023 • edited Loading

jennijuju commented Sep 28, 2023

Stebalien commented Sep 28, 2023

zhiqiangxu commented Sep 28, 2023

zhiqiangxu commented Sep 28, 2023 • edited Loading

jennijuju commented Sep 28, 2023

zhiqiangxu commented Sep 28, 2023

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

zhiqiangxu commented Oct 11, 2023 • edited Loading

zhiqiangxu commented Jun 30, 2023 •

edited

Loading

zhiqiangxu Jul 21, 2023 •

edited

Loading

zhiqiangxu Jul 21, 2023 •

edited

Loading

zhiqiangxu Jul 25, 2023 •

edited

Loading

jennijuju commented Sep 28, 2023 •

edited

Loading

zhiqiangxu commented Sep 28, 2023 •

edited

Loading

zhiqiangxu commented Oct 11, 2023 •

edited

Loading