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

fix: lotus-miner: remove provecommit1 method #12251

Merged
merged 8 commits into from
Jul 24, 2024
Merged

fix: lotus-miner: remove provecommit1 method #12251

merged 8 commits into from
Jul 24, 2024

Conversation

LexLuthr
Copy link
Contributor

@LexLuthr LexLuthr commented Jul 17, 2024

Related Issues

Proposed Changes

This PR drops the support for ProveCommit1.

Additional Info

Checklist

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

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • Update CHANGELOG.md or signal that this change does not need it.
    • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
    • If the change does not require a CHANGELOG.md entry, do one of the following:
      • Add [skip changelog] to the PR title
      • Add the label skip/changelog to the PR
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@LexLuthr LexLuthr requested a review from magik6k July 17, 2024 15:23
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Pretty sure we need to call processPieces with the last arg set to true to convert dealid sectors to ddo-style -

pams, dealIDs, err := m.processPieces(ctx.Context(), sector, false)

Also pretty sure we need to convert all precommits to ddo-style, otherwise deals in f05 won't get activated

@LexLuthr LexLuthr requested a review from magik6k July 22, 2024 12:51
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Don't see anything wrong here, looks good!

@rjan90 rjan90 enabled auto-merge (squash) July 24, 2024 17:02
@rjan90 rjan90 merged commit 2ad743a into master Jul 24, 2024
80 checks passed
@rjan90 rjan90 deleted the miner/remove-pcm1 branch July 24, 2024 17:02
jennijuju pushed a commit that referenced this pull request Jul 25, 2024
* remove provecommit1

* add changelog

* update precommit and commit params

* fix lint error

* fix commit params
jennijuju pushed a commit that referenced this pull request Jul 25, 2024
* remove provecommit1

* add changelog

* update precommit and commit params

* fix lint error

* fix commit params
@rvagg
Copy link
Member

rvagg commented Jul 25, 2024

