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: niporep: multi-sector onboarding through UnmanagedMiner #12180

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jul 4, 2024

  • Test various multi-sector cases with niporep
  • UnmanagedMiner can now onboard multiple sectors for one miner and manage PoSt for all of them

This has tests for both real and mock proofs with NI-PoRep, it includes testing 1 and more (up to max 65) sectors in mock proofs, including cases where there are expected sector failures but activation can still continue. They also proceed to confirm Window PoSt on all the sectors and that the miner gets the appropriate power for them all. Then they continue to snap a deal into one of the onboarded sectors (and because of the new PoSt handling, the new snapped sector can be successfully PoSted if need be--although there's no need here beyond just possible accident of snapping and then needing to PoSt before the test ends).

Lots of refactoring in UnmanagedMiner to make this all work, and because I've spent so much time in there there's a fair bit of reorganisation too. Unfortunately that means there's a significant amount of diff churn in here, some of which is just moving things around. There's now only 1 main onboarding method: TestUnmanagedMiner#OnboardSectors(proofType abi.RegisteredSealProof, withPieces bool, count int, opts ...OnboardOpt) []OnboardedSector. This handles all cases of onboarding new sectors, including for the existing manual onboarding test. There's also SnapDeal to do a single deal snap into a specific sector.

WindowPoSt stuff has been heavily reworked, some of it simplified, although there's new complexity thanks to partition management.

There's an internal sectorInfo map for all committed sectors for a miner that retains all properties (we kept a lot of these in separate maps previously). This only gets saved to the miner on commit, but prior to that it gets built up through the precommit and provecommit generation phases. Only after we successfully submit a commit and the sector is acknowledged as successfully activated do we save it to the miner, and then Window PoSt can pick it up, and SnapDeal can work with it. There's also OnboardedSector which is a subset of sector info that we can return to the caller. I decided not to expose everything but to keep the public bits as simple as is needed for now.

In the NI-PoRep itest, it now has a list of scenarios that it runs through. Currently only the mock proofs ones are run locally or in CI. The real proofs need LOTUS_RUN_VERY_EXPENSIVE_TESTS=1 and some of them are explicitly skipped so the skip: true needs to be commented out. They are mostly useful for now for local testing as they are quite costly and not as useful for a future where we have some way of running LOTUS_RUN_VERY_EXPENSIVE_TESTS=1 in CI.

@rvagg rvagg requested a review from aarshkshah1992 July 4, 2024 07:05
@jennijuju jennijuju mentioned this pull request Jul 4, 2024
8 tasks
@rvagg rvagg force-pushed the rvagg/niporep-multiple-sectors branch 2 times, most recently from 06022ee to 7cb4edb Compare July 8, 2024 06:34
itests/niporep_manual_test.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member Author

rvagg commented Jul 8, 2024

Working

--- PASS: TestManualNISectorOnboarding (2080.88s)
    --- PASS: TestManualNISectorOnboarding/mock_proofs,_miners_with_1,_3_(1_bad)_and_6_sectors (63.81s)
    --- PASS: TestManualNISectorOnboarding/mock_proofs,_miner_with_65_sectors (18.37s)
    --- PASS: TestManualNISectorOnboarding/real_proofs,_1_miner_with_1_sector (142.48s)
    --- PASS: TestManualNISectorOnboarding/real_proofs,_1_miner_with_2_sectors (226.80s)
    --- PASS: TestManualNISectorOnboarding/real_proofs,_1_miner_with_3_sectors (255.70s)
    --- PASS: TestManualNISectorOnboarding/real_proofs,_1_miner_with_4_sectors (314.56s)
    --- PASS: TestManualNISectorOnboarding/real_proofs,_1_miner_with_7_sectors (535.01s)
    --- PASS: TestManualNISectorOnboarding/real_proofs,_1_miner_with_7_sectors,_1_bad (524.15s)

Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

Review 1

itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

Review 2

itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/kit/node_unmanaged.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/niporep_manual_test.go Show resolved Hide resolved
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

Finished.

itests/niporep_manual_test.go Outdated Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
itests/kit/node_unmanaged.go Show resolved Hide resolved
@aarshkshah1992 aarshkshah1992 added the skip/changelog This change does not require CHANGELOG.md update label Jul 9, 2024
@rvagg rvagg force-pushed the rvagg/niporep-multiple-sectors branch from 94ab87c to dcbebdb Compare July 9, 2024 10:25
@rvagg rvagg added skip/changelog This change does not require CHANGELOG.md update and removed skip/changelog This change does not require CHANGELOG.md update labels Jul 9, 2024
@rvagg rvagg force-pushed the rvagg/niporep-multiple-sectors branch from dcbebdb to aa4d6a1 Compare July 9, 2024 10:32
* Test various multi-sector cases with niporep
* UnmanagedMiner can now onboard multiple sectors for one miner and
  manage PoSt for all of them
@rvagg rvagg force-pushed the rvagg/niporep-multiple-sectors branch from d0343b2 to 676dbec Compare July 9, 2024 11:52
@rvagg rvagg merged commit dbe8b2d into master Jul 9, 2024
79 checks passed
@rvagg rvagg deleted the rvagg/niporep-multiple-sectors branch July 9, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants