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(miner): remove batch balancer-related functionality #12919

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tediou5
Copy link

@tediou5 tediou5 commented Feb 25, 2025

Related Issues

close #12902

Proposed Changes

Removed BatchPreCommitAboveBaseFee, AggregateAboveBaseFee, and the related checks.

The MinCommitBatch is actually constrained by the SNARK circuits, so it will remain at 4 without change:

  • You need 4 or more sectors to ensure that the verification time for aggregated proofs is smaller than for batched proofs.

  • You need 13 sectors or more to ensure that the aggregated proof size is smaller than the batched proof size.

The MaxCommitBatch limit comes from the original (FIP-0013), and it seems there's not much incentive to modify it, so these two parameters will remain unchanged.

Additional Info

Checklist

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

Sorry, something went wrong.

@BigLep BigLep requested a review from rvagg February 25, 2025 08:00
@BigLep
Copy link
Member

BigLep commented Feb 26, 2025

@tediou5 : no pressure, but is this ready for review? If so, lets move it out of draft to make it clear. Thanks.

@BigLep
Copy link
Member

BigLep commented Feb 26, 2025

@tediou5 : thanks for working on this. We wanted to make sure you saw #12902 (comment)

@tediou5
Copy link
Author

tediou5 commented Feb 27, 2025

@tediou5 : thanks for working on this. We wanted to make sure you saw #12902 (comment)

@BigLep Got it, let me check the part of aggregation.

@tediou5
Copy link
Author

tediou5 commented Feb 27, 2025

@BigLep Yes, I think it's ready for review now. I'd like to get some feedback as well since I'm not very familiar with this part of the code, and I'm a bit worried I might have missed something.

@tediou5 tediou5 marked this pull request as ready for review February 27, 2025 07:19
@tediou5 tediou5 force-pushed the feat/lotus-miner/impl-fip0100 branch from e01b8bf to 6426700 Compare March 3, 2025 08:05
@Stebalien
Copy link
Member

Stebalien commented Mar 7, 2025

Please run make gen and commit the results.

Also, this appears to now be submitting batches that are too large (See the failing tests):

batch of 1000 too large, max 256

I think this is the max batch size for precommit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ⌨️ In Progress
Status: 🔎 Awaiting Review
Development

Successfully merging this pull request may close these issues.

FIP 0100: remove batch balancer-related functionality
3 participants