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.

Sign up for GitHub

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

Jump to bottom

feat: api: Clean API for Miners #12112

Merged
merged 30 commits into from
Jul 14, 2024
Merged

feat: api: Clean API for Miners #12112

merged 30 commits into from
Jul 14, 2024

Conversation

snadrus
Copy link
Contributor

@snadrus snadrus commented Jun 19, 2024 •
edited
Loading

This is urgently needed to unblock Curio compatibility with NV23, which is blocking Curio from running on CalibrationNet.

Related Issues

To help Lotus have more of a clean API, this PR:

  • Enables access to the latest params without depending on lotus/build (which added bulk and risk to Curio).
  • Extracts buildconstants out of lotus/build to enable clean access to that content without the other "build" items.
  • "Types" packages (API surface) depend on buildconstants instead of lotus/build
  • Rm Curio, including building and testing. (these changes broke it anyway)
  • Deprecated access to those vars through lotus/build, but piped them through to change the least code possible

Review is quite easy: the "161 files changed" is mostly deletions.

Proposed Changes

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
  • 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.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 19, 2024 •
edited
Loading

All checks have completed

❌ Failed Check / Check (lint-all) (pull_request)
❌ Failed Test / Test (itest-eth_legacy_transactions) (pull_request)
❌ Failed Test / Test (itest-harmonydb) (pull_request)

@snadrus snadrus marked this pull request as draft June 19, 2024 15:14
@snadrus snadrus marked this pull request as ready for review June 20, 2024 22:54
@snadrus snadrus changed the title proofparams alternate feat/clean API for miners Jun 20, 2024
@snadrus snadrus requested a review from rvagg June 20, 2024 22:55
rvagg
rvagg previously requested changes Jun 21, 2024
View reviewed changes
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

wew, well that evolved quite a bit since first commit I reviewed!
will need to take some time to get my head around these, but passing tests would be good

@snadrus snadrus mentioned this pull request Jun 22, 2024
Merged
@snadrus
Copy link
Contributor Author

snadrus commented Jun 24, 2024

We have 2 options for cleaning-up build:

  1. move dependency-free build constants out (that API needs). What I did here.
  2. Move big dependencies out of build/

I'm open to either.

@snadrus
Copy link
Contributor Author

snadrus commented Jun 26, 2024

@rvagg please review despite conflicts. The nature of this PR is such that conflicts will occur continuously but have an obvious fix.

@snadrus snadrus requested a review from rvagg June 26, 2024 22:04
@snadrus
Copy link
Contributor Author

snadrus commented Jul 11, 2024 •
edited
Loading

There is no new code in here, only moved build-constants to another package as /build is too large and full of unnecessary pieces for Curio.
This will experience modifications continuously, so there's no point in me fixing it until we can commit to the intent.

@snadrus snadrus added the skip/changelog This change does not require CHANGELOG.md update label Jul 11, 2024
@rvagg
Copy link
Member

rvagg commented Jul 12, 2024

Looks good to me now. This does warrant a changelog entry though, particularly the deprecations but it'll be relevant to anyone else wanting to consume some of these without pulling in all the garbage you're now able to avoid.
I'd also appreciate another set of more experienced eyes. @ZenGround0 ?

@rvagg rvagg requested a review from ZenGround0 July 12, 2024 04:44
@rjan90 rjan90 changed the title feat/clean API for miners feat: api: Clean API for Miners Jul 12, 2024
ZenGround0
ZenGround0 reviewed Jul 12, 2024
View reviewed changes
build/buildconstants/params_mainnet.go Show resolved Hide resolved
build/buildconstants/params_shared_vals.go Outdated Show resolved Hide resolved
build/buildconstants/params_shared_vals.go Outdated Show resolved Hide resolved
itests/kit/ensemble.go Outdated Show resolved Hide resolved
build/version.go Outdated Show resolved Hide resolved
ZenGround0
ZenGround0 approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@ZenGround0 ZenGround0 left a comment

Choose a reason for hiding this comment

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

Not sure what's going on with this test flake or something real: https://github.com/filecoin-project/lotus/actions/runs/9911095972/job/27383006018?pr=12112. Once tests pass I'm ok with this.

@snadrus snadrus dismissed rvagg’s stale review July 14, 2024 14:05

not actionable

@snadrus snadrus merged commit 21abfc6 into master Jul 14, 2024
79 checks passed
@snadrus snadrus deleted the curioAvoidBuildFolder branch July 14, 2024 14:06
@ribasushi ribasushi mentioned this pull request Jul 16, 2024
Merged
@rjan90
Copy link
Contributor

rjan90 commented Jul 16, 2024

As discussed in Slack, I will remove the release/backport label, because we will not be backporting this PR to the v1.28.0 train. Instead we will follow up shipping these changes in a subsequent release fairly quick after the stable v1.28.0 is out on July 23rd.

@ribasushi ribasushi mentioned this pull request Jul 16, 2024
Merged
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request Jul 17, 2024
@snadrus @ribasushi
feat: api: Clean API for Miners (filecoin-project#12112)
b34c514 
* proofparams alternate

* createminer

* const factored from /build and types updated to use it

* buildconstants for more places

* deprecate msg

* itest cleanup

* alerting interface

* house cleaning

* rm policy and drand from buildconstants

* clean up curio further

* aussie waffle

* pr fixes

* fix lints

* little fixes

* oops this got updated

* unbreak test builds

* test fixes

* comments - cleanups

* itests fix alerting

* rm obsolete alertinginterface

* spelling oops

* changelog

* tests need buildconstants port

* Fully migrate BlockGasTarget

* ulimit should not depend on build

* complete the simplest deprecations

* bringing back versions
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request Jul 17, 2024
@snadrus @ribasushi
feat: api: Clean API for Miners (filecoin-project#12112)
037905c 
* proofparams alternate

* createminer

* const factored from /build and types updated to use it

* buildconstants for more places

* deprecate msg

* itest cleanup

* alerting interface

* house cleaning

* rm policy and drand from buildconstants

* clean up curio further

* aussie waffle

* pr fixes

* fix lints

* little fixes

* oops this got updated

* unbreak test builds

* test fixes

* comments - cleanups

* itests fix alerting

* rm obsolete alertinginterface

* spelling oops

* changelog

* tests need buildconstants port

* Fully migrate BlockGasTarget

* ulimit should not depend on build

* complete the simplest deprecations

* bringing back versions
@rjan90 rjan90 mentioned this pull request Jul 23, 2024
Merged
8 tasks
@rvagg rvagg mentioned this pull request Jul 24, 2024
Merged
8 tasks
ribasushi pushed a commit to ribasushi/ci-abusing-lotus-fork that referenced this pull request Jul 24, 2024
@snadrus @ribasushi
feat: api: Clean API for Miners (filecoin-project#12112)
95ed532 
* proofparams alternate

* createminer

* const factored from /build and types updated to use it

* buildconstants for more places

* deprecate msg

* itest cleanup

* alerting interface

* house cleaning

* rm policy and drand from buildconstants

* clean up curio further

* aussie waffle

* pr fixes

* fix lints

* little fixes

* oops this got updated

* unbreak test builds

* test fixes

* comments - cleanups

* itests fix alerting

* rm obsolete alertinginterface

* spelling oops

* changelog

* tests need buildconstants port

* Fully migrate BlockGasTarget

* ulimit should not depend on build

* complete the simplest deprecations

* bringing back versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvagg rvagg rvagg left review comments

@ZenGround0 ZenGround0 ZenGround0 approved these changes

Assignees
No one assigned
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@snadrus @rvagg @rjan90 @ZenGround0 @LexLuthr