FIP 0013 Aggregate Seal Verification

[ec2]

Summary of changes
Changes introduced in this pull request:

• This implements FIP 0013 which allows for the batching of seal proofs.
• A new method is added to the miner actor. Namely method 26: ProveCommitAggregate. Method 25 will be a different PR which will allow for batching of PreCommits.

Reference issue to close (if applicable)

Closes #1105

Other information and links

[cryptoquick]

This one's kinda hard to review, as most of our actor stuff is. Should I just trust that this is good, since it's supposed to reference actor code from lotus? And with this PR, will we be able to sync mainnet?

[ghost]

LGTM, I took the time to walk through the FIP and the Spec to understand the proving state machine this upgrade improves. It would be great to implement a FIP that updates the VM soon.

[ghost]

LGTM

[cryptoquick]

In the market actor, you're missing the ProveCommitAggregate, even though it's not quite clear if it's used. Still, it might be good to complete these enums.

On this line:
https://github.com/ChainSafe/forest/pull/1185/files#diff-613de2d546245600c363b9b0c6cef398ef3cff18478ee849ddb6bbece5e09d2cR54

You're missing:
ProveCommitAggregate = 26

Also, I'm not sure I'm seeing an apples-to-apples comparison. I think there's changes in filecoin-project/specs-actors#1381 I'd expect to see here but aren't, but it's hard to tell. Also, I'm seeing additional code that's not in that other PR. I'm not fully finished with my review, but let me know.

[ec2]

I am not missing ProveCommitAggregate = 26. Its on L127 in the miner actor's mod.rs.

[cryptoquick]

@ec2 Could you update #1105 with a link to the appropriate Lotus v5 diff so I can review against that also? I think they tag the branches, right?

[ec2]

@ec2 Could you update #1105 with a link to the appropriate Lotus v5 diff so I can review against that also? I think they tag the branches, right?

Ok. I just did.

[cryptoquick]

Alright, I took my time and picked this over with a fine-toothed comb, including the prove_commit_aggregate, request_unsealed_sector_cids, confirm_sector_proofs_valid_internal methods in the miner actor. It looks like a faithful translation. Some things weren't 100% aligned wtih the order in Lotus, like in confirm_sector_proofs_valid_internal, in Forest, vs confirmSectorProofsValid in Lotus. I'm guessing it's because in most cases it's just more Rust-idiomatic to do it that way, but I had to do a bit of scrolling and searching. It takes a lot of time and focus to review these things! I also double checked the other files, like aggregate_network_fee added to the monies actor (plus the math there, that was tricky), and get_all_precommitted_sectors in the miner state file. I'll admit I don't understand the logic in max_prove_commit_duration, so I'd just ask that you double-check that for me, since it looks complex.

Also, I did find a few specific discrepancies I'd like you to confirm for me are correct, and intentional. Overall, really good work.

[cryptoquick]

Just looked over your changes. Awesome, let's get this in. Now for the real challenge! actors/v5 final boss branch, and maintaining mainnet consensus!