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

feat!: Convert buffer usage to Uint8Array #306

Merged
merged 9 commits into from
Jan 25, 2025

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Jan 13, 2025

Fixes #303

Problems:

  1. The enr packages references globalThis.Buffer in the base64 conversion utilities and this causes segmentation faults in the Ultralight hive tests
  2. Buffer was only used for bytes (instead of Uint8Array) because of the requirements of bcrypto which only accepted Buffer as input.
  3. The bigint-buffer package causes CI failures in Ultralight with arbitrary napi-ok === false errors that are indecipherable

Solution:

  • Remove all Buffer types and utilities and replace with equivalent Uint8Arrays
  • Switch toHex/fromHex usage to hexToBytes/bytesToHex from ethereum-cryptography
  • Switch all utf8 conversion to ethereum-cryptography utf8ToBytes/bytesToUtf8 utilities
  • Remove all references to globalThis.buffer
  • Update base64 utilities conversion to use scure/base (author same as @noble/hashes)
  • Remove bigint-buffer and replace with internal bigIntToBytes/bytesToBigInt conversion utilities

@acolytec3 acolytec3 changed the title Convert buffer usage to Uint8Array chore: Convert buffer usage to Uint8Array Jan 13, 2025
@acolytec3 acolytec3 marked this pull request as ready for review January 13, 2025 19:54
@acolytec3 acolytec3 requested a review from a team as a code owner January 13, 2025 19:54
@acolytec3
Copy link
Contributor Author

@wemeetagain Hopefully this isn't too far beyond what you had in mind with regard to #303. Removing buffers provided an opportunity to fix 2 other pain points for Ultralight. If the bigint-buffer performance drop you'll see from going over to the the bigint -> hex -> butes route is a concern, I'd be happy to look for ways to improve the performance though I think @paulm has done a pretty thorough job of squeezing all the performance he can out of that conversion process.

@acolytec3
Copy link
Contributor Author

@wemeetagain bump

@wemeetagain wemeetagain changed the title chore: Convert buffer usage to Uint8Array feat!: Convert buffer usage to Uint8Array Jan 23, 2025
}

export function numberToBuffer(value: number, length: number): Buffer {
export function numberToBuffer(value: number, length: number): Uint8Array {
Copy link
Member

Choose a reason for hiding this comment

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

Is this function still used? maybe delete it or refactor the usage of Buffer here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Not sure how I missed that but will do.

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

Generally LGTM, thanks for further simplifying the codebase.

Just had one small comment

@acolytec3
Copy link
Contributor Author

@wemeetagain should be ready for final review now.

@wemeetagain wemeetagain merged commit 1cdc424 into ChainSafe:master Jan 25, 2025
6 checks passed
@github-actions github-actions bot mentioned this pull request Jan 25, 2025
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.

Buffer usage
2 participants