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

feat(remove): Temporally remove the internal miner functionality #8184

Merged
merged 6 commits into from
Jan 23, 2024

Conversation

oxarbitrage
Copy link
Contributor

Motivation

Close #8180

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?

Solution

  • Removed the least amount of code to get rid of the custom equihash dependency.
  • Commented the changes as TODOs in the code.
  • Created issue to restore.
  • Fixed config files needed to make the tests pass.

Review

Anyone can review, i marked as critical as we are in a merge freeze and it will not get merged otherwise.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

Create a hotfix including this removal of feature in the changelog. #8181

@oxarbitrage oxarbitrage added A-release Area: Zebra releases and release management P-Critical 🚑 labels Jan 22, 2024
@oxarbitrage oxarbitrage requested a review from a team as a code owner January 22, 2024 23:07
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team January 22, 2024 23:07
@github-actions github-actions bot added C-feature Category: New features extra-reviews This PR needs at least 2 reviews to merge labels Jan 22, 2024
arya2
arya2 previously approved these changes Jan 23, 2024
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This looks good, I think we should skip adding the test config until we resolve this TODO if config_tests passes without it.

zebra-chain/Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Arya <aryasolhi@gmail.com>
@oxarbitrage
Copy link
Contributor Author

test config changes were done because config test were NOT passing without them.

@oxarbitrage
Copy link
Contributor Author

@oxarbitrage oxarbitrage requested a review from a team as a code owner January 23, 2024 13:48
@oxarbitrage
Copy link
Contributor Author

This looks good, I think we should skip adding the test config until we resolve this TODO if config_tests passes without it.

You are right, there are more removals needed to make the tests pass with different feature combinations. I pushed a commit to check against CI (094e3fe)

@oxarbitrage oxarbitrage removed the extra-reviews This PR needs at least 2 reviews to merge label Jan 23, 2024
@mergify mergify bot merged commit d45864f into main Jan 23, 2024
129 checks passed
@mergify mergify bot deleted the issue8180 branch January 23, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-release Area: Zebra releases and release management C-feature Category: New features P-Critical 🚑
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove or comment out zebra internal miner
2 participants