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

Backport: feat: sealing: Switch to calling PreCommitSectorBatch2 #11215

Merged
merged 9 commits into from
Aug 29, 2023

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Aug 29, 2023

Related Issues

This PR backports #11142 to feat/nv21

Related to #11125

Proposed Changes

Additional Info

This is required because nv21 will remove the old Precommit methods. DDO also requires the use of the new methods, so any integration work must be on top of this code

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, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@magik6k magik6k requested a review from a team as a code owner August 29, 2023 11:49
@magik6k magik6k changed the title Backport/nv21/feat/act precommv2 Backport: feat: sealing: Switch to calling PreCommitSectorBatch2 Aug 29, 2023
requires:
- build
suite: itest-ccupgrade
target: "./itests/ccupgrade_test.go"
Copy link
Member

Choose a reason for hiding this comment

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

a bit surprised t see t tests being removed 👁️

Copy link
Contributor Author

@magik6k magik6k Aug 29, 2023

Choose a reason for hiding this comment

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

This is the extremely old replace-sector-with-another-sector CC upgrade, not snapdeals.

It's not even supported in actors anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

Woah

Copy link
Member

Choose a reason for hiding this comment

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

Oh that right thanks

@@ -107,7 +108,10 @@ func (mgr *SectorCommittedManager) OnDealSectorPreCommitted(ctx context.Context,

// Watch for a pre-commit message to the provider.
matchEvent := func(msg *types.Message) (bool, error) {
matched := msg.To == provider && (msg.Method == builtin.MethodsMiner.PreCommitSector || msg.Method == builtin.MethodsMiner.PreCommitSectorBatch || msg.Method == builtin.MethodsMiner.ProveReplicaUpdates)
matched := msg.To == provider && (msg.Method == builtin.MethodsMiner.PreCommitSector ||
msg.Method == builtin.MethodsMiner.PreCommitSectorBatch ||
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity - any reason why we wanna keep PrecommitsectorBatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sectors still can be onboarded with the old precommit, and this helper may still be pulled by e.g. by boost, so we probably want to keep support for the old method at least until the next network version.

I don't think there's much harm in keeping old stuff here, the whole markets/ package will go anyways as we deprecate legacy markets.

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.

3 participants