Skip to content

Replace uint256/uint160 with opaque blobs where possible (First)#1395

Merged
random-zebra merged 5 commits into
PIVX-Project:masterfrom
furszy:2020_replace_direct_use_of_0_uint256
Mar 17, 2020
Merged

Replace uint256/uint160 with opaque blobs where possible (First)#1395
random-zebra merged 5 commits into
PIVX-Project:masterfrom
furszy:2020_replace_direct_use_of_0_uint256

Conversation

@furszy
Copy link
Copy Markdown

@furszy furszy commented Mar 11, 2020 •
edited
Loading

Initial PR towards a big back port upstream#5490.

This is including:

  1. upstream@4f152496
    Replace x=0 with .SetNull(),
    x==0 with IsNull(), x!=0 with !IsNull().
    Replace uses of uint256(0) with UINT256_ZERO.

  2. upstream@80765854
    Replace GetLow64 with GetCheapHash

  3. upstream@2eae3157
    Replace uint256(1) with static constant

  4. Moved uint256(str) string parse to the adequate uint256S(str)method.

@furszy
Replace direct use of 0 with SetNull and IsNull.
741f43c 
Replace x=0 with .SetNull(),
x==0 with IsNull(), x!=0 with !IsNull().
Replace uses of uint256(0) with UINT256_ZERO.
@furszy furszy self-assigned this Mar 11, 2020
@furszy furszy changed the title Replace direct use of 0 with SetNull and IsNull. [WIP]Replace uint256/uint160 with opaque blobs where possible (cont'd) Mar 11, 2020
@furszy furszy changed the title [WIP]Replace uint256/uint160 with opaque blobs where possible (cont'd) [WIP] Replace uint256/uint160 with opaque blobs where possible (cont'd) Mar 11, 2020
@furszy
[Backport] Replace uint256(1) with static constant.
14319cf 
SignatureHash and its test function SignatureHashOld
return uint256(1) as a special error signaling value.
Return a local static constant with the same value instead.
@furszy furszy changed the title [WIP] Replace uint256/uint160 with opaque blobs where possible (cont'd) [WIP] Replace uint256/uint160 with opaque blobs where possible (First) Mar 11, 2020
@furszy furszy changed the title [WIP] Replace uint256/uint160 with opaque blobs where possible (First) Replace uint256/uint160 with opaque blobs where possible (First) Mar 12, 2020
@furszy furszy force-pushed the 2020_replace_direct_use_of_0_uint256 branch from 7e5e347 to 128d53a Compare March 14, 2020 19:31
@furszy
Copy link
Copy Markdown
Author

furszy commented Mar 14, 2020

PR description updated.

Have added lot more modifications, going into the same opaque blob uint256 direction.

There is lot more work that needs to be done to get to the final goal and this is already touching 59 files. All yours guys, going to continue in another PR on top of this work.

@furszy furszy mentioned this pull request Mar 15, 2020
Merged
random-zebra
random-zebra reviewed Mar 17, 2020
View reviewed changes
Copy link
Copy Markdown

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Looks good.
Just few SetNull() missing and some inconsistency with x == UINT256_ZERO and x.IsNull().

Comment thread src/coins.h
Comment thread src/coins.cpp
Comment thread src/masternode-budget.cpp Outdated
Comment thread src/masternode-budget.cpp Outdated
Comment thread src/primitives/transaction.cpp
Comment thread src/test/uint256_tests.cpp Outdated
Comment thread src/txdb.cpp Outdated
@furszy furszy force-pushed the 2020_replace_direct_use_of_0_uint256 branch from 128d53a to 400f6f4 Compare March 17, 2020 15:12
@furszy
Copy link
Copy Markdown
Author

furszy commented Mar 17, 2020

comments solved, squashed the changes.

random-zebra
random-zebra approved these changes Mar 17, 2020
View reviewed changes
Copy link
Copy Markdown

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK 400f6f4 and merging...

@random-zebra random-zebra merged commit b3fcbea into PIVX-Project:master Mar 17, 2020
akshaynexus added a commit to ZENZO-Ecosystem/ZENZO-Core that referenced this pull request Mar 22, 2020
@akshaynexus
Merge PIVX-Project#1395: Replace uint256/uint160 with opaque blobs wh…
e124b7e 
…ere possible (First)

400f6f4 Moving uint256(str) to proper uint256S(str). --> uint256 string parse. (furszy)
b0163f6 x=0 moved to x=UINT256_ZERO (furszy)
14319cf [Backport] Replace uint256(1) with static constant. (furszy)
d96eab9 Replace GetLow64 with GetCheapHash (furszy)
741f43c Replace direct use of 0 with SetNull and IsNull. (furszy)

Pull request description:

  Initial PR towards a big back port [upstream#5490](bitcoin#5490).

  This is including:

  1) [upstream@4f152496](bitcoin@4f15249)
  Replace x=0 with .SetNull(),
  x==0 with IsNull(), x!=0 with !IsNull().
  Replace uses of uint256(0) with UINT256_ZERO.

  2) [upstream@80765854](bitcoin@8076585)
  Replace GetLow64 with GetCheapHash

  3) [upstream@2eae3157](bitcoin@2eae315)
  Replace uint256(1) with static constant

  4) Moved `uint256(str)` string parse to the adequate `uint256S(str)`method.

