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

Remove BIGNUM use #214

Merged
merged 6 commits into from
Jun 20, 2018
Merged

Remove BIGNUM use #214

merged 6 commits into from
Jun 20, 2018

Conversation

aguycalled
Copy link
Member

This PR completely removes the use of the OpenSSL's class BIGNUM, substituting the uses of CBigNum with the class uint256 with extended arithmetic capabilities (arith_uint256). OpenSSL deprecated some BIGNUM functions in version 1.1, making the wallet not being able to compile in systems which use the newer version. This patch fixes the refered issue.

There's an special case where arith_uint256 is not suitable to handle the needed operations, namely when verifying the kernel hash of proof of stake blocks. Due to the value of the transactions in the first blocks being very high (total supply coming from navajocoin's blockchain because of coin swap) the multiply operation can easily overflow leading to errors in the block validation.

For that reason, arith_uint512 is used in CheckStakeKernelHashV2().

Copy link
Contributor

@red010b37 red010b37 left a comment

Choose a reason for hiding this comment

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

Code is fine.

However, I'm not keen to jam new code into the 4.2.0 release. I am happy for it to go into the cold staking / cfund next release or even as a patch on its own.

@aguycalled aguycalled changed the base branch from v4.2.0-rc to master June 12, 2018 20:01
@aguycalled aguycalled merged commit 739c102 into master Jun 20, 2018
@aguycalled aguycalled deleted the v4.1.2-remove-bignum branch June 20, 2018 17:25
aguycalled pushed a commit to skreener/navcoin-core that referenced this pull request Feb 3, 2019
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.

2 participants