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.

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

Serialization of time classes in fc is architecture-dependent #1196

Closed
1 of 17 tasks
pmconrad opened this issue Jul 28, 2018 · 6 comments
Closed
1 of 17 tasks

Serialization of time classes in fc is architecture-dependent #1196

pmconrad opened this issue Jul 28, 2018 · 6 comments
Assignees
Labels
2f Testing Status indicating the solution is being evaluated against the test case(s) 3d Bug Classification indicating the existing implementation does not match the intention of the design 4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists 6 P2P Impact flag identifying the peer-to-peer (P2P) layer 9a Tiny Effort estimation indicating TBD

Comments

@pmconrad
Copy link
Contributor

Bug Description
The (binary) serialization of fc::time_point, fc::time_point_sec and fc::microseconds is architecture-dependent (big-endian vs. little-endian).

Impacts
Describe which portion(s) of BitShares Core may be impacted by this bug. Please tick at least one box.

  • API (the application programming interface)
  • Build (the build process or something prior to compiled code)
  • CLI (the command line wallet)
  • Deployment (the deployment process after building such as Docker, Travis, etc.)
  • DEX (the Decentralized EXchange, market engine, etc.)
  • P2P (the peer-to-peer network for transaction/block propagation)
  • Performance (system or user efficiency, etc.)
  • Protocol (the blockchain logic, consensus, validation, etc.)
  • Security (the security of system or user data, etc.)
  • UX (the User Experience)
  • Other (please add below)

Within our x86-dominated ecosystem the bug has no effects. However, it will be impossible to sync a node on a big-endian machine.

Steps To Reproduce
n/a

Expected Behavior
Serialization should be architecture-independent.

Host Environment
all

Additional context
https://github.com/bitshares/bitshares-fc/blob/master/include/fc/io/raw.hpp#L93-L132

CORE TEAM TASK LIST

  • Evaluate / Prioritize Bug Report
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@pmconrad pmconrad added 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 3d Bug Classification indicating the existing implementation does not match the intention of the design 6 P2P Impact flag identifying the peer-to-peer (P2P) layer 9a Tiny Effort estimation indicating TBD labels Jul 28, 2018
@pmconrad
Copy link
Contributor Author

Might be a good idea to scan for similar problems in other areas.

@clockworkgr
Copy link
Member

Are there many/any big-endian machines left? apart from IBM mainframes?

@ryanRfox ryanRfox added the 4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists label Feb 12, 2019
@jmjatlanta
Copy link
Contributor

Anyone have a spare mainframe linux partition to test this with? I'd love to port and see some performance numbers on some recent IBM power or z hardware.

@pmconrad
Copy link
Contributor Author

More candidates: all the hash classes in fc::crypto use uint32 or uint64 arrays as a buffer for storing the hash. This is mostly fine, but sometimes an element of these buffers is accessed directly, which leads to architecture-dependent results. See sha256::hash64().

The way the block number is stored in the block ID attempts to be clean, see https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/protocol/block.cpp#L43 . However, fc::endian_reverse_* reverses the byte order always, not in an architecture-dependent way... https://github.com/bitshares/bitshares-fc/blob/master/include/fc/bitutil.hpp

TODO: check all usages of fc::endian_reverse_*

@christophersanborn
Copy link
Member

Are there many/any big-endian machines left? apart from IBM mainframes?

Apparently ARM can run in either little- or big-endian mode, according to this quote from here:

"The ARM architecture was little-endian before version 3. Since then ARM processors became BI-endian and feature a setting which allows for switchable endianness."

I don't know how easily "switchable" it is, like if it's a start-up option determined in essence by the OS, or if programs can switch on-the-fly,... but if somebody wants to run a personal access node on a Raspberry Pi, or maybe someday as an applet on their WiFi router, then this issue may have some relevance.

@pmconrad pmconrad self-assigned this Mar 1, 2019
@pmconrad pmconrad added 2f Testing Status indicating the solution is being evaluated against the test case(s) and removed 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements labels Apr 13, 2019
@pmconrad
Copy link
Contributor Author

Resolved by #1714

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2f Testing Status indicating the solution is being evaluated against the test case(s) 3d Bug Classification indicating the existing implementation does not match the intention of the design 4a Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists 6 P2P Impact flag identifying the peer-to-peer (P2P) layer 9a Tiny Effort estimation indicating TBD
Projects
None yet
Development

No branches or pull requests

6 participants