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

chore: remove support of v1 proofs in Miner actor #1391

Merged
merged 5 commits into from
Sep 27, 2023

Conversation

elmattic
Copy link
Contributor

@elmattic elmattic commented Aug 30, 2023

We were supporting partially v1 PoSt proof types in v11, now we don't support them anymore in v12.

Q: Do we need to remove v1 variants in RegisteredPoStProof altogether?

Closes #1260

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2023

Codecov Report

Merging #1391 (638154e) into master (a327238) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1391      +/-   ##
==========================================
+ Coverage   91.00%   91.04%   +0.03%     
==========================================
  Files         145      145              
  Lines       27529    27485      -44     
==========================================
- Hits        25053    25023      -30     
+ Misses       2476     2462      -14     
Files Changed Coverage Δ
actors/miner/src/policy.rs 97.64% <ø> (+5.64%) ⬆️
runtime/src/runtime/policy.rs 98.44% <ø> (-0.07%) ⬇️
actors/miner/src/lib.rs 84.82% <100.00%> (+0.08%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Collaborator

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Generally looks good to me!

Q: Do we need to remove v1 variants in RegisteredPoStProof altogether?

Yes, I think we'll want to do so in the FVM when releasing FVM4.

@@ -358,30 +358,24 @@ impl ProofSet {
/// Create a `ProofSet` for enabled `RegisteredPoStProof`s
pub fn default_post_proofs() -> Self {
let mut proofs = vec![false; REGISTERED_POST_PROOF_VARIANTS];
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be able to drop the size of this REGISTERED_POST_PROOF_VARIANTS field now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean exactly by dropping the size of REGISTERED_POST_PROOF_VARIANTS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the number can change from 15 to 10?

Copy link
Contributor Author

@elmattic elmattic Sep 26, 2023

Choose a reason for hiding this comment

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

You mean from 15 to 5, no? Let me know if I am mistaken, I might not have all the context here.

I have also created counterpart PR in the FVM repo: filecoin-project/ref-fvm#1896

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, nvm, I was wrong -- we can't reduce this number at all, because it needs to be indexed up to StackedDRGWindow64GiBV1P1, which is 15.

Copy link
Contributor Author

@elmattic elmattic Sep 27, 2023

Choose a reason for hiding this comment

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

Yes but once the builtin-actors repo is making use of FVM4 this should be possible (the V1P1 proofs are indexed from 0 to 4 there).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, these numbers are consensus-critical, so we can't change them (easily).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

Copy link
Collaborator

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@arajasek arajasek added this pull request to the merge queue Sep 27, 2023
Merged via the queue into master with commit e1d0eef Sep 27, 2023
15 checks passed
@arajasek arajasek deleted the elmattic/drop-v1-proof-support branch September 27, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Miner v12: cleanup: drop all support for v1 window PoSt proof types
3 participants