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

fix: possible overflow in difficulty calculation (fixes #3923) #4090

Conversation

jorgeantonio21
Copy link
Contributor

Description

Motivation and Context

  • The main goal is to avoid possible overflow of difficulty, which even though extremely unlikely, it is a possible scenario.

How Has This Been Tested?

  • So far, changes have not been tested. I am more than happy to get recommendations of tests. I haven't add them at the moment because it is not clear how to build an hash whose big endian is a scalar less than 2^192. I am therefore, very interested to understand this concept and add tests accordingly.

@sdbondi sdbondi changed the title attempt to tackle issue #3923 fix: possible overflow in difficulty calculation (fixes #3923) May 11, 2022
@CjS77
Copy link
Collaborator

CjS77 commented May 11, 2022

You can also write simple unit tests that are something like

#[test]
fn high_target() {
 let target: &[u8] = &[0xff, 0xff, 0xff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 ];
let expected = 1; # replace by the actual difficulty
assert!(big_endian_difficulty(target), expected);
} 

#[test]
fn max_difficulty() {
 let target = u64::MAX;
let expected = 1; # replace by the actual difficulty
assert!(big_endian_difficulty(target.to_be_bytes()), expected);
} 

#[test]
fn stop_overflow() {
 let target = 64;
let expected = u64::MAX; # replace by the actual difficulty
assert!(big_endian_difficulty(target.to_be_bytes()), expected);
} 

@jorgeantonio21
Copy link
Contributor Author

jorgeantonio21 commented May 11, 2022

#[test]
fn high_target() {
 let target: &[u8] = &[0xff, 0xff, 0xff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 ];
let expected = 1; # replace by the actual difficulty
assert!(big_endian_difficulty(target), expected);
} 

#[test]
fn max_difficulty() {
 let target = u64::MAX;
let expected = 1; # replace by the actual difficulty
assert!(big_endian_difficulty(target.to_be_bytes()), expected);
} 

#[test]
fn stop_overflow() {
 let target = 64;
let expected = u64::MAX; # replace by the actual difficulty
assert!(big_endian_difficulty(target.to_be_bytes()), expected);
} 

I added these tests, with minor mods to get the correct values. Thank you for the suggestion

Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

Looking good. If you could finlise the test cases to improve the coverage slightly, and not keep feature flags consistent, we should be good.

(And a note that the little_endian version has the same bug, but this is for another PR)

base_layer/core/src/proof_of_work/difficulty.rs Outdated Show resolved Hide resolved
@CjS77
Copy link
Collaborator

CjS77 commented May 11, 2022

I think you also need to run cargo fmt -p tari_core to pass the clippy tests here.

Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

LGTM - assuming tests pass

@aviator-app aviator-app bot merged commit e8d1091 into tari-project:development May 12, 2022
aviator-app bot pushed a commit that referenced this pull request Jul 20, 2022
…ion (#4097)

Description
Quick fix to overflow problem in difficulty. This problem had already been settled for `big_endian_difficulty`.

Motivation and Context
In #4090, @CjS77 point it out that the `little_endian_difficulty` method had the same problem. This PR is an attempt to tackle it. 

How Has This Been Tested?
Adapted tests from previous PR (#4090)
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 this pull request may close these issues.

None yet

2 participants