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

Protect uint256 plain integer math #409

Merged
merged 1 commit into from Jul 31, 2016

Conversation

Projects
None yet
2 participants
@dexX7
Copy link
Member

dexX7 commented Jul 30, 2016

Currently there may be an overflow-ish issue when dividing and rounding up certain numbers.

This pull request adds a test for this case, and protects against it, by expecialy checking, wheter the numerator is zero.

This resolves #408.

@dexX7 dexX7 added this to the 0.0.11.0 milestone Jul 30, 2016

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Jul 30, 2016

The approach of returning a straight zero when the numerator is zero seems sound.

Confirming discussions on Slack:

  • without the patch I get
inputvalues (offered, desired, available) = 100000000, 10000, 0  
amountOffered256 = 0000000000000000000000000000000000000000000000000000000005f5e100  
amountDesired256 = 0000000000000000000000000000000000000000000000000000000000002710  
amountAvailable256 = 0000000000000000000000000000000000000000000000000000000000000000  
amountDesired256 after DivideAndRoundUp = 0000002af31dc4611873bf3f70834acdae9f0f4f534f5d60585a5f1c1a3ced1c0000002af31dc4611873bf3f70834acdae9f0f4f534f5d60585a5f1c1a3ced1c
  • with the patch I get
inputvalues (offered, desired, available) = 100000000, 10000, 0  
amountOffered256 = 0000000000000000000000000000000000000000000000000000000005f5e100  
amountDesired256 = 0000000000000000000000000000000000000000000000000000000000002710  
amountAvailable256 = 0000000000000000000000000000000000000000000000000000000000000000  
amountDesired256 after DivideAndRoundUp = 0000000000000000000000000000000000000000000000000000000000000000

OK to merge, and this should probably be considered a critical update.

OK to merge once this full reparse with --startclean and --omniseedblockfilter=false finishes.

@dexX7 dexX7 modified the milestones: 0.0.11.1, 0.0.11.0 Jul 30, 2016

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Jul 30, 2016

Just as a heads up - this passed the checkpoint for 420,000 just fine.

I'm currently syncing the last 1000 or so blocks until it's up to date so I can compare the hashes for the latest block against 0.0.11-rc2 (before we changed this). I fully expect them to match, it's just another sanity check.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Jul 30, 2016

Awesome, thanks for the detailed testing!

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Jul 30, 2016

0.0.11.1

zathras@zathras-pc:~/github/build/omnicore$ src/omnicore-cli omni_getmetadexhash
{
    "block" : 422923,
    "blockhash" : "000000000000000004d5af1014d188d191f81cb68753e2a4c17b5b0b7b7ec03f",
    "propertyid" : 0,
    "metadexhash" : "fe4e32511a4dee94b0670965b86244ddb340cc55e4ff9178c7bf65b59fdf1f47"
}
zathras@zathras-pc:~/github/build/omnicore$ src/omnicore-cli omni_getcurrentconsensushash
{
    "block" : 422923,
    "blockhash" : "000000000000000004d5af1014d188d191f81cb68753e2a4c17b5b0b7b7ec03f",
    "consensushash" : "9f44976c4057510eb0d0431a821afd405b51fc593feea53df3c7ec13f703c63b"
}

0.0.11-rc2

c:\Program Files\Omni Core\daemon>omnicore-cli.exe omni_getmetadexhash
{
    "block" : 422923,
    "blockhash" : "000000000000000004d5af1014d188d191f81cb68753e2a4c17b5b0b7b7ec03f",
    "propertyid" : 0,
    "metadexhash" : "fe4e32511a4dee94b0670965b86244ddb340cc55e4ff9178c7bf65b59fdf1f47"
}
c:\Program Files\Omni Core\daemon>omnicore-cli.exe omni_getcurrentconsensushash
{
    "block" : 422923,
    "blockhash" : "000000000000000004d5af1014d188d191f81cb68753e2a4c17b5b0b7b7ec03f",
    "consensushash" : "9f44976c4057510eb0d0431a821afd405b51fc593feea53df3c7ec13f703c63b"
}                                            ������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������������

All good on the final test.

I also don't usually use these calls interactively so didn't notice the extra propertyid field being returned, I'll submit a separate fix for that, so this PR is ready to go.

@dexX7 dexX7 merged commit b9b88d1 into OmniLayer:omnicore-0.0.10 Jul 31, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

dexX7 added a commit that referenced this pull request Jul 31, 2016

Merge pull request #409
b9b88d1 DivideAndRoundUp protection (dexX7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment