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

Hashing without memory allocations #69

Merged
merged 6 commits into from
Dec 31, 2024

Conversation

scnale
Copy link
Contributor

@scnale scnale commented Nov 6, 2024

This adds a few functions that allow the user to hash without extra memory allocations.

This doesn't include calldata variants because they would be pointless: the KECCAK256 instruction only operates with data in memory so a copy to memory would be necessary anyway.

@scnale scnale requested a review from nonergodic as a code owner November 6, 2024 14:12
@AgusVelez5
Copy link
Contributor

AgusVelez5 commented Nov 6, 2024

I got the idea that have the calldata variant is pointless but with the current implementation of keccak256SubarrayUnchecked and keccak256Subarray if we are working on the calldata, we will not be copying the whole bytes to memory?
Could there be a way to avoid that and copy only the necessary bytes, like making a slice, saving it in memory and then using it?
However, I am not sure it is worth do it.

@scnale
Copy link
Contributor Author

scnale commented Nov 6, 2024

Ah, you're right. It might be worth adding a calldata variant in that case.

@scnale
Copy link
Contributor Author

scnale commented Nov 7, 2024

Actually, I think it's not necessary. You can slice the calldata buffer and it'll copy just the slice, not the whole buffer.

Like this:

function test_calldataSlice(bytes calldata data) public {
        bytes calldata slice = data[5:data.length];
        bytes32 hash = slice.keccak256Subarray(0, slice.length);
}

@AgusVelez5
Copy link
Contributor

Yeah, that's true but the user should be aware of that. The common way we usually use the BytesParsingLib is doing the operations directly over the encoded buffer. Maybe if we offer the functions outside the lib, like utils makes you pay more attention.

@scnale
Copy link
Contributor Author

scnale commented Nov 7, 2024

I don't think we should add functions without good reason. Every new piece of code means more maintenance in the long run. Harder to read codebase, etc.

src/libraries/BytesParsing.sol Outdated Show resolved Hide resolved
@nonergodic nonergodic force-pushed the feature/hashing-without-memory-alloc-2 branch from b1edae2 to 63b9241 Compare December 28, 2024 00:02
@scnale
Copy link
Contributor Author

scnale commented Dec 29, 2024

When reviewing this PR after the latest changes I realized something: it's actually useful to have a calldata variant because we can use the space after the free memory pointer to copy the data, hash it and then return the result. The good thing about this is that we don't consume a fragment of the memory while respecting the memory model.

nonergodic
nonergodic previously approved these changes Dec 29, 2024
Copy link
Collaborator

@nonergodic nonergodic left a comment

Choose a reason for hiding this comment

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

Love it!

@nonergodic
Copy link
Collaborator

nonergodic commented Dec 29, 2024

One thought:
The current signature of function keccak256SliceCdUnchecked(bytes calldata encoded, uint offset, uint length) is in line with the memory version which is good.

Otoh, since slicing calldata is now cheap, one could reduce the signature to just function keccak256Cd(bytes calldata encoded) and rely on composition with BytesParsing.sliceCdUnchecked (using Solidity's [start:end] syntax would introduce checks).

Would have the small advantage that hashing a full bytes calldata is cleaner because you don't have to manually specify offset and length when you don't need it, and cheaper because you don't have to push it on the stack and have that offset add.

@scnale
Copy link
Contributor Author

scnale commented Dec 30, 2024

Hm, makes sense. I'll do that then.

@nonergodic nonergodic merged commit 5c24ed7 into main Dec 31, 2024
1 check passed
@nonergodic nonergodic deleted the feature/hashing-without-memory-alloc-2 branch December 31, 2024 07:12
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