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(panic): Log a warning instead of panicking for unused mining configs #7290

Merged
merged 7 commits into from Aug 29, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jul 25, 2023

Motivation

This PR will log a warning if there is a non-default mining section in the loaded config when compiled without the "getblocktemplate-rpcs" feature, instead of panicking.

Part of #7118.

Solution

  • Move the getblocktemplate config to config/mining.rs
  • Log a warning when there's a mining section in a config without the getblocktemplate-rpcs feature

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@arya2 arya2 requested a review from a team as a code owner July 25, 2023 23:10
@arya2 arya2 requested review from dconnolly and removed request for a team July 25, 2023 23:10
@arya2 arya2 self-assigned this Jul 25, 2023
@arya2 arya2 added C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup I-panic Zebra panics with an internal error message I-usability Zebra is hard to understand or use P-Optional ✨ labels Jul 25, 2023
@teor2345 teor2345 added the no-review-reminders Turn off review reminders label Jul 31, 2023
@arya2 arya2 marked this pull request as draft August 8, 2023 20:15
@arya2 arya2 removed the request for review from dconnolly August 16, 2023 16:16
@oxarbitrage
Copy link
Contributor

@arya2 do you need someone to finish this work ? I can find some time to bump it up if you want.

@arya2
Copy link
Contributor Author

arya2 commented Aug 19, 2023

@arya2 do you need someone to finish this work ? I can find some time to bump it up if you want.

Yes, please.

@oxarbitrage oxarbitrage self-assigned this Aug 19, 2023
@teor2345
Copy link
Contributor

If we make getblocktemplate-rpcs into a default feature, this might change priority.

@oxarbitrage oxarbitrage marked this pull request as ready for review August 25, 2023 18:30
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good, let's get it merged before any more RPC work.

I committed a quick tweak to the mining config argument passing, because we don't need to pass () now the confit is always enabled.

@teor2345 teor2345 removed the no-review-reminders Turn off review reminders label Aug 29, 2023
mergify bot added a commit that referenced this pull request Aug 29, 2023
mergify bot added a commit that referenced this pull request Aug 29, 2023
@mergify mergify bot merged commit 2b81d84 into main Aug 29, 2023
290 checks passed
@mergify mergify bot deleted the unused-config-warnings branch August 29, 2023 07:45
@oxarbitrage oxarbitrage mentioned this pull request Sep 1, 2023
40 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement I-panic Zebra panics with an internal error message I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants