Skip to content

feat(miner): remove batch balancer-related functionality#12919

Merged
rvagg merged 1 commit intofilecoin-project:masterfrom
tediou5:feat/lotus-miner/impl-fip0100
Mar 19, 2025
Merged

feat(miner): remove batch balancer-related functionality#12919
rvagg merged 1 commit intofilecoin-project:masterfrom
tediou5:feat/lotus-miner/impl-fip0100

Conversation

@tediou5
Copy link
Copy Markdown
Contributor

@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:

@BigLep BigLep requested a review from rvagg February 25, 2025 08:00
@BigLep
Copy link
Copy Markdown
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 BigLep moved this from 🐱 Todo to ⌨️ In Progress in nv25 Track Board Feb 26, 2025
@BigLep
Copy link
Copy Markdown
Member

BigLep commented Feb 26, 2025

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

@tediou5
Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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
@BigLep BigLep moved this from ⌨️ In Progress to 🔎 Awaiting Review in nv25 Track Board Feb 27, 2025
@tediou5 tediou5 force-pushed the feat/lotus-miner/impl-fip0100 branch from e01b8bf to 6426700 Compare March 3, 2025 08:05
@Stebalien
Copy link
Copy Markdown
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?

@tediou5 tediou5 force-pushed the feat/lotus-miner/impl-fip0100 branch 3 times, most recently from fd62830 to 792fc55 Compare March 10, 2025 05:32
@rvagg
Copy link
Copy Markdown
Member

rvagg commented Mar 12, 2025

Sorry for the delay in bringing up this important point: I think we're going to have to retain all of this functionality but gate it behind a network version check, at least until the feature is live (nv25?), then we can remove it. The problem is that if we roll out a version of lotus-miner with all of this removed, we're either going to throw users to the wolves of not having this functionality in place during a period where there is the batch balancer in operation (nv24), or advise them to upgrade just after the upgrade that activates FIP-0100.

So instead, we should:

  • Retain the config settings but mark them clearly as DEPRECATED and note they will be removed in the next release
  • Gate everything with a call to StateNetworkVersion and do a if nv < network.Version25 around everything that involves the batch balancer.

e.g.

^ we should go through and remove all of these too, in a separate PR

@BigLep
Copy link
Copy Markdown
Member

BigLep commented Mar 13, 2025

@tediou5 : do you think you'll be able to make the updates so we can get this merged by the end of the week (2025-03-14) so an RC can be cut at the beginning of next week (2025-03-17)?

@tediou5
Copy link
Copy Markdown
Contributor Author

tediou5 commented Mar 14, 2025

@tediou5 : do you think you'll be able to make the updates so we can get this merged by the end of the week (2025-03-14) so an RC can be cut at the beginning of next week (2025-03-17)?

@BigLep I think I can get it done. Let me give it a try.

@tediou5 tediou5 force-pushed the feat/lotus-miner/impl-fip0100 branch from 792fc55 to 2c3a301 Compare March 14, 2025 07:19
@tediou5 tediou5 moved this from ⌨️ In Progress to 🔎 Awaiting Review in FilOz Mar 14, 2025
@tediou5
Copy link
Copy Markdown
Contributor Author

tediou5 commented Mar 14, 2025

@Stebalien @rvagg @BigLep Restored the previous logic and now execute it conditionally based on the network version.

@tediou5 tediou5 force-pushed the feat/lotus-miner/impl-fip0100 branch from 2c3a301 to b0ee00a Compare March 14, 2025 07:40
Comment thread chain/actors/policy/policy.go Outdated
Comment thread cmd/lotus-sim/simulation/stages/precommit_stage.go
Comment thread node/config/def.go
Copy link
Copy Markdown
Contributor Author

@tediou5 tediou5 left a comment

Choose a reason for hiding this comment

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

Thank you for your review.

Comment thread cmd/lotus-sim/simulation/stages/precommit_stage.go
Comment thread chain/actors/policy/policy.go Outdated
@BigLep
Copy link
Copy Markdown
Member

BigLep commented Mar 18, 2025

@tediou5 : I'm trying to follow where this is at. Has all feedback been incorporated except for where you have questions?

Copy link
Copy Markdown
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM. We're going to need to do this the right way without hard-coded constants, but this PR has the MVP for the upgrade.

@github-project-automation github-project-automation Bot moved this from 🔎 Awaiting Review to ✔️ Approved by reviewer in FilOz Mar 18, 2025
@rvagg rvagg force-pushed the feat/lotus-miner/impl-fip0100 branch from 354dfee to aa9972c Compare March 19, 2025 02:46
@rvagg rvagg merged commit 945451c into filecoin-project:master Mar 19, 2025
88 checks passed
@github-project-automation github-project-automation Bot moved this from ✔️ Approved by reviewer to 🎉 Done in FilOz Mar 19, 2025
@github-project-automation github-project-automation Bot moved this from 🔎 Awaiting Review to 🎉 Done in nv25 Track Board Mar 19, 2025
@tediou5
Copy link
Copy Markdown
Contributor Author

tediou5 commented Mar 19, 2025

LGTM. We're going to need to do this the right way without hard-coded constants, but this PR has the MVP for the upgrade.

Yes, I agree. I prefer to estimate gas so that messages are only constrained by MaxBlockGasLimit rather than a fixed constant. But I think I'll submit this in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🎉 Done
Status: 🎉 Done

Development

Successfully merging this pull request may close these issues.

FIP 0100: remove batch balancer-related functionality

4 participants