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

fix: swap bytes for as_le_bytes in big endian world #431

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

pcw109550
Copy link
Contributor

@pcw109550 pcw109550 commented Feb 18, 2025

Motivation

Was tweaking https://github.com/op-rs/kona which has this repo as a dependency. Compiled kona down to mips64 which is big endian, and spotted this bug while comparing the results with little endian arch.

Minimal reproduction:

use alloy_primitives::U256;
use alloc::vec::Vec;
use alloy_rlp::Encodable;
...
    let mut out = Vec::<u8>::new();
    U256::from(0x12345678).encode(&mut out); // rlp encodes num

When we print out variable...

  • in little endian, [132, 18, 52, 86, 120], which is correct
  • in big endian, [132, 72, 44, 106, 30], which is wrong.
    both results must be identical.

Solution

Must swap bytes for big endian architectures, not reversing bits.

PR Checklist

  • Added Tests
    • Unfortunately, no. We may add test suite or ci pipeline for big endian targets
  • Added Documentation
  • Updated the changelog

@pcw109550 pcw109550 requested a review from prestwich as a code owner February 18, 2025 07:54
@pcw109550 pcw109550 changed the title fix: swap bytes for as_le_bytes fix: swap bytes for as_le_bytes in big endian world Feb 18, 2025
Copy link
Contributor

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Right, sorry about that, we don't test the library with big endian so there are likely more of these bugs

@prestwich prestwich enabled auto-merge February 18, 2025 12:46
@prestwich
Copy link
Collaborator

i can cut a release later today

@prestwich prestwich merged commit 90aa19b into recmo:main Feb 18, 2025
17 checks passed
@prestwich
Copy link
Collaborator

this was included in v1.13.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants