Manual CC & Snapdeals onboarding (No NI-Porep)

[aarshkshah1992]

Related Issues

This PR aims to refactor #12017 by creating a generic testkit manual miner that can be used to onboard CC sectors and Snap deal sectors. It does not have any of the NI-PoRep tests introduced in #12045.

Proposed Changes

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

• Commits have a clear commit message.
• PR title is in the form of of <PR type>: <area>: <change being made>
  • example: fix: mempool: Introduce a cache for valid signatures
  • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
  • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
• If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
• New features have usage guidelines and / or documentation updates in
  • Lotus Documentation
  • Discussion Tutorials
• Tests exist for new functionality or change in behavior
• CI is green

[rvagg]

There's already a sectorSize in the test kit, I just couldn't figure out a way to feed that back, I wanted to ask the kit "what sector size am I using?" but I think 2k just gets hard-wired everywhere in itests now, which is a little unfortunate. Maybe we should make a SectorSize() method on the miner so we can: use the 2k default that the test kit uses, have a SectorSize() option, and also have a SectorSize() method on the miner so we can work with it.

But also this brought up another, adjacent, issue when I was looking at it, we have this line:

lotus/itests/kit/ensemble.go

Lines 284 to 286 in 6302607

	// Will use 2KiB sectors by default (default value of sectorSize).
	proofType, err := miner.SealProofTypeFromSectorSize(options.sectorSize, n.genesis.version, false)
	require.NoError(n.t, err)

But we also have kit.TestSpt and I'm adding kit.TestSptNi, so hard-wired seal proof types; I don't think they should be hard-wired but maybe we add new methods SealProofType() and SealProofTypeNi() to get them instead of these constants.

[rvagg]

this still needs to go, it's in the opts, we should only have it once

[aarshkshah1992]

Fixed this.

[rvagg]

there's already one on the ensemble itself,

lotus/itests/kit/ensemble_opts.go

Line 38 in 6302607

func MockProofs(e ...bool) EnsembleOpt {

I think we should be able to fetch n.options.mockProofs when we instantiate TestUnmanagedMiner so there's no need for a new option, use what's there, just expose it in the TestUnmanagedMiner struct.

[aarshkshah1992]

I have removed tests with mock proofs completely for now. Will add it back as a separate test.

[rvagg]

I'm a little concerned about the complete removal of mock tests from this because there's so many places where you need to bypass ffi calls when you are running with mock proofs; I'll trust that you've got that under control but it is harder now the logic has been spread around a bit more.

[aarshkshah1992]

I have added back all the mock proof tests (for both cc sectors and sectors with pieces)

[github-actions]

All checks have completed

❌ Failed Check / Check (lint-all) (pull_request)
⏭️ Skipped Check / Check (gen-check) (pull_request)

[aarshkshah1992]

@rvagg This is ready for a review. We can merge this and I can then continue work on top of this.

[rvagg]

log line has MinerB, could either remove this entirely or replace it with f0 actor address—which would be helpful because you can then correlate it with the order they were instantiated

[aarshkshah1992]

Fixed all these log lines.

[rvagg]

ditto log line

[aarshkshah1992]

Fixed.

[rvagg]

MinerB

[rvagg]

2 more in this function

[aarshkshah1992]

Fixed.

[rvagg]

this isn't a loop anymore, it just does one post then exits

which is going to be a problem because we're going to have additions (including ni-porep) that will take longer to onboard so existing sectors need to continue to post

which has implications for other changes in here -- the channel has a buffer of 1, but what if we just want to make it continue to post without waiting for a response? I think my original version had a channel for first post because that's the only one we actually care about so we can check power.

It also disputed all posts to make sure it did the right thing on each post. I don't think that's as critical to keep in the loop, but it was a nice feature that you always got an assertion that your post was valid without having to include additional logic. The only catch with that is when you want to assert that a post should not be valid, but that could also just be a boolean "expect valid" (to check each time and expect a failure to dispute) or "submit dispute" (to check each time or not check at all and make it the caller's responsibility).

I was just doing this last week with ni-porep, experimenting with a test case where the post was invalid and making sure the dispute didn't error to make sure that the dispute mechanism was working.

However I'm actually not sure about the value of adding tests for disputing succeeding, because we have itests/wdpost_dispute_test.go already. I don't know that ni-porep adds anything novel that would need to add a new dispute check on windowpost, we'd need to check on that. But I still think it's nice to add a dispute mechanism when we're not using fake proofs, even if we can't flip between "expect valid" and "expect invalid".

[rvagg]

I guess in these tests you're only needing the single PoSt anyway because you wait and assert power just for that single miner, so if they fail the next post then it shouldn't bother your assertions? So you're probably not going to see flaky failures due to speed differences. Maybe this is OK then and we should expand test coverage by adding new itests rather than expanding this file and having minerC, minerE, etc. Then a single PoSt might be fine for many cases. Possibly not for snapping deals though? That might need some repeated PoSts?

[rvagg]

I've gone ahead and squashed this down and included it in #12017 and also rebased that branch on the current #12005 which has master in it, so we don't get too far out on a limb! We can keep iterating from there rather than having these big chains of PRs and commits.