chore(miner): remove deprecated code as a result of FIP-0100 #13049
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Please update the PR title to match https://github.com/filecoin-project/lotus/blob/master/CONTRIBUTING.md#pr-title-conventions
tediou5
changed the title
There was a problem hiding this comment.
Please update the PR title to match https://github.com/filecoin-project/lotus/blob/master/CONTRIBUTING.md#pr-title-conventions
tediou5
changed the title
There was a problem hiding this comment.
Please update the PR title to match https://github.com/filecoin-project/lotus/blob/master/CONTRIBUTING.md#pr-title-conventions
tediou5
changed the title
github-actions
bot
dismissed
their stale review
PR title now matches the required format.
BigLep
changed the title
github-actions
bot
dismissed
their stale review
PR title now matches the required format.
|
@Stebalien : can you please look given @rvagg is out? |
|
(This is followup to #12919 now that we've upgraded to nv25) |
|
@rvagg : I assume you'll look later this week when you come back online. |
storage/pipeline/commit_batch.go
Outdated
| @@ -233,13 +233,6 @@ func (b *CommitBatcher) maybeStartBatch(notif bool) ([]sealiface.CommitBatchRes, | |||
|
|
|||
| individual := (total < cfg.MinCommitBatch) || (total < miner.MinAggregatedSectors) || blackedOut() || !cfg.AggregateCommits | |||
There was a problem hiding this comment.
this line concerns me a bit and I think we should consider tweaking it
- We could remove all the
blackedOutcode, it's for Skyr and I don't think we need backward compatibility - if we have failing tests because of it we could just remove those tests; I don't think we have a policy of lotus miner being backward compatible, just lotus. - I'd like to propose we remove
AggregateCommitsfrom the config entirely. I can't think of a good reason it should still exist.
But, what I don't have a good handle on is the way that AggregateCommits is used on line 210 where we hold off on a batch if it's true. I worry that the AggregateCommits setting might be confusing two things - whether to use aggregate proofs and how to handle the number of commits in a single PCS3.
@tediou5 would you mind taking a look at this? I'd prefer us to not change the logic that determines when to send a commit message but we want it to default to aggregate proofs whenever possible. I think those two might be entangled.
There was a problem hiding this comment.
Of course, removing AggregateCommits is a good idea. Let me check the surrounding code.
There was a problem hiding this comment.
@rvagg I’ve looked through the code and nothing seems to be blocking.
There’s just one small issue:
if a user really does want to submit only a single ProveCommit (I know that’s unlikely, but it could happen), then with aggregation still forced on they’d have to wait up to 24 hours or manually run publish-now.
Do you think that’s acceptable?
Also, lotus-shed still contains mathProveCommitAggFeesCmd and mathPreCommitAggFeesCmd; I think we could remove those two methods as well, right?
There was a problem hiding this comment.
if a user really does want to submit only a single ProveCommit (I know that’s unlikely, but it could happen), then with aggregation still forced on they’d have to wait up to 24 hours or manually run publish-now.
I´m inclined to say that we are fine with this condition, given that most Lotus-Miner users know the lotus-miner sectors batching commit --publish-now cmd
|
@rvagg could you look over the latest changes here, and see if things look okay so that we can get it merged? |
There was a problem hiding this comment.
Pull Request Overview
This PR removes deprecated batching and fee‐threshold logic introduced for FIP-0100 (nv25), cleaning up related config fields, policy functions, and documentation.
- Consolidates partition declaration limits using the static
policy.DeclarationsMaxconstant. - Removes old precommit/commit aggregation flags (
AggregateCommits,AggregateAboveBaseFee,BatchPreCommitAboveBaseFee) and associated base-fee gating logic. - Cleans up deprecated policy functions (
GetDeclarationsMax,AggregateProveCommitNetworkFee) and prunes unused config/docs entries and test setup.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| storage/wdpost/wdpost_run.go | Replaced dynamic GetDeclarationsMax call with static policy.DeclarationsMax |
| storage/pipeline/terminate_batch.go | Removed obsolete nv25‐related TODO and no-op condition |
| storage/pipeline/precommit_batch.go | Dropped base-fee threshold gating before nv25 |
| storage/pipeline/commit_batch.go | Removed deprecated aggregation flags and fee computation logic |
| node/modules/storageminer.go | Dropped deprecated seal config fields |
| node/config/types.go | Removed deprecated commit/precommit fee config entries |
| node/config/doc_gen.go | Pruned documentation entries for removed config fields |
| node/config/def.go | Adjusted default sealing config by removing deprecated fields |
| itests/sector_pledge_test.go | Removed test setup that mutated deprecated base-fee fields |
| itests/sector_miner_collateral_test.go | Cleaned up unused deprecated config in test |
| itests/direct_data_onboard_test.go | Removed unused AggregateCommits assignment in test |
| documentation/en/default-lotus-miner-config.toml | Deleted commented‐out config entries for deprecated fields |
| cli/spcli/sectors.go | Switched from policy.GetDeclarationsMax to policy.DeclarationsMax |
| chain/actors/policy/policy.go.template & .go | Removed deprecated GetDeclarationsMax and AggregateProveCommitNetworkFee |
Comments suppressed due to low confidence (2)
storage/pipeline/commit_batch.go:228
- [nitpick] This comment references nv21 and aggregation logic that has been removed; please update or remove it so that the doc matches the new batching behavior post-FIP-0100.
// After nv21, we have a new ProveCommitSectors2 method, which supports
cli/spcli/sectors.go:952
- [nitpick] Ensure that
policy.DeclarationsMaxis documented or surfaced in the CLI help text so users understand the new static limit in place of the old dynamic one.
if scount > addrSectors || len(p.Extensions) >= policy.DeclarationsMax {
| // Also respect the AddressedPartitionsMax (which is the same as DeclarationsMax (which is all really just MaxPartitionsPerDeadline)) | ||
| declMax, err := policy.GetDeclarationsMax(nv) | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("getting max declarations: %w", err) | ||
| } | ||
| if partitionsPerMsg > declMax { | ||
| partitionsPerMsg = declMax | ||
| if partitionsPerMsg > policy.DeclarationsMax { | ||
| partitionsPerMsg = policy.DeclarationsMax | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider cleaning up the surrounding whitespace or updating the preceding comment to reflect that nv25-specific logic has been removed, improving readability.
Copilot uses AI. Check for mistakes.
|
@rvagg : repinging in case you have bandwidth now for this. |
Related Issues
Proposed Changes
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that: