Replace uint256/uint160 by opaque blobs where possible #5478

laanwj · 2014-12-15T15:44:27Z

This pull replaces almost all uses of uint256 and all uses of uint160 to use opaque byte blobs blob256 and blob160 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.

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).
uint160 is completely removed as Bitcoin Core does no 160-bit integer arithmetic.

Overall steps (see commits)

Even though the diff is huge, I've tried to follow a logical and easy to review process:

Introduce base_uint::SetNull and base_uint::IsNull() as well as other methods that will later be on base_blob, to prepare for migration
Replace x=0 with .SetNull(), x==0 with IsNull(), x!=0 with !IsNull(). Replace uses of uint256(0) with uint256().
Introduce blob256 and blob160 as well as conversion functions (only needed for blob256, we don't ever compute with uint160).
Replace GetLow64() with GetCheapHash()
Rename uint256 and uint160 to blob256 and blob160 except where big integers are really necessary. For reviewing convenience I separated this out into

A) pure renames uintXXX to blobXXX, can easily be verified (in reverse) with
find -name *.h -print0 | xargs -0 sed -i 's/blob256/uint256/g'
find -name *.cpp -print0 | xargs -0 sed -i 's/blob256/uint256/g'
find -name *.h -print0 | xargs -0 sed -i 's/blob160/uint160/g'
find -name *.cpp -print0 | xargs -0 sed -i 's/blob160/uint160/g'

B) string conversions uint256("string") to blob256S("string")

C) Added #includes and predeclared classes

D) Necessary conversions between uint256 and blob256 Focus reviewing here
Remove now-unused methods from base_uint and blob160/blob256, eg GetHash, also remove unused uint160.

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

Convert between blobs and uints, mostly for proof of work checks.

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.

Clients outside the class have no business poking at the internals.

Avoid dangerous cases where 0 is interpreted as std::string(0). Keyword `explicit` does not help here.

paveljanik · 2014-12-15T20:46:43Z

This appears to be unused now:

src/test/uint256_tests.cpp:const double R1Sdouble = 0.7096329412477836074;

laanwj · 2014-12-16T05:16:15Z

@paveljanik Thanks, I'll remove it. All *S variables in uint256_tests.cpp are for testing uint160, which went away.

No uint160 arithmetic is used at all. Also remove the tests.

laanwj · 2014-12-16T07:45:43Z

Re: people complaining about rebasing their pulls, if the large diff in 'A: pure renames' is problematic, we could cheat by changing uint256 and uint160 to be blob types and introduce a new type for actual 256 bit integer arithmetic. But as clear type names are important I don't really like this.

sipa · 2014-12-16T11:24:25Z

src/blob256.h

+
+    friend inline bool operator==(const base_blob& a, const base_blob& b) { return memcmp(a.data, b.data, sizeof(a.data)) == 0; }
+    friend inline bool operator!=(const base_blob& a, const base_blob& b) { return memcmp(a.data, b.data, sizeof(a.data)) != 0; }
+    friend inline bool operator<(const base_blob& a, const base_blob& b) {


Would it break anything if we defined the ordering of blob256 as platform-dependent? That would allow using memcmp for this operator too.

In fact, I think that would allow implementing blob* as wrappers around byte arrays, and leave all integer conversion to uint*.

EDIT: Sorry, they already are byte-arrays; I should read more before commenting.

EDIT2: In fact, I think the implementation below is already identical to just "memcmp(a.data, b.data, sizeof(a.data)) < 0".

Yes, it's identical to memcmp, good catch. Will use that.

sipa · 2014-12-16T12:44:19Z

I would actually like the PR as a whole to just introduce arith_uint256 or something (for the version with arithmetic semantics) and leave uint256/uint160 in place (for the version without). That will result in a much smaller patchset, and require much less rebasings while this is being reviewed.

Perhaps later there can be mass rename that is trivial to review and merge.

laanwj · 2014-12-16T13:25:41Z

Ok, reluctantly agreed... As I say above already I hate the idea of using uint160/uint256 for what are not actually integers and introduce a yes_this_is_really_an_int256 for real uint256 arithmetic, but yes the diff will be much smaller.

laanwj · 2014-12-16T14:05:41Z

Closing, will reopen after reorganization.

Continued in #5490

laanwj added 12 commits December 15, 2014 10:05

Temporarily add SetNull/IsNull/GetCheapHash to base_uint

722cdf9

Eases step-by-step migration to blob.

Replace direct use of 0 with SetNull and IsNull

fa06531

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

Replace GetLow64 with GetCheapHash

9e6b762

Add blob256.cpp/h to build

354700c

Add UintToBlob256 and BlobToUint256

ee72dde

Convert between blobs and uints, mostly for proof of work checks.

Replace uint256(1) with static constant

9b84328

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.

protect base_uint begin() and end()

1369788

Clients outside the class have no business poking at the internals.

blob256: make initialization from string explicit

4008b7a

Avoid dangerous cases where 0 is interpreted as std::string(0). Keyword `explicit` does not help here.

A: pure renames uintXXX to blobXXX

d9569ed

B: string conversions

cc28e23

C: Includes and predeclared classes

9c57d5d

D: Necessary conversions between uint256 and blob256

65f2ff8

laanwj force-pushed the 2014_12_the_blob2 branch from 6777175 to e129c6a Compare December 15, 2014 17:11

theuni mentioned this pull request Dec 15, 2014

don't assume the address of a uint256 is a pointer to its internal representation #5480

Closed

laanwj added 2 commits December 16, 2014 08:00

Remove uint160

2a0f581

No uint160 arithmetic is used at all. Also remove the tests.

Remove now-unused methods from uint256 and base_uint

cfe9453

Add tests for blob256

8f1563a

laanwj force-pushed the 2014_12_the_blob2 branch from e129c6a to 8f1563a Compare December 16, 2014 09:03

laanwj added the Refactoring label Dec 16, 2014

sipa reviewed Dec 16, 2014
View reviewed changes

laanwj closed this Dec 16, 2014

laanwj mentioned this pull request Dec 16, 2014

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

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 by opaque blobs where possible #5478

Replace uint256/uint160 by opaque blobs where possible #5478

Uh oh!

laanwj commented Dec 15, 2014

Uh oh!

paveljanik commented Dec 15, 2014

Uh oh!

laanwj commented Dec 16, 2014

Uh oh!

laanwj commented Dec 16, 2014

Uh oh!

sipa Dec 16, 2014

Uh oh!

sipa Dec 16, 2014

Uh oh!

laanwj Dec 16, 2014

Uh oh!

sipa commented Dec 16, 2014

Uh oh!

laanwj commented Dec 16, 2014

Uh oh!

laanwj commented Dec 16, 2014

Uh oh!

Uh oh!

Replace uint256/uint160 by opaque blobs where possible #5478

Replace uint256/uint160 by opaque blobs where possible #5478

Uh oh!

Conversation

laanwj commented Dec 15, 2014

Overall steps (see commits)

Uh oh!

paveljanik commented Dec 15, 2014

Uh oh!

laanwj commented Dec 16, 2014

Uh oh!

laanwj commented Dec 16, 2014

Uh oh!

sipa Dec 16, 2014

Choose a reason for hiding this comment

Uh oh!

sipa Dec 16, 2014

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 16, 2014

Choose a reason for hiding this comment

Uh oh!

sipa commented Dec 16, 2014

Uh oh!

laanwj commented Dec 16, 2014

Uh oh!

laanwj commented Dec 16, 2014

Uh oh!

Uh oh!