Big endian support

laanwj commented at 1:02 pm on December 19, 2014: member

Fix issue #888.

This has been structured so that each compatibility change is one commit that touches only one file. After the initial build change, they are independent.

Most extensive changes are in ‘src/serialize.h: base serialization level endianness neutrality’. I had to replace READDATA and WRITEDATA with functions that take sized integer types to make use of the proper endian.h functions. I’m confident that the end result is the same, although this may require more tests.

I’ve tested this on mipsbe32.

All tests pass
Testnet syncs correctly
Node can successfully function on P2P mainnet
Checked that data directory can be copied between endians with no adverse results (only peers.dat required special attention here)

Known issues (to be fixed before merge):

~~DNS seeding always comes with 0 results on BE~~ (confirmed as working by @paveljanik on real hardware, must have been issue with my qemu-user setup)

laanwj force-pushed on Dec 19, 2014

gmaxwell commented at 4:25 am on December 21, 2014: contributor

I am pleased to report that using this patch I was able to sync from scratch over the network a PPC (debian) host. The old 2.3GHz cpu was pretty slow at verifying eventually taking several seconds per block.

""" { “chain”: “main”,
“blocks” : 335186,
“headers” : 335186,
“bestblockhash” : “000000000000000012ae1802369ba841a728ad8979376254bd9b5fb195d227d8”, “difficulty” : 39457671307.13873291, “verificationprogress” : 0.99999844, “chainwork” : “00000000000000000000000000000000000000000003796a2dda7d37a94cb1ff” } """

The whole process took about 33 hours.

theuni referenced this in commit 5dbc0fe712 on Dec 29, 2014

laanwj added the label Feature on Jan 2, 2015

laanwj added the label Priority Medium on Jan 2, 2015

laanwj force-pushed on Jan 5, 2015

paveljanik commented at 1:24 pm on January 6, 2015: contributor

Mac Mini PPC with 512MB RAM just finished testnet sync. Debian GNU/Linux 7.7. No issues so far. Even with few manual stops during the IBD. Generating new wallet was extremely slow… Few compile warnings for signed/unsigned and always true (I will investigate them later).

in src/arith_uint256.cpp: in 289706e9e5 outdated

 9@@ -10,6 +10,7 @@
10 
11 #include <stdio.h>
12 #include <string.h>
13+#include "crypto/common.h"

Diapolo commented at 2:24 pm on January 7, 2015:

I just want to know if we completly stop doing any include ordering/cleanup or even stop following the style we currently have in most files? If the new philosophy is new features before everything else I’m losing the last fun for this project :-/.

sipa commented at 4:30 pm on January 7, 2015:

I’m sorry to hear that you don’t find fun in many of the much more interesting things you’ve been doing before, but no - we removed this from the policy because the cost is just not worth the benefits. It requires too much time from maintainers and people writing patches for a very tiny (but non-zero) benefit, so we reverted it for that reason.

jgarzik commented at 4:51 pm on January 7, 2015:

@Diapolo Every code cleanup – including those authored by myself – has review costs and potentially disrupts more important functional work that actually changes behavior for the users.

Style takes a back seat to more practical changes, as it must. It sucks when it’s your changes.

Include file ordering is just not important enough to us to care about maintaining it in our own changes. We absolutely value your contributions, but you have to understand how to work with everyone else in the project and judge your impact on other developers, not just the codebase.

We don’t have rules, we have guidelines.

jonasschnelli commented at 12:10 pm on January 8, 2015:

Maybe this is the wrong place for this discussion. But better here then nowhere. IMO clean code leads to good software quality in general. Once the code gets a mess, the quality in general will follow. “A broken window is the start of a vagabonded house”. Thats why i honor @Diapolo work on one hand.

But: 1.) Nowadays, code-structure can be handled by tools like clang-format. There is no need for nits for spaces, indents, etc. Even if the clang-format output is not stable, i think a re-formatting from time-to-time over the whole code makes sense and will prevent us from thousands of nits and gives us more time to focus on the “real stuff”.

2.) Alphabetic ordering of includes just don’t make sense. Someone does it, someone not. I think we don’t need nit’s for this.

3.) Cleanup commits like #5616 are okay, but will break (requires rebase) all other and upcoming pull requests in this area. That’s why i would recommend to keep these kind of pull request to a minimum. Better provide a large cleanup commit once (at best in a code-quiet time). To much of cleanup-commit will slow down important work.

zander commented at 6:43 am on January 13, 2015:

Include ordering is there for a very good purpose, which is to ensure stand-alone headers. This purpose is just not really all that relevant in a project whose headers are not installed and exported for 3rd party use. Like a DLL based project would need it.

tl;dr dropping the requirement for header-inclusion-ordering makes sense to me.

theuni commented at 11:45 pm on January 7, 2015: member

utACK, nothing to complain about.

Do you have an idea where the DNS issue lies?

theuni commented at 0:05 am on January 8, 2015: member

Mmm.. on second thought, I suppose I’d first like to know the result of moving a fully-sync’d .bitcoin dir from a BE machine to a LE one, or the other way around.

laanwj commented at 7:30 am on January 8, 2015: member

@theuni As I mention in the OP, I’ve tested that, and it worked. UTXO databases and block databases are compatible between BE and LE.

I haven’t had time to look into the DNS seeding issue.

paveljanik commented at 7:45 am on January 8, 2015: contributor

