-
Notifications
You must be signed in to change notification settings - Fork 4
perf: use calldata for proof #58
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
base: feature/rlp
Are you sure you want to change the base?
perf: use calldata for proof #58
Conversation
|
| function _hashCalldata(uint256 offset, uint256 length) private pure returns (bytes32 hash) { | ||
| assembly ("memory-safe") { | ||
| calldatacopy(mload(0x40), offset, length) | ||
| hash := keccak256(mload(0x40), length) | ||
| } | ||
| } |
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.
Rather than adjusting the free memory pointer at the end we allow previous proofs (in memory) to be overwritten. Avoiding unnecessary memory expansion.
| function _calldataOffsetAndLength(bytes calldata proof) private pure returns (uint256 offset, uint256 length) { | ||
| assembly ("memory-safe") { | ||
| offset := proof.offset | ||
| length := proof.length | ||
| } | ||
| } |
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.
High level Solidity doesn't allow us to do this for whatever reason...
| if (keyIndex == 0 && !string(bytes.concat(keccak256(node.encoded))).equal(string(nodeId))) | ||
| return ProofError.INVALID_ROOT_HASH; // Root node must match root hash | ||
| if (node.encoded.length >= 32 && !string(bytes.concat(keccak256(node.encoded))).equal(string(nodeId))) | ||
| return ProofError.INVALID_LARGE_INTERNAL_HASH; // Large nodes are stored as hashes | ||
| if (!string(node.encoded).equal(string(nodeId))) return ProofError.INVALID_INTERNAL_NODE_HASH; // Small nodes must match directly | ||
| bytes32 nodeHash = _hashCalldata(node.offset, node.length); | ||
| bool nodeHashMatches = string(bytes.concat(nodeHash)).equal(string(nodeId)); | ||
| if (keyIndex == 0 && !nodeHashMatches) return ProofError.INVALID_ROOT_HASH; // Root node must match root hash | ||
| if (node.length >= 32 && !nodeHashMatches) return ProofError.INVALID_LARGE_INTERNAL_HASH; // Large nodes are stored as hashes | ||
| if (!(nodeHash == keccak256(nodeId))) return ProofError.INVALID_INTERNAL_NODE_HASH; // Small nodes must match directly |
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 a variable avoids subsequent branches from expanding memory further.
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.
Also I think my updated L150 is functionally equivalent just avoids loading the proof into memory again.
| uint256 offset; // Raw RLP encoded node calldata offset | ||
| uint256 length; // Raw RLP encoded node calldata length |
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.
Actually makes more sense to simply store the proof elements index.
Fixes #????
Likely doesn't follow style guidelines, but I wanted to better communicate the idea. Proofs are always provided by a caller as calldata, thus we should use calldata to avoid memory expansion. Proving an account alone with optimisms SecureMerkleTrie implementation is over 400k gas for reference.
PR Checklist
npx changeset add)