Continuing the discussion above (#12251 (comment)) about how we invoke this removed code path post nv22. (Permalinks below point to v1.28.0)

In

if !individual && !cfg.AggregateAboveBaseFee.Equals(big.Zero()) {
if ts.MinTicketBlock().ParentBaseFee.LessThan(cfg.AggregateAboveBaseFee) {
individual = true
}
}
we have the basefee check, where if the basefee is below the batch balancer level we individual = true.

Since nv22, it always goes into this block, regardless of what individual is, and any sectors that return from processBatchV2() having been processed are removed from the todo list:

if nv >= MinDDONetworkVersion {
// After nv21, we have a new ProveCommitSectors2 method, which supports
// batching without aggregation, but it doesn't support onboarding
// sectors which were precommitted with DealIDs in the precommit message.
// We prefer it for all other sectors, so first we use the new processBatchV2
var sectors []abi.SectorNumber
for sn := range b.todo {
sectors = append(sectors, sn)
}
res, err = b.processBatchV2(cfg, sectors, nv, !individual)
if err != nil {
err = xerrors.Errorf("processBatchV2: %w", err)
}
// Mark sectors as done
for _, r := range res {
if err != nil {
r.Error = err.Error()
}
for _, sn := range r.Sectors {
for _, ch := range b.waiting[sn] {
ch <- r // buffered
}
delete(b.waiting, sn)
delete(b.todo, sn)
delete(b.cutoffs, sn)
}
}
}

When we get to processIndividually(), we end up without anything in todo so don't process anything:

if individual {
resV1, err = b.processIndividually(cfg)
} else {
var sectors []abi.SectorNumber

But the catch is in processBatchV2(), where we have a check for deal IDs and we don't process those sectors:

if b.todo[sector].DealIDPrecommit {
// can't process sectors precommitted with deal IDs with ProveCommitSectors2
continue
}

So the effect of this is that CC sectors can go in via PCS3, but deal sectors go in via PCS.

There's additional changes in here that relate to deal IDs as they pertain to inspecting pieces. In particular this code inspects pieces, looking for non-filler pieces and always opts out of DDO (false). It's only true when it's used for ReplicaUpdate. Either way we get a list of deal IDs back for non-CC sectors, which forces us into the PCS onboarding state as long as the basefee condition holds:

pams, dealIDs, err := m.processPieces(ctx.Context(), sector, false)
if err != nil {
return err
}
res, err := m.commiter.AddCommit(ctx.Context(), sector, AggregateInput{
Info: proof.AggregateSealVerifyInfo{
Number: sector.SectorNumber,
Randomness: sector.TicketValue,
InteractiveRandomness: sector.SeedValue,
SealedCID: *sector.CommR,
UnsealedCID: *sector.CommD,
},
Proof: sector.Proof,
Spt: sector.SectorType,
ActivationManifest: miner2.SectorActivationManifest{
SectorNumber: sector.SectorNumber,
Pieces: pams,
},
DealIDPrecommit: len(dealIDs) > 0,
})

@jennijuju
Copy link
Member

Thanks! would've been great if its in PR description in the first place

jennijuju pushed a commit that referenced this pull request Jul 25, 2024
* remove provecommit1

* add changelog

* update precommit and commit params

* fix lint error

* fix commit params
jennijuju pushed a commit that referenced this pull request Jul 25, 2024
* remove provecommit1

* add changelog

* update precommit and commit params

* fix lint error

* fix commit params
@TippyFlitsUK
Copy link
Contributor

Started sealing a non-DDO deal on mainnet 1.27.2 to WaitSeed stage. Upgraded to 1.28.1 and the sector fails to finalize as expected.

12251 Test.txt

rjan90 added a commit that referenced this pull request Aug 19, 2024
…lease) (#12400)

* fix: lotus-miner: remove provecommit1 method (#12251)

* remove provecommit1

* add changelog

* update precommit and commit params

* fix lint error

* fix commit params

* dep: f3: Update go-f3 to 0.0.6, enable it on mainnet (#12295)

* Update go-f3 to 0.0.6

Signed-off-by: Jakub Sztandera <[email protected]>

* Enable F3 in passive configuration in mainnet config

Signed-off-by: Jakub Sztandera <[email protected]>

* Add changelog

Signed-off-by: Jakub Sztandera <[email protected]>

* add new butterfly assets

---------

Signed-off-by: Jakub Sztandera <[email protected]>
Co-authored-by: Jennifer Wang <[email protected]>

* retract v1.28.0

* update v1.28.0 changelog and add v1.28.1

* Update CHANGELOG.md

* wip - update f3

* don't convert bigint type

We now use the same one in GPBFT.

* update docs

* fix wrong param name

* update butterfy assets

* update go-f3

* update changelog

* update version

* fix typo

* Update CHANGELOG.md

Co-authored-by: Steven Allen <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Rod Vagg <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Rod Vagg <[email protected]>

* apply f3 patch

* chore: bump versions and make gen/docsgen-cli

chore: bump versions and make gen/docsgen-cli

* chore: update v1.28.2 changelog

chore: update v1.282. changelog

* feat: f3: update go-f3 to 0.2.0 (#12390)

* Update go-f3 to 0.2.0

Includes:
 - fix for excessive bandwidth usage
 - significant performance improvements
 - minor consensus fixes

Signed-off-by: Jakub Sztandera <[email protected]>

* add changelog

Signed-off-by: Jakub Sztandera <[email protected]>

* chore(f3): update to final released version

---------

Signed-off-by: Jakub Sztandera <[email protected]>
Co-authored-by: Steven Allen <[email protected]>

* fix!: sealer: handle initialisation error without panic

storage/pipeline.NewPreCommitBatcher and storage/pipeline.New now have an additional
error return to deal with errors arising from fetching the sealing config.

* add breaking API upgrade warning to the ChangeLog

* NewCommitBatcher now has an additional
error return to deal with errors arising from fetching the sealing config.

* fix: miner: Fix DDO pledge math (#12341)

* Power is units of Space * Time so multiply by deal duration

* fix: miner: Fix DDO pledge math

* appease the changelog checker

* Fix gen

---------

Co-authored-by: zenground0 <[email protected]>

* chore: fix lint error

- Updated the logging statement in `testOutOfGasError` to correctly reference `build.BlockGasLimit` instead of `buildconstants.BlockGasLimit`.

* fix: update changelog to reference bandwidth issue ticket

fix: update changelog to reference bandwidth issue ticket

* Update CHANGELOG.md

Co-authored-by: Steve Loeppky <[email protected]>

* Update CHANGELOG.md

* chore: make gen and make docsgen-cli

Run `make gen` and `make docsgen-cli`

---------

Signed-off-by: Jakub Sztandera <[email protected]>
Co-authored-by: LexLuthr <[email protected]>
Co-authored-by: Jakub Sztandera <[email protected]>
Co-authored-by: Jennifer Wang <[email protected]>
Co-authored-by: Jiaying Wang <[email protected]>
Co-authored-by: Steven Allen <[email protected]>
Co-authored-by: Rod Vagg <[email protected]>
Co-authored-by: aarshkshah1992 <[email protected]>
Co-authored-by: Łukasz Magiera <[email protected]>
Co-authored-by: zenground0 <[email protected]>
Co-authored-by: Steve Loeppky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants