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

mining-device cpu miner does not hit the target #1188

Closed
jbesraa opened this issue Oct 7, 2024 · 15 comments · Fixed by #1195
Closed

mining-device cpu miner does not hit the target #1188

jbesraa opened this issue Oct 7, 2024 · 15 comments · Fixed by #1195

Comments

@jbesraa
Copy link
Contributor

jbesraa commented Oct 7, 2024

When running the mining-device locally I was not able to hit the target set here https://github.com/stratum-mining/stratum/blob/main/roles/test-utils/mining-device/src/lib/mod.rs#L201

The test was not run for too long, probably for a couple of minutes. While in practice that might not be a problem, in test environment we want shares to be submitted in a matter of seconds in order to not run the test for too long.

@GitGab19
Copy link
Collaborator

GitGab19 commented Oct 7, 2024

I think the best way to manage this would be something like this:
Screenshot 2024-10-07 at 17 38 33
We can take the divisor as an optional argument passed in the command to run the miner. In this way you can decide how much easier your jobs will be.

@GitGab19
Copy link
Collaborator

GitGab19 commented Oct 7, 2024

Also, I noticed we are multiplying the target with the number of threads here:

let p = std::thread::available_parallelism().unwrap().get() as u32 - 3;
let nominal_hash_rate = measure_hashrate(5) as f32 * p as f32;

I don't think it's correct (cc @Fi3).

If you agree on both removing this, and adding the parameter I was mentioning, I can open a PR to fix this, so unblocks the integration test @jbesraa is writing

@Fi3
Copy link
Collaborator

Fi3 commented Oct 7, 2024

td::thread::available_parallelism()

it is the number cores, I think that the above code is correct

@GitGab19
Copy link
Collaborator

GitGab19 commented Oct 7, 2024

Yes, but the measure_hashrate function computes the hashrate of the entire machine, how does it relate to the number of cores?

@Fi3
Copy link
Collaborator

Fi3 commented Oct 7, 2024

it compute it in a single core. Then when we hash we use all the available

@Fi3
Copy link
Collaborator

Fi3 commented Oct 7, 2024

at the beginning wasn't parallel then sjors asked to add it cause sometimes (for specific use cases that I do not remember) is good to have a little bit more power

@GitGab19
Copy link
Collaborator

GitGab19 commented Oct 7, 2024

I see, but I still don't get why there's the -3 into the p computation.
However, my proposal to add an extra optional argument (the divisor) still remains.

@plebhash
Copy link
Collaborator

plebhash commented Oct 8, 2024

porting over from #1066 (comment) so it doesn't get lost

once we fix this issue with mining-device, we should have the following integration test:

  • start TP
  • start Pool (connected to TP)
  • start SV2 CPU Miner (connected to Pool)

we can assert that:

  • Pool connects to TP and receives template (basically what's already being done on success_pool_template_provider_connection)
  • SV2 CPU Miner connects to Pool (SetupConnection / SetupConnection.Success)
  • SV2 CPU Miner opens standard channel with Pool (OpenStandardMiningChannel / OpenStandardMiningChannel.Success)
  • Pool sends Standard Job to SV2 CPU Miner (NewMiningJob)
  • Pool sends New PrevHash to SV2 CPU Miner (SetNewPrevHash)
  • SV2 CPU Miner sends share to Pool (SubmitSharesStandard)
  • Pool confirms receiving share (SubmitShares.Success)

we can assert things like channel ids and job ids

@GitGab19
Copy link
Collaborator

GitGab19 commented Oct 8, 2024

I think the best way to manage this would be something like this: Screenshot 2024-10-07 at 17 38 33 We can take the divisor as an optional argument passed in the command to run the miner. In this way you can decide how much easier your jobs will be.

@Fi3 @jbesraa any feedback on this?

@plebhash
Copy link
Collaborator

plebhash commented Oct 8, 2024

@Fi3 one question that remains unanswered:

let p = std::thread::available_parallelism().unwrap().get() as u32 - 3;

why are we substracting 3 here?

@plebhash
Copy link
Collaborator

plebhash commented Oct 8, 2024

I think the best way to manage this would be something like this: Screenshot 2024-10-07 at 17 38 33 We can take the divisor as an optional argument passed in the command to run the miner. In this way you can decide how much easier your jobs will be.

@Fi3 @jbesraa any feedback on this?

maybe a better approach than --divisor CLI arg, we could add a --nominal-hashrate-multiplier, where we take a f32

  • if 0 < nominal_hashrate_multiplier < 1, we are essentially advertising a nominal hashrate that is smaller than the real hashrate
  • if nominal_hashrate_multiplier > 1, we are essentially advertising a nominal hashrate that is bigger than the real hashrate

this would be an optional CLI arg, and when left empty, the SV2 CPU miner would simply advertise the result of measure_hashrate

@plebhash
Copy link
Collaborator

plebhash commented Oct 8, 2024

I noticed that measure_hashrate is not taking handicap into consideration

that is a problem (independently of the discussion we're having here)

if handicap bigger than 0, then measure_hashrate must take that into consideration, otherwise it will report something that is not accurate with the real hashrate that the CPU miner is able to deliver

@GitGab19
Copy link
Collaborator

GitGab19 commented Oct 8, 2024

I don't think it's the reason behind handicap. Here's the description from the README: "This value is used to slow down the cpu miner, it represents the number of micro-seconds that are awaited between hashes [default: 0]".
I still don't know the reason why it was introduced, but it seems to be just a way to slow down the mining-device

@plebhash
Copy link
Collaborator

plebhash commented Oct 8, 2024

I don't think it's the reason behind handicap. Here's the description from the README: "This value is used to slow down the cpu miner, it represents the number of micro-seconds that are awaited between hashes [default: 0]". I still don't know the reason why it was introduced, but it seems to be just a way to slow down the mining-device

if handicap is 0, the CPU miner uses the maximum available hashpower on the system

if handicap is bigger than 0, the CPU miner uses less than the max hashpower

so it feels reasonable that handicap is reflected on measure_hashrate, because it has a direct impact in the CPU miner hashpower

@Fi3
Copy link
Collaborator

Fi3 commented Oct 9, 2024

@Fi3 one question that remains unanswered:

let p = std::thread::available_parallelism().unwrap().get() as u32 - 3;

why are we substracting 3 here?

I guess for not overwhelm the cpu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants