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

Enable bootup from genesis (e.g., Butterfly testing) #12121

Closed
Tracked by #11939
BigLep opened this issue Jun 20, 2024 · 3 comments · Fixed by #12132
Closed
Tracked by #11939

Enable bootup from genesis (e.g., Butterfly testing) #12121

BigLep opened this issue Jun 20, 2024 · 3 comments · Fixed by #12132
Assignees
Labels
P1 P1: Must be resolved

Comments

@BigLep
Copy link
Member

BigLep commented Jun 20, 2024

Done Criteria

As part of the network upgrade process, there is a way to start from genesis (which is ideally "non-Frankenstein") to enable things like local devnet and Butterfly testing.

Why Important

Testing on a devnet or Butterfly are important for faster iteration cycles than on Calibration or Mainnet. As a result of filecoin-project/builtin-actors#1540 for FIP-0084, this ability has been lost.

User/Customer

FIP implementers, maintainers

Notes

  1. FIP-0084: Remove ProveCommit and dependencies builtin-actors#1540 removed ConfirmSectorProofsValid but lotus needs it for preseals to get a genesis miner started.
    fix: restore ConfirmSectorProofsValid builtin-actors#1553 was one proposal for solving this, but it was deemed only acceptable for local devnet testing. I don't think we want to make the Lotus release/testing setup even more complicated with specially patched builtin-actors for "testing-from-genesis" scenarios.
  2. There was #fil-lotus-dev discussion on 2024-06-18 about this in https://filecoinproject.slack.com/archives/CP50PPW2X/p1718677407074449
@BigLep BigLep added the P1 P1: Must be resolved label Jun 20, 2024
@BigLep
Copy link
Member Author

BigLep commented Jun 20, 2024

@ZenGround0 : I created this issue and assigned it to you, at least to help clarify the next steps. I've marked this as P1 because I believe nv23 butterfly testing is blocked. I assume the FilOz team will talk offline on competing priorities here.

@ZenGround0
Copy link
Contributor

ZenGround0 commented Jun 20, 2024

Whats going on / how to fix it

When we setup genesis state we need to setup miners with power. Today we use the actors logic in ConfirmSectorProofsValid to modify the genesis state to add new sector commitments.

Annoying history lesson

A little history on CSPV. Until FIP 0084 the main way for miners to onboard sectors was as follows

  1. Send a precommit to PreCommitSector(s2)
  2. Send a prove commit to ProveCommitSector at some point later
  3. ProveCommitSector would NOT verify the PoRep it would send a message to the power actor which would enqueue the PoRep and its auxillary data for proving
  4. During cron the power actor verifies all the poreps in the queue
  5. Finally power actor cron sends a CSPV message to the miner actor responsible for the porep saying "hey there miner actor you have a valid porep you can change your state to account for this new sector you've onboarded"

The present

This historical rube goldberg machine was removed in FIP 0084 so the only way to modify miner state to account for a new sector is to call ProveCommitSectors3 (or ProveCommitAggregate) which directly does the proving of the porep and the state accounting update in the same message.

I think the reason preseal used CSPV instead of porep is that CSPV conveniently doesn't need to verify anything, it is called by someone who already verified porep. Preseals don't have valid proofs so the Porep verification needs to be faked out if we hit it in our function setting up state.

However faking this out is not so bad, in fact its an expected part of the VM being used for genesis generation as seen in the fakedSigSyscalls struct.

The correct thing to do is modify this to allow porep verification to pass and construct the input to ProveCommitSectors3 corresponding to preseal precommit.

We've gotta be careful to guard new and some pre-existing code paths with version checks. For example here I think we'll want to wrap this message send around if nv < 23 because ProveCommitSectors should actually send the power update directly.

--edit-- ^ I take that back its actually window post that adds power so no need to touch that. But the point stands about subtlety in guarding code paths with nvs.

@anorth
Copy link
Member

anorth commented Jun 20, 2024

@ZenGround0 's proposal sounds fine, but I'll throw out an alternative for longer term. We could add a built-in miner method in the build-in actors that creates genesis power (could skip pre-commit too). We'd need to ensure this can only be invoked by the System actor, but then we'd have some genesis-making code that would be maintained alongside the actors. They might find it useful for integration tests anyway.

@BigLep BigLep moved this from 🥞 Todo to 🔍 In Review in nv23 Track Board Jun 24, 2024
@github-project-automation github-project-automation bot moved this from 🔍 In Review to ✅ Done in nv23 Track Board Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 P1: Must be resolved
Projects
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

4 participants