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) companion #4097

Conversation

jorgeantonio21
Copy link
Contributor

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)

@CjS77
Copy link
Collaborator

CjS77 commented May 12, 2022

Love the enthusiasm, but let #3923 get merged, and then rebase. :)
But looks good to me.

@jorgeantonio21
Copy link
Contributor Author

Thanks for the encouragement @CjS77, I probably rushed a bit here. In the meanwhile, I will keep looking at the codebase :)

@CjS77
Copy link
Collaborator

CjS77 commented May 13, 2022

#4090 is merged, so rebase / reset the changes to remove the conflicts and we should be good

@stringhandler
Copy link
Collaborator

Just need to resolve the conflicts here

@aviator-app aviator-app bot added the mq-failed label Jul 2, 2022
@jorgeantonio21
Copy link
Contributor Author

The PR should be ready to be merged ! Let me know if you have any additional comments/suggestions :)

stringhandler
stringhandler previously approved these changes Jul 4, 2022
@sdbondi
Copy link
Member

sdbondi commented Jul 13, 2022

@jorgeantonio21 Unfortunately, to merge this we need all commits to be signed.

You may also need to run this command to resign all commits:
git rebase --exec 'git commit --amend --no-edit -n -S' -i development

(might need to remove the command from the last merge commit d4cdac5 in the interactive rebase)

@jorgeantonio21 jorgeantonio21 force-pushed the ja-overflow-little-endian-difficulty branch 2 times, most recently from b37193b to 905f8ee Compare July 20, 2022 10:48
@jorgeantonio21
Copy link
Contributor Author

@sdbondi I followed your instructions, if I haven't screwed up, commits should now be signed and the PR be ready to merge. Thanks for the advice !

sdbondi
sdbondi previously approved these changes Jul 20, 2022
@sdbondi
Copy link
Member

sdbondi commented Jul 20, 2022

@jorgeantonio21 Looks like it didn't take :/
image

@sdbondi
Copy link
Member

sdbondi commented Jul 20, 2022

@jorgeantonio21 I think if you make the commit over again it should work.

As in:

git reset --soft development
git commit -am "....."

@delta1
Copy link
Contributor

delta1 commented Jul 20, 2022

@jorgeantonio21 I used this to get set up https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

stringhandler
stringhandler previously approved these changes Jul 20, 2022
@aviator-app aviator-app bot removed the mq-failed label Jul 20, 2022
@stringhandler stringhandler dismissed their stale review July 20, 2022 12:55

Think there are some commits getting reverted here...

@jorgeantonio21 jorgeantonio21 force-pushed the ja-overflow-little-endian-difficulty branch from ed40e6f to 4ce678b Compare July 20, 2022 13:07
@aviator-app aviator-app bot merged commit ddb8453 into tari-project:development Jul 20, 2022
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.

5 participants