-
Notifications
You must be signed in to change notification settings - Fork 12.1k
Add new clz(bytes)
and clz(uint256)
functions
#5725
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
42c79f1
Add new equal, nibbles and countLeadingZeroes functions
ernestognw 5754ab8
Rename countLeadingZeroes to clz
ernestognw 4383e01
Merge branch 'master' into feat/bytes-rlp
ernestognw 5a44b11
up
ernestognw d6db2d7
Document
ernestognw 9b8af8f
Merge branch 'master' into feat/bytes-rlp
ernestognw 2cf0acf
up
ernestognw 8116913
Merge branch 'master' into feat/bytes-rlp
Amxx c5426dd
Merge branch 'master' into feat/bytes-rlp
ernestognw 1dfee41
up
ernestognw ab00bb1
up
ernestognw e6092ae
Add clz(bytes)
ernestognw ede80e9
up
ernestognw d65d7a5
Minimize diff
ernestognw fb91c85
up
ernestognw c749346
Change CLZ behavior to count zero bits instead of zero bytes
Amxx 12c87dd
Update Bytes.sol
Amxx 740a056
fix Math tests
Amxx 34e0783
Remove nibbles
ernestognw 93a4eed
Update changesets
ernestognw 82ca12b
Update contracts/utils/math/Math.sol
ernestognw 56da3b5
Update contracts/utils/Bytes.sol
ernestognw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'openzeppelin-solidity': minor | ||
--- | ||
|
||
`Bytes`: Add a `nibbles` function to split each byte into two nibbles. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'openzeppelin-solidity': minor | ||
--- | ||
|
||
`Bytes`: Add an `equal` function to compare byte buffers. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'openzeppelin-solidity': minor | ||
--- | ||
|
||
`Bytes`: Add a `clz` function to count the leading zero bytes in a `uint256` value. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
pragma solidity ^0.8.20; | ||
|
||
import {Test} from "forge-std/Test.sol"; | ||
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; | ||
import {Bytes} from "@openzeppelin/contracts/utils/Bytes.sol"; | ||
|
||
contract BytesTest is Test { | ||
function testIndexOf(bytes memory buffer, bytes1 s) public pure { | ||
testIndexOf(buffer, s, 0); | ||
} | ||
|
||
function testIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure { | ||
uint256 result = Bytes.indexOf(buffer, s, pos); | ||
|
||
// Should not be found before result | ||
for (uint256 i = pos; result != type(uint256).max && i < result; i++) assertNotEq(buffer[i], s); | ||
if (result != type(uint256).max) assertEq(buffer[result], s); | ||
} | ||
|
||
function testLastIndexOf(bytes memory buffer, bytes1 s) public pure { | ||
testLastIndexOf(buffer, s, 0); | ||
} | ||
|
||
function testLastIndexOf(bytes memory buffer, bytes1 s, uint256 pos) public pure { | ||
pos = bound(pos, 0, buffer.length); | ||
uint256 result = Bytes.lastIndexOf(buffer, s, pos); | ||
|
||
// Should not be found before result | ||
for (uint256 i = pos; result != type(uint256).max && i < result; i++) assertNotEq(buffer[i], s); | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (result != type(uint256).max) assertEq(buffer[result], s); | ||
} | ||
|
||
function testSlice(bytes memory buffer, uint256 start) public pure { | ||
testSlice(buffer, start, buffer.length); | ||
} | ||
|
||
function testSlice(bytes memory buffer, uint256 start, uint256 end) public pure { | ||
bytes memory result = Bytes.slice(buffer, start, end); | ||
uint256 sanitizedEnd = Math.min(end, buffer.length); | ||
uint256 sanitizedStart = Math.min(start, sanitizedEnd); | ||
assertEq(result.length, sanitizedEnd - sanitizedStart); | ||
for (uint256 i = 0; i < result.length; i++) assertEq(result[i], buffer[sanitizedStart + i]); | ||
} | ||
|
||
function testNibbles(bytes memory value) public pure { | ||
bytes memory result = Bytes.nibbles(value); | ||
assertEq(result.length, value.length * 2); | ||
for (uint256 i = 0; i < value.length; i++) { | ||
bytes1 originalByte = value[i]; | ||
bytes1 highNibble = result[i * 2]; | ||
bytes1 lowNibble = result[i * 2 + 1]; | ||
|
||
assertEq(highNibble, originalByte & 0xf0); | ||
assertEq(lowNibble, originalByte & 0x0f); | ||
} | ||
} | ||
|
||
function testSymbolicEqual(bytes memory a, bytes memory b) public pure { | ||
assertEq(Bytes.equal(a, b), Bytes.equal(a, b)); | ||
} | ||
|
||
function testSymbolicCountLeadingZeroes(uint256 x) public pure { | ||
uint256 result = Bytes.clz(x); | ||
assertLe(result, 32); // [0, 32] | ||
|
||
if (x != 0) { | ||
uint256 firstNonZeroBytePos = 32 - result - 1; | ||
uint256 byteValue = (x >> (firstNonZeroBytePos * 8)) & 0xff; | ||
assertNotEq(byteValue, 0); | ||
|
||
// x != 0 implies result < 32 | ||
// most significant byte should be non-zero | ||
uint256 msbValue = (x >> (248 - result * 8)) & 0xff; | ||
assertNotEq(msbValue, 0); | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty inefficient, consider using
unchecked
at minimum.Also unclear why this is useful:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
unchecked
would be safe since overflow is impossible givenbytes memory
can realistically only have a length smaller than 256 bits.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd fix tests first and then optimize, it's likely that we may leverage other functions of the Bytes library
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial motivation for this function is to use it in #5680 for TrieProof, but I agree that reallocating memory is perhaps not the most efficient thing to do. I suspect TrieProof may not require the
nibbles()
function if these are read in place, but I need to experiment a bit with that.We can add the unchecked, but, is there an alternative you were thinking of? I am more worried about the memory expansion cost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if inspecting the nibbles JIT rather than converting to separate nibbles array is worthwhile to explore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In regard to memory expansion, we need calldata variants for all methods related to MPT proofs (both RLP decoding and MPT branch verification) since the input data is almost always provided by the user via calldata. There's rarely a reason to copy the full RLP payload into memory, because typical use cases (like verifying a historical blockhash) only require extracting one or two fields (e.g. stateRoot, txRoot). Operating directly on calldata avoids unnecessary memory allocation and is significantly more gas-efficient. Comparing my personal implementation to
RLPReader
I'm saving about 40k gas when parsing every block header field (which should never really be needed but serves as a good gas comparison and unit test).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the nibbles method it may make sense to first check the size of the input, if less than or equal to 32 bytes the above calculation could be much simpler (no loop needed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback @0xClandestine!
We discussed it internally and we agreed to remove the
nibbles
function for now since it doesn't seem to be useful outside of the context of MPT. I'll reimplement the function privately in MPT