Fix sv2 cpu miner hash rate calculation

[Fi3]

In order to calculte the hash rate of the machine in which is running the sv2 cpu miner was using a fster function that the one that use to actually mine. This commit change the function that the cpu miner use to calculte the hash rate using the same one that is used when it mine.

[Sjors]

Instead of duplicating the code, can you add a new helper function? That way future improvements to the mining code don't cause the same bug.

[Fi3]

@Sjors can you be more specific about the helper function? Where do you thing that should be added?

[Sjors]

I don't understand the code organisation well enough.

using the same one that is used when it mine

At first glance it looks to me that we now have two functions that are copy-pasted, rather than one function that's called from two places. But maybe I missed something.

[Fi3]

In measure_hashrate (the function modified by this code). We have:

• Initialize an arbitrary block header (most of the body)
• Call miner.next() for a duration (this incr the nonce and do the dsha)
• return number of hashes calculated / duration

Each of the above 3 part are very specific of this function, one could thing that the first part can be de-duplicated, but in the function we create an header from some random vectors, in other parts of the code we compose them from SetPrevhash + NewJob messages. I don't think that could make much sense to have a function that do both things. Also would be very strange to create 2 messages SetPrevhash + NewJob in the measure_hashrate function only to derive an arbitrary block header when we could just create it right away and put it in the miner.

Not sure if this clarify your doubts? Or if I even answered your questions? Please let me know if your comment was about it or not,

[Sjors]

Alright, let's keep it this way then. Is there something that needs testing, or does it just need code review? Maybe @plebhash & @lorbax can take a look?

[Fi3]

Code review will be enough

[Fi3]

This PR needs to be rebased on dev

false alarm, I didn't really find a bug

[plebhash]

why are we setting the target as 0?

doesn't this make it basically impossible to find a share?

difficulty = genesis_target / target = genesis_target / 0 = ∞

we are bypassing this restriction because we are ignoring the error returned by miner.next_share(), but that makes the code more confusing and counter-intuitive than it needs to be

but in order to improve readability and make a more clear usage of the API provided by miner.next_share(), I would refactor this snippet into something like:

Suggested change

	let mut miner = Miner::new(0);
	miner.new_target(vec![0_u8; 32]);
	miner.header = Some(header);
	while start_time.elapsed() < duration {
	let _ = miner.next_share();
	hashes += 1;
	}
	// set the max possible target, so that every share is valid
	miner.new_target(vec![255_u8; 32]);
	miner.header = Some(header);
	while start_time.elapsed() < duration {
	miner.next_share().expect("every share is valid under max_target");
	hashes += 1;
	}

the downside for this approach is that now next_share will spam the console:

2024-05-03T20:59:11.630062Z  INFO mining_device: Found share with nonce: 0, for target: Some(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff), with hash: 0x03986db2375a8106218561993e202c09b8ed969157417664ccba54c1ef8c5e6e
2024-05-03T20:59:11.630065Z  INFO mining_device: Found share with nonce: 0, for target: Some(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff), with hash: 0x03986db2375a8106218561993e202c09b8ed969157417664ccba54c1ef8c5e6e
2024-05-03T20:59:11.630082Z  INFO mining_device: Found share with nonce: 0, for target: Some(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff), with hash: 0x03986db2375a8106218561993e202c09b8ed969157417664ccba54c1ef8c5e6e
...

so next_share could probably have a filter for the line that prints this:

-        if hash < *self.target.as_ref().ok_or(())? {
-            info!(
-                "Found share with nonce: {}, for target: {:?}, with hash: {:?}",
-                header.nonce, self.target, hash,
-            );
+        let target = *self.target.as_ref().ok_or(())?;
+        let max_target_hashrate_measurement =
+            Uint256::from_be_bytes(vec![255_u8; 32].try_into().unwrap());
+        if hash < target {
+            if target < max_target_hashrate_measurement {
+                info!(
+                    "Found share with nonce: {}, for target: {:?}, with hash: {:?}",
+                    header.nonce, self.target, hash,
+                );
+            }

[Fi3]

we don't car if share are valid or not we just want to execute the dsha function in the time interval and count how many time we are doing that so we know the hashrate of the machine that is running this cpuminer.

[Fi3]

the majior issue with the above code is that next_share return an Result (for sake of simplicity) instead of a custom type like enum ValidShare | InvalidShare

[Fi3]

i'm going do push something next week

[plebhash]

ok

it took me some time but I ended up understanding everything is going on

the suggested change is just a small cosmetic change to improve code readability, but not essential

feel free to ignore this if you wish

[plebhash]

I made some suggestions for minor improvements, but they're not essential. I'll approve and leave it up to @Fi3 to decide if he wants to merge it right away or implement those improvements before doing so.

[Fi3]

I made some suggestions for minor improvements, but they're not essential. I'll approve and leave it up to @Fi3 to decide if he wants to merge it right away or implement those improvements before doing so.

your suggestions should be already addressed in last commit