Big endian support #5510

laanwj · 2014-12-19T13:02:51Z

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)

gmaxwell · 2014-12-21T04:25:54Z

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.

paveljanik · 2015-01-06T13:24:10Z

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

Diapolo · 2015-01-07T14:24:55Z

src/arith_uint256.cpp

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 :-/.

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.

@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.

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.

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 · 2015-01-07T23:45:05Z

utACK, nothing to complain about.

Do you have an idea where the DNS issue lies?

theuni · 2015-01-08T00:05:52Z

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 · 2015-01-08T07:30:08Z

@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 · 2015-01-08T07:45:13Z

DNS seeding works OK here on PPC Mac Mini:

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

paveljanik · 2015-01-08T07:46:17Z

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

theuni · 2015-01-08T21:13:03Z

@laanwj Ah, I missed that part in the description. Thanks for verifying.

@paveljanik Thanks to you as well.

laanwj · 2015-01-14T09:13:00Z

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

laanwj · 2015-01-14T10:08:26Z

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

Ry6000 · 2015-02-22T08:24:40Z

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 · 2015-02-22T08:40:04Z

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.

Ry6000 · 2015-02-24T15:20:47Z

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 · 2015-03-01T11:18:54Z

@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.

- 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

laanwj · 2015-03-06T15:02:43Z

@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.

sipa · 2015-03-06T15:59:19Z

Untested ACK.

Serialization type-safety and endianness compatibility.

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.

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

@sipa

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

a0ae79d Replace CBlockHeader::GetHash with call to SerializeHash (Wladimir J. van der Laan) 62b30f0 Add serialize float/double tests (Wladimir J. van der Laan) 9f4fac9 src/txmempool.cpp: make numEntries a uint32_t (Wladimir J. van der Laan) f4e6487 src/arith_256.cpp: bigendian compatibility (Wladimir J. van der Laan) aac3205 src/netbase.h: Fix endian in CNetAddr serialization (Wladimir J. van der Laan) 01f9c34 src/serialize.h: base serialization level endianness neutrality (Wladimir J. van der Laan) 4e853aa src/script/script.h: endian compatibility for PUSHDATA sizes (Wladimir J. van der Laan) 4f92773 src/primitives/transaction.h: endian compatibility in serialization (Wladimir J. van der Laan) 81aeb28 src/primitives/block.cpp: endian compatibility in GetHash (Wladimir J. van der Laan) dec84ca src/net.cpp: endian compatibility in EndMessage (Wladimir J. van der Laan) 556814e src/main.cpp: endian compatibility in packet checksum check (Wladimir J. van der Laan) 3ca5852 src/hash.cpp: endian compatibility (Wladimir J. van der Laan) 4414f5f build: Endian compatibility (Wladimir J. van der Laan)

Ry6000 · 2015-03-06T21:38:32Z

@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!

Diapolo · 2015-03-08T11:26:59Z

src/script/script.h

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

@paveljanik

25224a8 Replace CBlockHeader::GetHash with call to SerializeHash (random-zebra) 49a7716 Add serialize float/double tests (random-zebra) 83c6d13 src/txmempool.cpp: make numEntries a uint32_t (random-zebra) 70c6606 src/serialize.h: base serialization level endianness neutrality (random-zebra) 4ad83fe src/script/script.h: endian compatibility for PUSHDATA sizes (random-zebra) 74f1699 src/primitives/transaction.h: endian compatibility in serialization (random-zebra) b5e5de6 src/primitives/block.cpp: endian compatibility in GetHash (random-zebra) b6b4b3e src/net.cpp: endian compatibility in EndMessage (random-zebra) 1d84b73 src/netbase.h: Fix endian in CNetAddr serialization (random-zebra) 4d98939 src/main.cpp: endian compatibility in packet checksum check (random-zebra) fe2727b src/hash.cpp: endian compatibility (random-zebra) Pull request description: First step in getting our serialization code up to par with upstream. Backports bitcoin#5510 with only minor adjustments. > Fix issue bitcoin#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) ACKs for top commit: furszy: Everything working here, utACK 25224a8 Fuzzbawls: utACK 25224a8 Tree-SHA512: 047e3bd6a7c6bbbd341da1ec3fb7698e19d52e170ba0d475e6f451cbb52e2c55f23a15999da663a8022ddc9df3a5c168f825b779c4afb1fd210b9da680503297

