Replace uint256/uint160 with opaque blobs where possible (cont'd) #5490

laanwj · 2014-12-16T17:43:22Z

Second attempt at #5478. See discussion there for context.

This pull replaces almost all uses of uint256 and all uses of uint160 to use opaque byte blobs with only the following operations:

Default initialization to 0, or from a vector of bytes
Assignment from other blobXXXs
IsNull() compare to special all-zeros value
SetNull() clear to special all-zeros value: Bitcoin needs IsNull() / SetNull() because we often use the all-zeroes value as a marker for errors and empty values.
< for sorting in maps.
!= == test for (in)equality
GetHex/SetHex/ToString because they're just so useful
begin()/end() raw access to the data
size() returns a fixed size
GetSerializeSize() Serialize Unserialize serialization just reads and writes the bytes
GetCheapHash() this is similar to GetLow64() and returns part of the value as uint64_t, for cheap hashing when the contents are assumed distributed uniformy random.

arith_uint256 (the old uint256, used for proof-of-work mainly), on the other hand, loses all functionality like raw bytes access and serialization. Its memory should not be accessed directly. This is necessary for #888 (big endian support).
arith_uint160 (the old uint160) is completely removed as Bitcoin Core does no 160-bit integer arithmetic.

For overall steps see commits.

sipa · 2014-12-17T00:02:07Z

Reviewed all commits up to f66f592 except b7155a9 and 958431c.

sipa · 2014-12-18T12:56:56Z

Verified b7155a9's code in aith_uint256.{h,cpp} to be a pure copy of the original code in uint256.{h,cpp}, apart from some type name changes, and IMPORTANT 2 deleted trailing white spaces in the comments above SetCompact.

sipa · 2014-12-18T13:24:51Z

Untested ACK, but the code (both blob and arithmetic versions of it) looks well tested.

laanwj · 2014-12-19T12:19:39Z

I've added a commit that (and left the rest untouched)

Removes direct arith_uint256 initialization from byte vector. It is only used in the tests, so I've provided an alternative function there that just goes through uint256.
Implements GetHex and SetHex in terms of uint256. As these functions work bytewise and expect little-endian they belong there (also removes a bit of duplication).

I'm not exactly sure about the latter. It is an improvement, but other options would be:

Reimplement GetHex and SetHex in terms of uint32_t's instead of bytes, but keep them on arith_uint256
We could even get away with removing at least arith_uint256's SetHex and initialization from strings completely (AFAIK also only used in the tests). It's nice to be able to print a number though.

sipa · 2014-12-30T00:54:06Z

Let's not let this go too unrebasable...

laanwj · 2014-12-30T17:43:02Z

Rebased for 78253fc and d78f0da

gmaxwell · 2015-01-01T10:36:55Z

ACK.

laanwj · 2015-01-02T11:03:24Z

Rebased for c444c62

sipa · 2015-01-02T15:18:58Z

@jgarzik @gavinandresen Care to review?

laanwj · 2015-01-03T09:42:43Z

This isn't catched by git, only by the compiler, but needs rebase for 0125988 as it introduces uint256(int)s in pmt_tests.

Also add a stub for arith_uint256 and its conversion functions, for now completely based on uint256. Eases step-by-step migration to blob.

Replace x=0 with .SetNull(), x==0 with IsNull(), x!=0 with !IsNull(). Replace uses of uint256(0) with uint256().

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.

If uint256() constructor takes a string, uint256(0) will become dangerous when uint256 does not take integers anymore (it will go through std::string(const char*) making a NULL string, and the explicit keyword is no help).

Also add conversion from/to uint256 where needed.

Introduce new opaque implementation of `uint256`, move old "arithmetic" implementation to `arith_uint256.

We never do 160-bit arithmetic.

- Methods that access the guts of arith_uint256 are removed, as these are incompatible between endians. Use uint256 instead - Serialization is no longer needed as arith_uint256's are never read or written - GetHash is never used on arith_uint256

Remove initialization from vector (as this is only used in the tests). Also implement SetHex and GetHex in terms of uint256, to avoid duplicate code as well as avoid endianness issues (as they work in term of bytes).

laanwj · 2015-01-05T15:03:52Z

Rebased for 0125988.
If Travis passes I'm just going to take the (minimal) risk and merge this, so that the big-endian patchset is easier to maintain.

6bd0dc2 arith_uint256: remove initialization from byte vector (Wladimir J. van der Laan) 30007fd Remove now-unused methods from arith_uint256 and base_uint (Wladimir J. van der Laan) edc7204 Remove arith_uint160 (Wladimir J. van der Laan) dba2e91 Add tests for new uint256 (Wladimir J. van der Laan) 92cdb1a Add conversion functions arith_uint256<->uint_256 (Wladimir J. van der Laan) bfc6070 uint256->arith_uint256 blob256->uint256 (Wladimir J. van der Laan) 734f85c Use arith_uint256 where necessary (Wladimir J. van der Laan) 34cdc41 String conversions uint256 -> uint256S (Wladimir J. van der Laan) 2eae315 Replace uint256(1) with static constant (Wladimir J. van der Laan) 8076585 Replace GetLow64 with GetCheapHash (Wladimir J. van der Laan) 4f15249 Replace direct use of 0 with SetNull and IsNull (Wladimir J. van der Laan) 5d3064b Temporarily add SetNull/IsNull/GetCheapHash to base_uint (Wladimir J. van der Laan)

… (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

…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

laanwj added the Refactoring label Dec 16, 2014

laanwj mentioned this pull request Dec 17, 2014

Replace uint256/uint160 by opaque blobs where possible #5478

Closed

laanwj mentioned this pull request Dec 19, 2014

Add mips, mipsel and aarch64 to depends platforms #5501

Merged

laanwj mentioned this pull request Dec 19, 2014

Big endian support #5510

Merged

laanwj force-pushed the 2014_12_the_blob5 branch from 8d31cbe to adc105a Compare December 30, 2014 17:43

laanwj force-pushed the 2014_12_the_blob5 branch from adc105a to 3eb5909 Compare January 2, 2015 11:02

laanwj mentioned this pull request Jan 2, 2015

Update libsecp256k1 and switch to its builtin RFC6979 #5506

Merged

laanwj added 12 commits January 5, 2015 15:14

Temporarily add SetNull/IsNull/GetCheapHash to base_uint

5d3064b

Also add a stub for arith_uint256 and its conversion functions, for now completely based on uint256. Eases step-by-step migration to blob.

Replace direct use of 0 with SetNull and IsNull

4f15249

Replace x=0 with .SetNull(), x==0 with IsNull(), x!=0 with !IsNull(). Replace uses of uint256(0) with uint256().

Replace GetLow64 with GetCheapHash

8076585

Replace uint256(1) with static constant

2eae315

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.

String conversions uint256 -> uint256S

34cdc41

If uint256() constructor takes a string, uint256(0) will become dangerous when uint256 does not take integers anymore (it will go through std::string(const char*) making a NULL string, and the explicit keyword is no help).

Use arith_uint256 where necessary

734f85c

Also add conversion from/to uint256 where needed.

uint256->arith_uint256 blob256->uint256

bfc6070

Introduce new opaque implementation of `uint256`, move old "arithmetic" implementation to `arith_uint256.

Add conversion functions arith_uint256<->uint_256

92cdb1a

Add tests for new uint256

dba2e91

Remove arith_uint160

edc7204

We never do 160-bit arithmetic.

arith_uint256: remove initialization from byte vector

6bd0dc2

Remove initialization from vector (as this is only used in the tests). Also implement SetHex and GetHex in terms of uint256, to avoid duplicate code as well as avoid endianness issues (as they work in term of bytes).

laanwj force-pushed the 2014_12_the_blob5 branch from 3eb5909 to 6bd0dc2 Compare January 5, 2015 14:48

laanwj merged commit 6bd0dc2 into bitcoin:master Jan 5, 2015

furszy mentioned this pull request Mar 11, 2020

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

Merged

nuttycom mentioned this pull request Feb 11, 2021

Backport of bitcoin/bitcoin#8020 zcash/zcash#4979

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace uint256/uint160 with opaque blobs where possible (cont'd) #5490

Replace uint256/uint160 with opaque blobs where possible (cont'd) #5490

Uh oh!

laanwj commented Dec 16, 2014

Uh oh!

sipa commented Dec 17, 2014

Uh oh!

sipa commented Dec 18, 2014

Uh oh!

sipa commented Dec 18, 2014

Uh oh!

laanwj commented Dec 19, 2014

Uh oh!

sipa commented Dec 30, 2014

Uh oh!

laanwj commented Dec 30, 2014

Uh oh!

gmaxwell commented Jan 1, 2015

Uh oh!

laanwj commented Jan 2, 2015

Uh oh!

sipa commented Jan 2, 2015

Uh oh!

laanwj commented Jan 3, 2015

Uh oh!

laanwj commented Jan 5, 2015

Uh oh!

Uh oh!

Replace uint256/uint160 with opaque blobs where possible (cont'd) #5490

Replace uint256/uint160 with opaque blobs where possible (cont'd) #5490

Uh oh!

Conversation

laanwj commented Dec 16, 2014

Uh oh!

sipa commented Dec 17, 2014

Uh oh!

sipa commented Dec 18, 2014

Uh oh!

sipa commented Dec 18, 2014

Uh oh!

laanwj commented Dec 19, 2014

Uh oh!

sipa commented Dec 30, 2014

Uh oh!

laanwj commented Dec 30, 2014

Uh oh!

gmaxwell commented Jan 1, 2015

Uh oh!

laanwj commented Jan 2, 2015

Uh oh!

sipa commented Jan 2, 2015

Uh oh!

laanwj commented Jan 3, 2015

Uh oh!

laanwj commented Jan 5, 2015

Uh oh!

Uh oh!