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

Downcasting in difficulty calculation can lead to overflow #3923

Closed
CjS77 opened this issue Mar 16, 2022 · 1 comment
Closed

Downcasting in difficulty calculation can lead to overflow #3923

CjS77 opened this issue Mar 16, 2022 · 1 comment
Labels
C-bug Category - fixes a bug, typically associated with an issue. E-good_first_issue Experience Level - Good for newcomers

Comments

@CjS77
Copy link
Collaborator

CjS77 commented Mar 16, 2022

The difficulty calculation has an overflow issue:

pub(crate) fn big_endian_difficulty(hash: &[u8]) -> Difficulty {
        let scalar = U256::from_big_endian(hash); // Big endian so the hash has leading zeroes
        let result = U256::MAX / scalar;
        result.low_u64().into()
    }

Here, scalar is the target for the mining algorithm. The smaller the target, the more difficult it is to find a hash with the requisite number of leading zeroes.

The definition of difficulty is TARGET_MAX / target. Once target falls below 2^192, the difficulty will overflow to zero, since we downcast to 64 bits by truncating the highest portion of the actual difficulty.

Now, this is possibly only a theoretical problem since we need a ridiculous hashrate before this become an eventuality. We will certainly have native 256-bit integers long before this happens.

However, to protect against weird things happening, it might be better to prevent the overflow anyway by returning u64::max if scalar <= 2^192.

@CjS77 CjS77 added C-bug Category - fixes a bug, typically associated with an issue. E-good_first_issue Experience Level - Good for newcomers labels Mar 16, 2022
jorgeantonio21 added a commit to jorgeantonio21/tari that referenced this issue May 11, 2022
@jorgeantonio21
Copy link
Contributor

Hello, I am interested in tackling this issue. I included the suggested code change in a newly created PR:

#4090

I also want to add test coverage for this new functionality, but I am having some difficulty with it, which I describe in the PR. Any help is highly appreciate it.

@aviator-app aviator-app bot closed this as completed in e8d1091 May 12, 2022
aviator-app bot pushed a commit that referenced this issue 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
C-bug Category - fixes a bug, typically associated with an issue. E-good_first_issue Experience Level - Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants