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

[RPC][Mining] Add sanity checks to generatecontinuous #879

Merged
merged 4 commits into from
Dec 10, 2020

Conversation

CaveSpectre11
Copy link
Collaborator

Note

This is built on top of #878 and requires code added in #877 and #878

Problem

generatecontinuous can get users in trouble by not knowing what their settings will do to their resources

Solution

Add checks in generatecontinuous:

  • Warn if RandomX is set to less than 4 threads.
  • Warn if Sha256D is more than one thread less than the number of cores
  • Provide ability to override the warnings with a new optional parameter
  • Disallow generatecontinuous true if already running
  • Rework the result of the command to provide a JSON status

Testing

$ veil-cli generatecontinuous true 1
{
 "success": true,
 "algorithm": "sha256d",
 "threads": 1,
 "message": "Mining started"
}
$ veil-cli generatecontinuous true 1
error code: -32603
error message:
Mining already active
$ veil-cli generatecontinuous false
{
 "success": true,
 "algorithm": "sha256d",
 "threads": 0,
 "message": "Mining stopped"
}
$ ./veil-cli setminingalgo randomx
{
  "success": true,
  "message": "Mining algorithm changed from sha256d to randomx"
}
$ veil-cli generatecontinuous true 1
error code: -8
error message:
Error: RandomX must be at least 4 threads
$ veil-cli setminingalgo sha256d
{
  "success": true,
  "message": "Mining algorithm changed from randomx to sha256d"
}
$ veil-cli generatecontinuous true 100
error code: -8
error message:
Error: Available cores: 4, limit sha256d to 3 threads
$ ./veil-cli generatecontinuous true 4 true
{
  "success": true,
  "algorithm": "sha256d",
  "threads": 4,
  "message": "Warning: Available cores: 4, limit sha256d to 3 threads

@CaveSpectre11 CaveSpectre11 added Component: RPC Related to the console commands themselves. Component: Miner Both PoW and PoS block creation Tag: Needs Release Notes Any PR or Issue that needs notable mention in release notes Tag: Waiting For Code Review Waiting for code review from a core developer labels Dec 6, 2020
@CaveSpectre11 CaveSpectre11 self-assigned this Dec 6, 2020
Copy link
Collaborator

@codeofalltrades codeofalltrades left a comment

Choose a reason for hiding this comment

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

ACK b654113

@codeofalltrades codeofalltrades added Code Review: Passed Tag: Waiting For QA A pull review is waiting for QA to test the pull request and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Dec 6, 2020
@seanPhill
Copy link
Collaborator

seanPhill commented Dec 6, 2020

All this works on mine, but I noticed when setting too many SHA mining threads that I got a reported number of cores double the number of actual cores.
generatecontinuous true 80
Error: Available cores: 12, limit sha256d to 11 threads (code -8)
Screen Shot 2020-12-07 at 8 24 26 am
I never set my SHA mining over 4, and sometimes when leaving the wallet mining for several days I come back and find it stuck, where stopping mining allows it to immediately catch up. In the above testing I was able to accidentally set an excessive RandomX number of cores, but it didn't freeze and I was able to simply set it again to a sane number.

@CaveSpectre11
Copy link
Collaborator Author

Error: Available cores: 12, limit sha256d to 11 threads (code -8)

This is going to take some digging, and probably outside the scope of the PR. We'll need to figure out why std::thread::hardware_concurrency(); returns the wrong number of cores for Mac (which may be challenging without a mac to develop on)

@seanPhill
Copy link
Collaborator

Just for comparison I tested it on a quad core i7 iMac with MacOS Catalina and got the same result.
Screen Shot 2020-12-07 at 10 29 20 am

@CaveSpectre11
Copy link
Collaborator Author

Little bit of research shows that hardware_concurrency returns the number of concurrent threads the system has; and can often be 2x the number of cores; but it's OS specific.

So I might need to tweak the wording here a little bit.

Copy link
Collaborator

@codeofalltrades codeofalltrades left a comment

Choose a reason for hiding this comment

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

ACK 6cc4d59

@codeofalltrades codeofalltrades merged commit 0697885 into Veil-Project:master Dec 10, 2020
@CaveSpectre11 CaveSpectre11 added Dev Status: Merged Issue is completely finished. QA: Passed This has passed QA testing and can be merged to master and removed Tag: Waiting For QA A pull review is waiting for QA to test the pull request labels Dec 11, 2020
@CaveSpectre11 CaveSpectre11 deleted the checkcpu branch December 11, 2020 14:22
@CaveSpectre11 CaveSpectre11 added this to the v1.1.1 milestone Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Review: Passed Component: Miner Both PoW and PoS block creation Component: RPC Related to the console commands themselves. Dev Status: Merged Issue is completely finished. QA: Passed This has passed QA testing and can be merged to master Tag: Needs Release Notes Any PR or Issue that needs notable mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants