uint256 to arith_uint256 migration (Second step). #1414
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Needs rebase, now that #1395 has been merged. |
furszy
force-pushed
the
2020_base_blob_uint256
branch
from
28addf6
to
b4ceeb3
Compare
|
PR rebased, #1395 was merged. Ready for review. |
New uint files added to makefile.
…for now. Once all of the invalidly instantiated fields using uint256 as a number get migrated to the arith_uint256, the uint256 file will be replaced with the blob_uint256 file.
furszy
force-pushed
the
2020_base_blob_uint256
branch
from
b4ceeb3
to
2371666
Compare
|
Updated. |
Coming from upstream@e96c5184e783cf940956bf803dae8690dcf2b496
|
Need to update CMakeLists with |
|
Done, added missing files to cmakelist. |
Fuzzbawls
approved these changes
May 12, 2020
random-zebra
added a commit
that referenced
this pull request
Jan 22, 2021
dea250c [Core] Migrate uint160 (CKeyID/CScriptID) to opaque blobs (random-zebra) 86a106e [Tests] Use arith_uint160 instead of uint160 in uint256_tests (random-zebra) 12240dc [BUG] fix a few places improperly checking boost::get<CKeyID> (random-zebra) Pull request description: Keep going with the migration started in #1395 and #1414. This time migrate the whole class `uint160`, so it is defined as a child of `base_blob` (essentially replace `uint160` with `blob_uint160`), thus migrating `CKeyID` and `CScriptID` as well. Since opaque blobs (unlike the `arith_uint` classes) have trivial copy-assignment operator, this fixes the "scary" `-Wclass-memaccess` warning reported in #2076 (tested before/after patch, with gcc v9.3.0) Bonus: this fixes also a minor bug found along the way: few places where we are checking the result of `boost::get<CKeyID>` over the pointed `CKeyID`, which shouldn't even have a `bool operator()`, and might even be un-initialized (though we always verify `IsValidDestination` before, so this is mostly redundant) instead of checking the pointer itself. ACKs for top commit: furszy: looking good, utACK dea250c Fuzzbawls: utACK dea250c Tree-SHA512: 2e6d6e2adfcd8a1c32941f0480b882ea801fe1816eaa0c5050efc506c0aa09f6d589d53fb13798a17ffd2858021c3a0ccd50465acbc7d5c5a71dc0fb0f398756
furszy
added a commit
that referenced
this pull request
May 10, 2021
722759d [Refactor] Remove arith_uint256::GetCheapHash() and fix uint256 implem. (random-zebra) a640051 [Refactor] uint256: update GetHex() and SetHex() (random-zebra) bce58ac [Refactor] uint256: raname data --> m_data and make it protected (random-zebra) 4ddd8f1 [BUG] Use arith_uint256 for old modifier's block sorting (random-zebra) b1c0afb [Trivial] Log stake modifier and timeBlockFrom in CheckKernelHash() (random-zebra) ddf0dcb [arith_uint256] Do not destroy *this content if passed-in operator may reference it (Karl-Johan Alm) 8fe19b1 [Refactor] uint256 add missing explicit instantiation for base_blob<512> (random-zebra) 22213ff [Refactor] Migrate uint256.h/.cpp to blob_uint implementation (random-zebra) 957f529 [Refactor] pow/pos: use arith_uint256 for difficulty targets (random-zebra) 9663d99 [Refactoring] GUI/RPC: use uint256S() to construct uint256 from string (random-zebra) b02a67b [Refactor] Use arith_uint256 for masternode ranking computations (random-zebra) 990072e [Refactor] Move trim256 and use arith_uint512 for HashQuark calculations (random-zebra) f8c41ca [Refactor] zerocoin: use arith_uint256 where needed (random-zebra) bce0583 [Refactor] zc ParamGeneration: pass big args by ref and use arith_uint (random-zebra) 6488f24 [Tests] Fix arith in pedersen_hash_/skiplist_/pmt_/transaction_tests (random-zebra) 86e177b [Tests] Replace uint256 tests with blob_uint256 tests (random-zebra) b4a459b [Refactoring] BIP38: init uint from string and use arith where needed (random-zebra) Pull request description: 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 ACKs for top commit: furszy: Have been running this since the last rebase and all good, ACK 722759d Fuzzbawls: ACK 722759d Tree-SHA512: cf593243c2f2dbba1a7534cacb7ed261518b4969498dfc703ea875faf87ad94a3ee25eb587aebda225d8617a94301bff5017bb7f00b04d91e0f3e3fbbd04e0f3
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is part of the large effort of making the uint256 file an opaque blob object and not treat every uint256 as an arith_uint256 anymore (same for the uint160 and uint512). Work coded on top of #1395, this is the second step.
As we cannot do it all at once, i opted to do the migration in steps.
The amount of changes needed is really extensive and the risk of damage the synchronization process -and other areas as well- is high, this will need changes on the MNs, budget and zerocoin library sources which are using the uint256 object purely as a number.
This step is doing the following changes:
Creating blob_uint256 files (opaque blob version of the uint256 files -- this is the future uint256 files that will be refactored once all of the fields that are using it as a number in our sources get migrated to use the new arith_uint256 classes).
Creating arith_uint256 files.
Cleaning shared code between the uint256 file and the arith_uint256 classes and only leaving in uint256 the old classes that needs to be checked and migrated -- if them are using the uint256 as a number and not as an opaque blob object -- (following the explanation in point 1).
Updating, cleaning and connecting the arith_uint256 unit test (including arith_uint160 and arith_uint256 checks and validations).