Skip to content

✨ toNibbles #1461

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

✨ toNibbles #1461

wants to merge 2 commits into from

Conversation

Vectorized
Copy link
Owner

Description

#1459

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Ran forge fmt?
  • Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

Comment on lines 209 to 210
mstore(add(o, add(i, i)),
and(0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f, or(shl(4, x), x)))

Choose a reason for hiding this comment

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

This method should respect each nibbles original height within it's expanded byte (e.g. hex"ABCD" -> hex"A00BC00D"). My bad, I wasn't clear enough in my issue.

This should fix it:

Suggested change
mstore(add(o, add(i, i)),
and(0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f, or(shl(4, x), x)))
let mask := 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f
mstore(add(o, add(i, i)), or(shl(8, and(x, not(mask))), and(x, mask)))

Copy link
Owner Author

Choose a reason for hiding this comment

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

wait… is the original optimism code the same?

Choose a reason for hiding this comment

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

Shit, no just confirmed your first implementation was correct. Was thinking of my private fork where I'm working on another optimization that reduces some logic down the line in MerkleTrie. Disregard the last commit, mbmb.

Co-authored-by: clandestine.eth <[email protected]>
Comment on lines +209 to +210
let mask := 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f
mstore(add(o, add(i, i)), or(shl(8, and(x, not(mask))), and(x, mask)))

Choose a reason for hiding this comment

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

Disregard bad suggestion:

Suggested change
let mask := 0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f
mstore(add(o, add(i, i)), or(shl(8, and(x, not(mask))), and(x, mask)))
mstore(add(o, add(i, i)),
and(0x0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f0f, or(shl(4, x), x)))

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.

2 participants