-
Notifications
You must be signed in to change notification settings - Fork 219
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
fix: possible overflow in difficulty calculation (fixes #3923) #4090
Conversation
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);
} |
I added these tests, with minor mods to get the correct values. Thank you for the suggestion |
There was a problem hiding this 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)
I think you also need to run |
There was a problem hiding this 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
…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)
Description
Motivation and Context
How Has This Been Tested?