laanwj force-pushed the 2014_12_bigendian branch 2 times, most recently from 23b1244 to db2bac8 Compare December 19, 2014 13:42

laanwj added Feature Priority Medium labels Jan 2, 2015

laanwj force-pushed the 2014_12_bigendian branch from c32fbeb to 289706e Compare January 5, 2015 16:39

Diapolo reviewed Jan 7, 2015
View reviewed changes

laanwj force-pushed the 2014_12_bigendian branch from 289706e to b7af281 Compare January 14, 2015 09:09

laanwj changed the title ~~WIP: Big endian support~~ Big endian support Jan 14, 2015

laanwj force-pushed the 2014_12_bigendian branch from 9283a50 to 0ed3fdc Compare January 18, 2015 18:26

laanwj added 2 commits March 6, 2015 15:54

build: Endian compatibility

4414f5f

- 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

src/hash.cpp: endian compatibility

3ca5852

laanwj force-pushed the 2014_12_bigendian branch 2 times, most recently from 60b9a58 to f16d769 Compare March 6, 2015 15:44

laanwj added 11 commits March 6, 2015 17:21

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

556814e

src/net.cpp: endian compatibility in EndMessage

dec84ca

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

81aeb28

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

4f92773

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

4e853aa

src/serialize.h: base serialization level endianness neutrality

01f9c34

Serialization type-safety and endianness compatibility.

src/arith_256.cpp: bigendian compatibility

f4e6487

src/txmempool.cpp: make numEntries a uint32_t

9f4fac9

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

Add serialize float/double tests

62b30f0

Replace CBlockHeader::GetHash with call to SerializeHash

a0ae79d

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

laanwj force-pushed the 2014_12_bigendian branch from f16d769 to a0ae79d Compare March 6, 2015 16:22

laanwj merged commit a0ae79d into bitcoin:master Mar 6, 2015

Diapolo reviewed Mar 8, 2015
View reviewed changes

src/script/script.h

Copy link

Diapolo Mar 8, 2015

Choose a reason for hiding this comment

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

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

laanwj mentioned this pull request Jul 21, 2015

No support for bigendian machines #888

Closed

Erkan-Yilmaz mentioned this pull request Sep 29, 2016

0.11 can not download blocks - on big endian machine myriadteam/myriadcoin#32

Closed

random-zebra mentioned this pull request Apr 24, 2020

[Core] Big endian support PIVX-Project/PIVX#1554

Merged

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

Big endian support #5510

Big endian support #5510

Uh oh!

Conversation

laanwj commented Dec 19, 2014

Uh oh!

gmaxwell commented Dec 21, 2014

Uh oh!

paveljanik commented Jan 6, 2015

Uh oh!

Diapolo Jan 7, 2015

Choose a reason for hiding this comment

Uh oh!

sipa Jan 7, 2015

Choose a reason for hiding this comment

Uh oh!

jgarzik Jan 7, 2015

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Jan 8, 2015

Choose a reason for hiding this comment

Uh oh!

zander Jan 13, 2015

Choose a reason for hiding this comment

Uh oh!

theuni commented Jan 7, 2015

Uh oh!

theuni commented Jan 8, 2015

Uh oh!

laanwj commented Jan 8, 2015

Uh oh!

paveljanik commented Jan 8, 2015

Uh oh!

paveljanik commented Jan 8, 2015

Uh oh!

theuni commented Jan 8, 2015

Uh oh!

laanwj commented Jan 14, 2015

Uh oh!

laanwj commented Jan 14, 2015

Uh oh!

Ry6000 commented Feb 22, 2015

Uh oh!

sipa commented Feb 22, 2015

Uh oh!

Ry6000 commented Feb 24, 2015

Uh oh!

sipa commented Mar 1, 2015

Uh oh!

laanwj commented Mar 6, 2015

Uh oh!

sipa commented Mar 6, 2015

Uh oh!

Ry6000 commented Mar 6, 2015

Uh oh!

Diapolo Mar 8, 2015

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants