Adding the actual mining process to the recipes (basic-pow, hybrid-consensus) #433
Conversation
Add the actual mining process.
Add the actual mining process.
- public "hash_meets_difficulty" function. - reduce difficulty due to the use of u8 type as nonce for pow mining.
There was a problem hiding this comment.
@Mhnd3 Thanks for the fix on making basic-pow and hybrid-consensus node start producing blocks again.
@JoshOrndorff @Mhnd3 there are some questions I am not sure about in the PoW code, raised in the code inline. Could you shed me some light here? Thanks.
nodes/basic-pow/src/service.rs
Outdated
| let worker = _worker.clone(); | ||
| let metadata = worker.lock().metadata(); | ||
| if let Some(metadata) = metadata { | ||
| let nonce = H256::from_slice(Sha3_256::digest(&[numb]).as_slice()); |
There was a problem hiding this comment.
Actually I am not familiar with writing consensus, so I am also learning here. A few questions here:
Why is nonce a hash, instead of simply an incrementing number? I see that numb pretty much act as an incrementing nonce. But then this number should probably be of type U256 instead of u8.
There was a problem hiding this comment.
Thanks @jimmychu0807 for the review!
The nonce as a Hash is mimicking the Kulupu implementation that uses the RandomX algorithm, as one of it's security goals that the Miners should not be able to identify "weaker" authorised parameters (such as specific nonce values) that make the PoW computation significantly more efficient, thus giving an advantage to a miner who would know such parameters, and in Kulupu implementation the nonce is actully the Hash of a random value
let nonce = H256::random_using(&mut rng);
In this implementation it doesn't really need to be a Hash at all, I think an U256 would be a better fit, as we really do increment the nonce by 1 every unsuccessful attempt.
the u8 was used cuz i didn't change anything in the rest of the code, where the nonce is a Hash, and the Sha3_256::digest() takes a &[u8] as input, so i found it a faster way.
Bottom line using the U256 for the nonce would eliminate the need for the numb variable. It was just quick and dirty implementation to make it work as discussed in #432
I updated the code to make it uses the type U256 for the nonce.
nodes/basic-pow/src/service.rs
Outdated
| if hash_meets_difficulty(&seal.work, seal.difficulty) { | ||
| let mut worker = worker.lock(); | ||
| worker.submit(seal.encode()); | ||
| } | ||
| numb = numb.saturating_add(1u8); | ||
| if numb == 255u8 { | ||
| numb = 0; | ||
| } | ||
| thread::sleep(Duration::new(0, 500_000_000)); |
There was a problem hiding this comment.
Isn't the logic here should be something like this?
| if hash_meets_difficulty(&seal.work, seal.difficulty) { | |
| let mut worker = worker.lock(); | |
| worker.submit(seal.encode()); | |
| } | |
| numb = numb.saturating_add(1u8); | |
| if numb == 255u8 { | |
| numb = 0; | |
| } | |
| thread::sleep(Duration::new(0, 500_000_000)); | |
| if hash_meets_difficulty(&seal.work, seal.difficulty) { | |
| let mut worker = worker.lock(); | |
| worker.submit(seal.encode()); | |
| } else { | |
| numb = numb.saturating_add(1u8); | |
| if numb == 255u8 { | |
| numb = 0; | |
| } | |
| } |
The last line of thread::sleep is in the else branch of if let Some(metadata) = metadata {..} else { thread::sleep }, based on code structure in https://github.com/kulupu/kulupu/blob/master/src/service.rs#L330-L377
There was a problem hiding this comment.
putting the thread::sleep in the else branch is for producing a delay in case no Block is yet ready to start mining, but i added it to create a delay in the mining process itself, since the difficulty was reduced due to the usage of the u8 numb variable to generate a nonce.
i updated this, the nonce uses the type U256 now.
- since there is no need for a random nonce, the nonce type `H256` was replaced with `U256`. - minor changes.
change the nonce type from `H256` to `U256`, as a Hash is not needed as nonce for this implementation.
|
@Mhnd3 thanks for addressing my concern and having the fixes. I just update the indenting spaces to tabs (we prefer tabs in rust code), and all look good to me. |
|
Thank you for contributing to the recipes @Mhnd3 ! We really appreciate it! <3 |
No description provided.