DNS seeding works OK here on PPC Mac Mini:

 02015-01-08 07:42:38 init message: Loading addresses...
 12015-01-08 07:42:38 ERROR: Read : Failed to open file /home/bitcoin/.bitcoin/testnet3/peers.dat
 22015-01-08 07:42:38 Invalid or missing peers.dat; recreating
 32015-01-08 07:42:38 Loaded 0 addresses from peers.dat  43ms
 4...
 52015-01-08 07:42:38 Loading addresses from DNS seeds (could take a while)
 62015-01-08 07:42:38 ERROR: AcceptToMemoryPool : inputs already spent
 72015-01-08 07:42:38 ERROR: AcceptToMemoryPool : inputs already spent
 82015-01-08 07:42:48 Added 27 addresses from 95.85.12.7: 0 tried, 27 new
 92015-01-08 07:42:48 Added 6 addresses from ::: 0 tried, 33 new
102015-01-08 07:42:48 Added 1 addresses from ::: 0 tried, 34 new
112015-01-08 07:42:49 Added 28 addresses from ::: 0 tried, 62 new
122015-01-08 07:42:49 65 addresses found from DNS seeds
132015-01-08 07:42:49 dnsseed thread exit

paveljanik commented at 7:46 am on January 8, 2015: contributor

I’m moving the bitcoin directory there and back between LE and BE host without issues so far.

theuni commented at 9:13 pm on January 8, 2015: member

@laanwj Ah, I missed that part in the description. Thanks for verifying. @paveljanik Thanks to you as well.

laanwj force-pushed on Jan 14, 2015

laanwj commented at 9:13 am on January 14, 2015: member

Had to rebase over the openssl fix (no code changes or conflicts) to make tests work again.

laanwj renamed this:
~~WIP: Big endian support~~
Big endian support
on Jan 14, 2015

laanwj commented at 10:08 am on January 14, 2015: member

Added the missing float/double serialization tests and removed the WIP tag.

laanwj force-pushed on Jan 18, 2015

ry60003333 commented at 8:24 am on February 22, 2015: none

Everything seems to be working here as well! I am using a PowerMac G5 with Debian 7; I’ve been waiting for big endian support for a long time, and can’t wait until this is merged into the main release!

sipa commented at 8:40 am on February 22, 2015: member

Untested ACK.

We do have a few special-cased pieces of code for little-endian vs generic. Do we need all of them? I would suggest to either:

get rid of all WORDS_BIGENDIAN ifdefs (except in compat), and use the generic code everywhere
have a mechanism to disable optimized little-endian code at compile-time, and do at least one automated test with generic code.

ry60003333 commented at 3:20 pm on February 24, 2015: none

If any core developers would like, I can provide full SSH access to a Debian 7 PowerPC system for testing purposes to make sure future builds work on big endian systems. I would love for this to be integrated in a 0.10 release.

sipa commented at 11:18 am on March 1, 2015: member

@laanwj I would be fine with just getting rid of all LE optimized code versions here, and later benchmark whether any are worth it to add back.

build: Endian compatibility

- Detect endian instead of stopping configure on big-endian
- Add `byteswap.h` and `endian.h` header for compatibility with
  Windows and other operating systems that don't come with them
- Update `crypto/common.h` functions to use compat
  endian header

4414f5ffe1

src/hash.cpp: endian compatibility 3ca5852dc2

laanwj commented at 3:02 pm on March 6, 2015: member

@ry60003333 This is too late for the 0.10 release, but can make it to 0.11 in july @sipa OK, fine with me, there are two cases of WORDS_BIGENDIAN left and neither of them seems to be in a performance critical place.

laanwj force-pushed on Mar 6, 2015

sipa commented at 3:59 pm on March 6, 2015: member

Untested ACK.

src/main.cpp: endian compatibility in packet checksum check 556814ec4e

src/net.cpp: endian compatibility in EndMessage dec84cae2a

src/primitives/block.cpp: endian compatibility in GetHash 81aeb28436

src/primitives/transaction.h: endian compatibility in serialization 4f92773f92

src/script/script.h: endian compatibility for PUSHDATA sizes 4e853aa163

src/serialize.h: base serialization level endianness neutrality

Serialization type-safety and endianness compatibility.

01f9c3449a

src/netbase.h: Fix endian in CNetAddr serialization

We've chosen to htons/ntohs explicitly on reading and writing
(I do not know why). But as READWRITE already does an endian swap
on big endian, this means the port number gets switched around,
which was what we were trying to avoid in the first place. So
to make this compatible, serialize it as FLATDATA.

aac3205375

src/arith_256.cpp: bigendian compatibility f4e6487219

src/txmempool.cpp: make numEntries a uint32_t

Don't ever serialize a size_t or long, their sizes are platform
dependent.

9f4fac98c4

Add serialize float/double tests 62b30f09ac

Replace CBlockHeader::GetHash with call to SerializeHash

Removes variability between LE and BE.
As suggested by @sipa.

a0ae79d775

laanwj force-pushed on Mar 6, 2015

laanwj merged this on Mar 6, 2015

laanwj closed this on Mar 6, 2015

laanwj referenced this in commit 7c3fbc34ae on Mar 6, 2015

ry60003333 commented at 9:38 pm on March 6, 2015: none

@laanwj 0.11 would work for me! I’ve been looking forward to being able to run full nodes on IBM POWER servers for a long time now!

in src/script/script.h: in a0ae79d775

13@@ -14,6 +14,7 @@
14 #include <string.h>
15 #include <string>
16 #include <vector>
17+#include "crypto/common.h"

Diapolo commented at 11:26 am on March 8, 2015:

So we are now also don’t try to differentiate the blocks by own headers, standard headers and library headers?

random-zebra referenced this in commit 0dedec8157 on May 17, 2020

MarcoFalke locked this on Sep 8, 2021

Big endian support #5510