ACKs for top commit:
  random-zebra:
    ACK 400f6f4 and merging...

Tree-SHA512: ed34b398ff00df8584efe079c764756d17bf5d7f1406aa6db84f82d9b544ae2b996c9b9e811d9d54989002a1f620ed798f77aa9e5b5d6c00a4de8310c5d6534e
@random-zebra random-zebra added this to the 4.1.0 milestone Apr 16, 2020
random-zebra added a commit that referenced this pull request May 12, 2020
@random-zebra
Merge #1414: uint256 to arith_uint256 migration (Second step).
e3ceda3 
d7ba881 Add missing blob/arith uint256 files to CMakeLists (furszy)
f05a97a Stack around the variable 'rv' was corrupted. (furszy)
2371666 arith_uint256 test back ported (includes arith_uint160 checks inside too) (furszy)
7d43a4b Migration process: uint256 main methods removed, using arith_uint256 for now. (furszy)
3669b9c uint512 modified to use the `blob_uint256` base class. New uint files added to makefile. (furszy)
7ca1fea arith_uint256 files created. (furszy)
21f824c Base blob uint256 files created. (furszy)

Pull request description:

  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:

  1) 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).

  2) Creating arith_uint256 files.

  3) 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).

  4) Updating, cleaning and connecting the arith_uint256 unit test (including arith_uint160 and arith_uint256 checks and validations).

ACKs for top commit:
  random-zebra:
    All good now 🍻 ACK d7ba881
  Fuzzbawls:
    ACK d7ba881

Tree-SHA512: 8f4561196c6dbf377a6c071b5dd2a329c819824ddcdb4774d5a610f9601f4df475993887349d1df5292b061090682d8f05803a302cdd2edabf9e3645dc1070e2
@random-zebra random-zebra mentioned this pull request Dec 20, 2020
Merged
random-zebra added a commit that referenced this pull request Jan 22, 2021
@random-zebra
Merge #2092: [Core] Migrate uint160 (CKeyID/CScriptID) to opaque blobs
6ceb077 
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
@random-zebra random-zebra mentioned this pull request Apr 14, 2021
Merged
furszy added a commit that referenced this pull request May 10, 2021
@furszy
Merge #2314: [Refactoring] Completed migration of uint* classes to blob
7e5430c 
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
@lyricidal lyricidal mentioned this pull request Feb 4, 2022
Merged
@furszy furszy deleted the 2020_replace_direct_use_of_0_uint256 branch November 29, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fuzzbawls Fuzzbawls Awaiting requested review from Fuzzbawls

1 more reviewer

@random-zebra random-zebra random-zebra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@furszy furszy

Labels

Refactoring Upstream

Projects

None yet

Milestone

4.1.0

Development

Successfully merging this pull request may close these issues.

2 participants

@furszy @random-zebra