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

feat: support 512 & 1GiB with huge refactors #717

Merged
merged 13 commits into from
Feb 10, 2025
Merged

Conversation

th7nder
Copy link
Contributor

@th7nder th7nder commented Feb 1, 2025

Description

It adds support for 512MiB & 1 GiB sector sizes.
Required significant refactors, because of the underlying code for proof generation is generic.
Needed to write some macros to make the code more readable, they're not the best though.

Fixes #716.

Note 1: it took 16 minutes to generate parent cache:

2025-02-03T01:12:48.625727Z  INFO storage_proofs_porep::stacked::vanilla::proof: replicate_phase1    
2025-02-03T01:12:48.626083Z  INFO storage_proofs_porep::stacked::vanilla::graph: using parent_cache[2048 / 16777216]    
2025-02-03T01:12:48.631266Z  INFO storage_proofs_porep::stacked::vanilla::cache: parent cache: generating /var/tmp/filecoin-parents/v28-sdr-parent-2aa9c77c3e58259481351cc4be2079cc71e1c9af39700866545c043bfa30fb42.cache    
[...]
2025-02-03T01:29:48.761003Z  INFO storage_proofs_porep::stacked::vanilla::cache: parent cache: generated    

Precommit took 31 mins. Prove Commit another 15 minutes.

Whole maat real_world test takes around 50minutes.
Params:

let seal_proof = primitives::proofs::RegisteredSealProof::StackedDRG512MiBV1;
let post_proof = primitives::proofs::RegisteredPoStProof::StackedDRGWindow512MiBV1;
{
    start_block: 400,
    end_block: 480,
}

Checklist

  • Are there important points that reviewers should know?
    • If yes, which ones?
  • Make sure that you described what this change does.
  • Have you tested this solution?
  • Were there any alternative implementations considered?
  • Did you document new (or modified) APIs?

@th7nder th7nder added the enhancement New feature or request label Feb 1, 2025
@th7nder th7nder added this to the Phase 3 milestone Feb 1, 2025
@th7nder th7nder self-assigned this Feb 1, 2025
@th7nder th7nder added the ready for review Review is needed label Feb 1, 2025
@th7nder th7nder marked this pull request as draft February 3, 2025 00:49
@th7nder
Copy link
Contributor Author

th7nder commented Feb 3, 2025

Blocked on #719

@th7nder th7nder force-pushed the feat/716/512mb branch 3 times, most recently from ab92ed4 to 4c70aa6 Compare February 5, 2025 12:16
@th7nder th7nder marked this pull request as ready for review February 5, 2025 19:22
Copy link
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

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

  • Not a fan of the macro but I get it
  • Would've prefered if you had solved the randomness issue in a separate PR

But I can't argue that it makes sense to me. So far, we've added FC supported (even if unofficially) sizes, it will be interesting if and when trying sizes like 2GiB

Leaving as comment for now for discussion

lib/polka-storage-proofs/src/porep/mod.rs Show resolved Hide resolved
lib/polka-storage-proofs/src/post/mod.rs Show resolved Hide resolved
@th7nder th7nder added ready for review Review is needed and removed ready for review Review is needed labels Feb 6, 2025
@th7nder th7nder requested review from cernicc and jmg-duarte February 6, 2025 14:15
@th7nder th7nder added ready for review Review is needed and removed ready for review Review is needed labels Feb 6, 2025
@th7nder th7nder requested a review from jmg-duarte February 6, 2025 15:36
jmg-duarte
jmg-duarte previously approved these changes Feb 6, 2025
lib/polka-storage-proofs/src/post/mod.rs Show resolved Hide resolved
@th7nder th7nder mentioned this pull request Feb 6, 2025
47 tasks
@pete-eiger
Copy link
Contributor

good stuff man, the new macro in the proofs module is much clearer now. One suggestion: can we make the generic parameters even more explicit by using syntax like match_seal_proof!(seal_proof, precommit_sector::<_, _, _, _>, …) so that it’s obvious which types are being inferred. This might reduce the ‘magic’ and improve readability further

you also mention that generating the parent cache, precommit, and prove commit take a very long time (16, 31, and 15 minutes, respectively). Do we have plans to optimize or cache these steps further? It might be helpful to add some discussion on future work or known performance limitations in the documentation.

pete-eiger
pete-eiger previously approved these changes Feb 7, 2025
Copy link
Contributor

@pete-eiger pete-eiger left a comment

Choose a reason for hiding this comment

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

left a comment above but generally lgtm

@jmg-duarte
Copy link
Contributor

good stuff man, the new macro in the proofs module is much clearer now. One suggestion: can we make the generic parameters even more explicit by using syntax like match_seal_proof!(seal_proof, precommit_sector::<_, _, _, _>, …) so that it’s obvious which types are being inferred. This might reduce the ‘magic’ and improve readability further

@pete-eiger that's what is does now: https://github.com/eigerco/polka-storage/pull/717/files#diff-358758f32b7b2a1a8319e10e612480d4626364d9b4a520f311ca83d3a964075dR312-R324

@th7nder th7nder dismissed stale reviews from pete-eiger and jmg-duarte via 0d8c25e February 7, 2025 16:06
@th7nder
Copy link
Contributor Author

th7nder commented Feb 7, 2025

you also mention that generating the parent cache, precommit, and prove commit take a very long time (16, 31, and 15 minutes, respectively). Do we have plans to optimize or cache these steps further? It might be helpful to add some discussion on future work or known performance limitations in the documentation.

  1. Parent Cache is only generated the first time we're doing the proving, so I'm not gonna change that for now.
  2. Prove Commit/PreCommit take a long time on my CPU, because every step is running on the CPU. Some of the steps can be computed on the GPU, but I didn't test that yet. It's part of Try out building stuff with an actual GPU #723. It'll be faster, but still not like instanteous, because that's the part of the protocol. It must be a bit slow to prevent outsourcing/generation attacks. It's by design.

@th7nder th7nder added ready for review Review is needed and removed ready for review Review is needed labels Feb 7, 2025
@th7nder th7nder added ready for review Review is needed and removed ready for review Review is needed labels Feb 7, 2025
Copy link
Member

@cernicc cernicc left a comment

Choose a reason for hiding this comment

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

Nice 💪

@jmg-duarte jmg-duarte merged commit 5df7d9d into develop Feb 10, 2025
6 checks passed
@jmg-duarte jmg-duarte deleted the feat/716/512mb branch February 10, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready for review Review is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

512MiB & 1GiB Sectors
4 participants