Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Core] Big endian support #1554

Merged
merged 11 commits into from May 17, 2020
Merged

[Core] Big endian support #1554

merged 11 commits into from May 17, 2020

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented Apr 24, 2020 •
edited

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)

@random-zebra random-zebra added this to the 5.0.0 milestone Apr 24, 2020
@random-zebra random-zebra self-assigned this Apr 24, 2020
@random-zebra random-zebra added this to In Progress in perpetual updating PIVX Core to BTC Core via automation Apr 24, 2020
@random-zebra
src/hash.cpp: endian compatibility
fe2727b 
backports  bitcoin/bitcoin@3ca5852
@random-zebra
src/main.cpp: endian compatibility in packet checksum check
4d98939 
backports bitcoin/bitcoin@556814e
@random-zebra
src/netbase.h: Fix endian in CNetAddr serialization
1d84b73 
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.

backports bitcoin/bitcoin@aac3205
@random-zebra
src/net.cpp: endian compatibility in EndMessage
b6b4b3e 
backports bitcoin/bitcoin@dec84ca
@random-zebra
src/primitives/block.cpp: endian compatibility in GetHash
b5e5de6 
adapted from bitcoin/bitcoin@81aeb28
@random-zebra
src/primitives/transaction.h: endian compatibility in serialization
74f1699 
backports bitcoin/bitcoin@4f92773
@random-zebra
src/script/script.h: endian compatibility for PUSHDATA sizes
4ad83fe 
backports bitcoin/bitcoin@4e853aa
@random-zebra
src/serialize.h: base serialization level endianness neutrality
70c6606 
Serialization type-safety and endianness compatibility.

backports bitcoin/bitcoin@01f9c34
@random-zebra
src/txmempool.cpp: make numEntries a uint32_t
83c6d13 
Don't ever serialize a size_t or long, their sizes are platform
dependent.

backports bitcoin/bitcoin@9f4fac9
@random-zebra
Add serialize float/double tests
49a7716 
backports bitcoin/bitcoin@62b30f0
@random-zebra
Replace CBlockHeader::GetHash with call to SerializeHash
25224a8 
Removes variability between LE and BE.
As suggested by @sipa.

adapted from bitcoin/bitcoin@a0ae79d
@random-zebra random-zebra mentioned this pull request Apr 24, 2020
Merged
1 task
@furszy
Copy link

furszy commented May 4, 2020

Concept ACK, code looking good.

furszy
furszy approved these changes May 13, 2020
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Nice back port job 🤘 .

Everything working here, utACK 25224a8

@furszy furszy requested a review from Fuzzbawls May 16, 2020 23:41
Fuzzbawls
Fuzzbawls approved these changes May 17, 2020
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

utACK 25224a8

perpetual updating PIVX Core to BTC Core automation moved this from In Progress to Ready May 17, 2020
@random-zebra random-zebra merged commit 0dedec8 into PIVX-Project:master May 17, 2020
perpetual updating PIVX Core to BTC Core automation moved this from Ready to Done May 17, 2020
Fuzzbawls added a commit that referenced this pull request May 19, 2020
@Fuzzbawls
Merge #1557: [Core] Prevector
8dfc480 
a8b965e Explicitly initialize prevector _union (random-zebra)
bdd98e8 [Trivial] Add a comment on the use of prevector in script. (random-zebra)
bbaa6e3 Clarify prevector::erase and avoid swap-to-clear (random-zebra)
28a8435 prevector: assert successful allocation (random-zebra)
de41cb5 Only call clear on prevector if it isn't trivially destructible (random-zebra)
8784df3 Make CScript (and prevector) c++11 movable. (random-zebra)
83f9ac6 serialize: Deprecate `begin_ptr` / `end_ptr` (random-zebra)
c3ecc12 prevector: add C++11-like data() method (random-zebra)
5490aa0 test prevector::swap (random-zebra)
035760e prevector::swap: fix (unreached) data corruption (random-zebra)
0e71400 prevector: destroy elements only via erase() (random-zebra)
9811a68 [Core] Prevector type (random-zebra)

Pull request description:

  Based on:
  - [x] #1554

  This introduces `prevector<N, T>`, a new basic data type which is a fully API compatible drop-in replacement for `std::vector<T>` and uses it for the script, to reduce the considerable memory overhead of vectors in cases where they normally contain a small number of small elements.

  Original tests in Bitcoin showed a reduction in dbcache memory usage by **23%**, and made an initial sync **13%** faster.

  Backported from the following upstream's PRs, with only additional edits in the 2nd layer scripts (`masternode-payments`, `masternode-budget`) and in the unit tests (to address the differences in `insecure_rand`):

  - bitcoin#6914
  - bitcoin#7888
  - bitcoin#8850
  - bitcoin#9349
  - bitcoin#9505
  - bitcoin#9856
  - bitcoin#10534
  - bitcoin#11011
  - bitcoin#14028

  ~~NOTE: Updates to `memusage.h` needed as well, when #1531 is merged.~~

ACKs for top commit:
  furszy:
    Tested ACK a8b965e .
  Fuzzbawls:
    ACK a8b965e

Tree-SHA512: 203b660dd8d9f98780be660dae0ac951766ba438836bd6f0ec921e7749c1a84143f3b920fe49f2cc399f4aa7e763098f78746d3b6ff845bcf913bb13de984bc1
@random-zebra random-zebra deleted the 202004_bigendian branch September 24, 2020 00:28
This was referenced Aug 15, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@furszy furszy furszy approved these changes

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

Assignees

@random-zebra random-zebra

Labels
Upstream
Projects
perpetual updating PIVX Core to BTC Core
  
Done
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@random-zebra @furszy @Fuzzbawls