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

[Refactoring] Completed migration of uint* classes to blob #2314

Merged
merged 17 commits into from
May 10, 2021

Conversation

random-zebra
Copy link

Finally out of an old rabbit hole, started with #1395, #1414, and #2092.
As per title, complete the migration:

  • use arith_uint* classes whenever the uint is interpreted as a number
  • make uint* classes child of base_blob, and use it everywhere else
  • remove blob_uint* temporary class
  • update tests

@random-zebra random-zebra added this to the 6.0.0 milestone Apr 14, 2021
@random-zebra random-zebra self-assigned this Apr 14, 2021
@random-zebra random-zebra marked this pull request as draft April 15, 2021 02:44
@random-zebra random-zebra marked this pull request as ready for review April 15, 2021 23:30
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Great work conclusion!
Code review ACK 90059b4

Will run it some time on mainnet before giving the final approval.

@random-zebra
Copy link
Author

Rebased on master fixing minor conflicts with #2288 and #2273 just merged.
I've been running this changeset no-stop on main net and testnet for the past 10 days without issue. Think it can be considered for merging.

@random-zebra
Copy link
Author

Rebased again.

random-zebra added a commit that referenced this pull request May 5, 2021
…SG usage for dynamic messages.

8ff6fa2 [test] validation_block_tests: New ASSERT_WITH_MSG to print dynamic error messages. (furszy)

Pull request description:

  Bug discovered checking GA failing reason in #2314 --> [job](https://github.com/PIVX-Project/PIVX/pull/2314/checks?check_run_id=2509761057)

  Essentially, `BOOST_ASSERT_MSG` only prints static string messages. As we are inputting a dynamic message, the value isn't being printed if the test fails.
  So, have added a new function `ASSERT_WITH_MSG(cond, msg)`  to support dynamic error messages.

ACKs for top commit:
  random-zebra:
    ACK 8ff6fa2 and merging... (this means another rebase for #2360)

Tree-SHA512: a47c8f94a497696a5bbb2b09b01f8dd1711b898a3d363211d12c79fd92b67a4f4d255a2c97ae192e17ccab706a604e16140a18bc7ea3f3539e8fad4559351902
random-zebra and others added 16 commits May 6, 2021 00:48
- delete old uint256.h/uint256.cpp files
- delete old uint512.h
- rename files blob_uint256.h/blob_uint256.cpp --> uint256.h/uint256.cpp
- rename class blob_uint256 --> uint256
- add uint160/512 blob implementation inside uint256.* files
- move UintToArith/ArithToUint to arith_uint256.cpp
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Have been running this since the last rebase and all good, ACK 722759d

@random-zebra
Copy link
Author

This will have few conflicts with #2352. They are both a pain to rebase / re-review / re-test, so am not sure which one we want to merge first. Pinging @Fuzzbawls

@Fuzzbawls
Copy link
Collaborator

I'm ok with prioritizing this over #2352 and did some initial code review already, will finish my review later today but looking good so far.

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 722759d

@furszy furszy merged commit 7e5430c into PIVX-Project:master May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants