changed field types in some structures to equivalent unambiguous types #4180

int -> int32_t unsigned int -> uint32_t unsigned chat -> uint8_t

Thus the implementation becomes not only unambiguous, but also more compliant with the protocol specification.

There's actually a significantly greater number of such ambiguities in classes within main.h which are not explicitly described by protocol specs. Apart from the ambiguity they often cause unnecessary casts. I'd like to convert them en masse into (u)int*_t and exterminate the casts but I need an opinion whether or not it's worth doing. I remember some scepticism about this from #bitcoin-dev.

I would be weary of just find-replacing all int's to int32_t's, etc.

In my opinion they should only be converted to using {,u}int{8,16,32,64}_t if the data structure is rigidly defined to be that size. Otherwise int should be considered the default type for numerical values and long used if it would make sense and be convenient to use a 64-bit value if available.

Rebase needed + verification needs to be done here that the resulting code is exactly the same.

I will proceed with this within a few days.

Thus the implementation becomes not only unambiguous, but also more compliant with the protocol specification.

The protocol specification is not the wiki, it's the sourcecode. If the wiki doesn't match that code, it should be changed - not the other way around.

Anyway, the changes look ok, but you definitely need to check case-by-case that they really cause no changes. I'd suggest writing up a report with a line or two describing each individual change. A lot of work I know, but that's just the price of working on Bitcoin Core.

The protocol specification is not the wiki, it's the sourcecode.

I'll quote you every time someone on #bitcoin-dev says there is no One True Implementation.

I disagree when it's just about the p2p protocol, as the language that two nodes speak to each other does not impact the network. When it's about the consensus rules, it's more complicated.

Anyway: I think in many places this is the correct thing to do. As is, the serialization framework really only works because of very strict assumptions on data type sizes. I think it's perfectly acceptable to turn every data type that is ever serialized (appears inside a READDATA(x)) without varint or compact size into a fixed-size type.

I'll quote you every time someone on #bitcoin-dev says there is no One True Implementation.

As you should. Those people are to a first approximation wrong. @sipa With serialization at least a change in semantics will usually fail hard and immediately - hashes won't match. What I'd worry about more is subtle changes in semantics, e.g. overflows.

@petertodd And you should worry about that. But replacing types by typedef'ed identical types shouldn't cause problems.

It's easy enough to verify that the binary stays the same. I'm volunteering to check (for x86_32 and x86_64) once this is rebased.

@laanwj Doh! Yeah, that's the right way to do it; satisfies my concerns for sure.

Huh, changing even one "unsigned int" to uint64_t (I'm on x86_64) gives me a different binary hash.

Unsigned int is 32 bit.

Also, make sure to strip the binary before comparison.

@kdomanski you are thinking about long and unsigned long -- they are either 32-bit or 64-bit depending on the architecture being used. It wont be possible to change variables that are longs without changing the binary.

Nor is there any need. If there are types being serialized right now whose size depends on the architecture, Bitcoin just wouldn't work already. The purpose of this is to change the types that have an implicitly assumed size to types with that size fixed. It shouldn't change anything for any architecture, but may allow us to port to new architectures in the future for which these assumptions don't hold.

You have to take a few precautions to make sure that executables remain the same:

• Strip the executable (as said)
• Provide -frandom-seed=bitcoin, to make sure gcc uses a deterministic random seed
• Possibly define NDEBUG (assertion errors contain line numbers; not that these are expected to change in this case) - yes, bitcoin makes this difficult
• Empty out CLIENT_BUILD / CLIENT_DATE so that no differences are introduced based on the git hash, the build date or '-dirty'

I may have forgotten a few. Anyhow, as said I'm willing to do the checks if you rebase this.

@laanwj Done. If something doesn't work out, please post assembly diff.

ACK.

Preparation

Create the following patch stripbuildinfo.patch:

diff --git a/src/main.cpp b/src/main.cpp
index ba521b6..8cc6fb2 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -27,9 +27,9 @@
 using namespace std;
 using namespace boost;

-#if defined(NDEBUG)
-# error "Bitcoin cannot be compiled without assertions."
-#endif
+
+
+

 //
 // Global state
diff --git a/src/version.cpp b/src/version.cpp
index d86caa3..32dd4e9 100644
--- a/src/version.cpp
+++ b/src/version.cpp
@@ -67,5 +67,5 @@ const std::string CLIENT_NAME("Satoshi");
 #    endif
 #endif

-const std::string CLIENT_BUILD(BUILD_DESC CLIENT_VERSION_SUFFIX);
-const std::string CLIENT_DATE(BUILD_DATE);
+const std::string CLIENT_BUILD("");
+const std::string CLIENT_DATE("");

Create a script do_build.sh and chmod +x it:

#!/bin/bash
git reset --hard && \
git clean -f -x -d && \
git checkout $1 && \
git apply ../stripbuildinfo.patch && \
./autogen.sh && \
./configure --disable-wallet --without-gui --without-cli --disable-tests CPPFLAGS="-DNDEBUG -O2 -frandom-seed=bitcoin" && \
make -j3 src/bitcoind && cp src/bitcoind ../bitcoind.$1 && \
strip -o ../bitcoind.$1.stripped -R.note.gnu.build-id ../bitcoind.$1

Followed steps for each platform

### clone the git tree
# do this in a separate tree as the script will wipe everything inside it for sake of hygiene
git clone https://github.com/kdomanski/bitcoin.git bitcoin-compare
cd bitcoin-compare

### build both versions
../do_build.sh a43fde7
../do_build.sh 003bbd5

### comparison
sha256sum ../*.stripped

x86-64

(On a 64-bit Ubuntu 14.04 system) Output:

cd7a6023a386a1abf6dc67596c58f69c7ff7549e6d6c6a31cd7097e59a4bc929  ../bitcoind.003bbd5.stripped
cd7a6023a386a1abf6dc67596c58f69c7ff7549e6d6c6a31cd7097e59a4bc929  ../bitcoind.a43fde7.stripped

Result: OK

x86-32

(On a 32-bit Ubuntu 12.04 VM) Output:

c12d214aa09b8ea1d1ca4fd5600621fb79875650620eab1ca94f71f91068056d  ../bitcoind.003bbd5.stripped
c12d214aa09b8ea1d1ca4fd5600621fb79875650620eab1ca94f71f91068056d  ../bitcoind.a43fde7.stripped

Result: OK

ARM32

(Cubox-i Debian Jessie/Sid)

• Changed build.sh to use one thread (-j1) and added --with-boost-libdir=/usr/lib/arm-linux-gnueabihf to build flags Output:

947ceb723a99c766e99ec4a9f85769c63bb74300866f67168ccae6cb3dfeabd1  ../bitcoind.003bbd5.stripped
947ceb723a99c766e99ec4a9f85769c63bb74300866f67168ccae6cb3dfeabd1  ../bitcoind.a43fde7.stripped

Result: OK

@laanwj Nice work!

ACK

@laanwj I restored the prototypes.

Untested ACK

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4180_40f841d3e330b83d252e4cc2874256a4cb4c6641/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Merged via 9f3d476 (fixed trivial conflict: removal of print functions)