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

Miner testing cleanup after FIP-0084 #1545

Open
ZenGround0 opened this issue May 25, 2024 · 2 comments
Open

Miner testing cleanup after FIP-0084 #1545

ZenGround0 opened this issue May 25, 2024 · 2 comments
Labels

Comments

@ZenGround0
Copy link
Contributor

ZenGround0 commented May 25, 2024

Cleanup configuration of commit_and_prove_sectors

commit_and_prove_sectors and commit_and_prove_sectors_with_cfgs are helper functions that add sectors to a miner actor in tests. Adding a sector is an important intermediate step for most of the miner actor's testing. There are > 40 calls to commit_and_prove_sectors or commit_and_prove_sectors_with_cfgs throughout miner actor tests.

#1540 changes these helper functions to use the new ProveCommitSectors3 and PreCommitSectors2 methods as part of deprecating ProveCommitSector.

The configuration for commit_and_prove_sectors had grown into a bad state when deals are concerned. It is essential for callers to provide two separate configuration states: a vector of deal_ids and a ProveCommitCfg. Before #1540 this was not as obvious because the deal_ids would be passed to a precommit which would then statefully hold configuration information. In the new flow it is an error for deal_ids to be passed to pre commit so now both configurations must be passed not just to commit_and_prove_sectors_with_cfgs but also deprecated_sector_commit internally.

In order to keep things working consistently both pre commiting in commit_and_prove_sectors_with_cfgs and prove committing in deprecated_sector_commit use the make_piece_specs_from_configs function to deterministically generate an assignment between deal ids and deals. But this feels not quite right.

One way to clean this up is to modify the ProveCommitCfg so that users must associate deal ids with deal states.

Another is to rely on more prove_commit_sectors3 native configuration info in the form of manifests. The recently added onboard_sectors and onboard_empty_sectors helper methods have not been significantly integrated into existing tests. Adapting these new methods and integrating them into other tests is another approach that would result in a cleaner factoring.

Remove deprecated_sector_commit

Both commit_and_prove_sectors and a few stray external callers still call the deprecated_sector_commit method. As of #1540 this is now a wrapper around prove_commit_sectors3. We should look into removing this method and either call prove_commit_sectors3 directly or with a better wrapper function. We can then easily remove ProveCommitSector params types and associated testing functions for constructing them.

@ZenGround0
Copy link
Contributor Author

Naming cleanup

Rename actor/miner/tests files prove_commit2_failures_test and prove_commit2_test to be prove_commit3

@anorth
Copy link
Member

anorth commented May 27, 2024

See also #698 while investigating this issue.

@anorth anorth added the cleanup label May 27, 2024
@anorth anorth changed the title Miner testing cleanup Miner testing cleanup after FIP-0084 May 28, 2024
@anorth anorth added this to FilOz May 28, 2024
@rjan90 rjan90 moved this to 📌 Triage in FilOz Jun 4, 2024
@rjan90 rjan90 removed the status in FilOz Jun 4, 2024
@rjan90 rjan90 moved this to 🐱 Todo in FilOz Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🐱 Todo
Status: 🥞 Todo
Development

No branches or pull requests

2 participants