Skip to content

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

Merged
codeofalltrades merged 4 commits intoVeil-Project:masterfrom
CaveSpectre11:checkcpu
Dec 10, 2020
Merged

[RPC][Mining] Add sanity checks to generatecontinuous#879
codeofalltrades merged 4 commits intoVeil-Project:masterfrom
CaveSpectre11:checkcpu

Conversation

@CaveSpectre11
Copy link
Collaborator

@CaveSpectre11 CaveSpectre11 commented Dec 6, 2020

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
codeofalltrades
codeofalltrades approved these changes Dec 6, 2020
View reviewed changes
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 •
edited
Loading

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

CaveSpectre11 commented Dec 6, 2020

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

seanPhill commented Dec 6, 2020

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

CaveSpectre11 commented Dec 6, 2020

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.

@seanPhill seanPhill mentioned this pull request Dec 7, 2020
Merged
codeofalltrades
codeofalltrades approved these changes Dec 8, 2020
View reviewed changes
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
@CaveSpectre11 CaveSpectre11 mentioned this pull request May 2, 2021
Closed
@CaveSpectre11 CaveSpectre11 mentioned this pull request Oct 3, 2021
Merged
@seanPhill seanPhill mentioned this pull request May 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codeofalltrades codeofalltrades codeofalltrades approved these changes

Assignees

@CaveSpectre11 CaveSpectre11

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

Milestone

v1.1.1

Development

Successfully merging this pull request may close these issues.

3 participants

@CaveSpectre11 @seanPhill @codeofalltrades