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

optimize getTransactionHash by implementing it in assembly #847

Merged
merged 6 commits into from
Oct 28, 2024

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Oct 18, 2024

This pull request includes significant changes to the Safe contract and its associated test suite. The changes focus on optimizing the encoding of transaction data and enhancing the test coverage for transaction hash calculations.

Optimizations in Safe contract:

  • contracts/Safe.sol: Rewrote the transaction data encoding logic in assembly to avoid multiple memory allocations, improving gas efficiency.

Enhancements in test suite:

  • test/core/Safe.Signatures.spec.ts: Enhanced the test case for calculating EIP-712 hash by introducing a loop to generate and test multiple random transactions. [1] [2]. The previous test case was inefficient as it contained empty safe transaction data. The test would still pass if you forgot to include it in hashing.
  • I also added a FV rule to verify hash computation correctness.

Benchmarks

Before

  ERC20 - transfer
           Used 51800n gas for >transfer<
    ✔ with an EOA (137ms)
           Used 82980n gas for >transfer<
    ✔ with a single owner Safe
           Used 88874n gas for >transfer<
    ✔ with a single owner and guard Safe
           Used 90024n gas for >transfer<
    ✔ with a 2 out of 2 Safe
           Used 97094n gas for >transfer<
    ✔ with a 3 out of 3 Safe
           Used 97094n gas for >transfer<
    ✔ with a 3 out of 5 Safe

After

ERC20 - transfer
           Used 51800n gas for >transfer<
    ✔ with an EOA (71ms)
           Used 82494n gas for >transfer<
    ✔ with a single owner Safe
           Used 88375n gas for >transfer<
    ✔ with a single owner and guard Safe
           Used 89547n gas for >transfer<
    ✔ with a 2 out of 2 Safe
           Used 96577n gas for >transfer<
    ✔ with a 3 out of 3 Safe
           Used 96589n gas for >transfer<
    ✔ with a 3 out of 5 Safe

On average, it saves ~485 gas, not much, but considering this is the hottest path, it should result in significant accumulated savings. (After 44 Safe transactions, a user would save 21k gas - enough for broadcasting a native token transfer)

Codesize

It saves 273 bytes in code size.

Before

SafeL2 22582 bytes (limit is 24576)

After

SafeL2 22309 bytes (limit is 24576)

@mmv08 mmv08 force-pushed the assembly-encodeSafeTransactionData branch 2 times, most recently from be08482 to da51b2d Compare October 18, 2024 20:04
@mmv08 mmv08 requested review from gjeanmart, a team, nlordell, akshay-ap and remedcu and removed request for a team October 18, 2024 20:10
@mmv08 mmv08 force-pushed the assembly-encodeSafeTransactionData branch from da51b2d to 9d2ea5b Compare October 18, 2024 20:13
@mmv08 mmv08 force-pushed the assembly-encodeSafeTransactionData branch from 9d2ea5b to 6fd853b Compare October 18, 2024 20:46
// we can save gas by reusing the same memory block by overwriting the data.
/* solhint-disable no-inline-assembly */
assembly {
let freeMemoryPointer := mload(0x40)
Copy link
Member Author

@mmv08 mmv08 Oct 18, 2024

Choose a reason for hiding this comment

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

random fact: renaming this variable increases gas by 12 units? weird

edit: probably because the contract metadata hash appended to the bytecode is different, and the target address contains one more non-zero byte

Copy link
Collaborator

Choose a reason for hiding this comment

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

We had "mined" an extra 0-byte in the data hash :P.

I wonder if we can mine a more optimal metadata hash by randomizing characters in a comment at the end of a file? Would be a fun thought experiment 😛

contracts/Safe.sol Outdated Show resolved Hide resolved
contracts/Safe.sol Outdated Show resolved Hide resolved
@mmv08 mmv08 requested a review from nlordell October 21, 2024 10:23
contracts/Safe.sol Outdated Show resolved Hide resolved
contracts/Safe.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

This translates to a 0.3% saving, so I'm not 100% sure it is worth the optimization to be honest.

That being said, it is fairly simple and there is no additional control flow introduced in the assembly, which makes me lean on the "optimization is slightly worth it" side of the fence.

Its really annoying that Solidity fails to generate reasonable code for these simple cases (as in... why on Earth does it unconditionally allocate intermediate "rvalues" (to borrow from C/C++ lingo)???).

@mmv08 mmv08 force-pushed the assembly-encodeSafeTransactionData branch from a90fdb8 to 16b5924 Compare October 21, 2024 11:51
@mmv08
Copy link
Member Author

mmv08 commented Oct 21, 2024

This translates to a 0.3% saving, so I'm not 100% sure it is worth the optimization to be honest.

That being said, it is fairly simple and there is no additional control flow introduced in the assembly, which makes me lean on the "optimization is slightly worth it" side of the fence.

Its really annoying that Solidity fails to generate reasonable code for these simple cases (as in... why on Earth does it unconditionally allocate intermediate "rvalues" (to borrow from C/C++ lingo)???).

I tried to optimize it further and removed the encodeTransactionData method altogether (I didn't adjust the comments):

    function getTransactionHash(
        address to,
        uint256 value,
        bytes calldata data,
        Enum.Operation operation,
        uint256 safeTxGas,
        uint256 baseGas,
        uint256 gasPrice,
        address gasToken,
        address refundReceiver,
        uint256 _nonce
    ) public view override returns (bytes32 txHash) {
        bytes32 separatorHash = domainSeparator();
        assembly {
            let ptr := mload(0x40)

            calldatacopy(ptr, data.offset, data.length)
            let calldataHash := keccak256(ptr, data.length)

            // Prepare the SafeTX struct for hashing, overwriting the copied calldata
            mstore(ptr, SAFE_TX_TYPEHASH)
            mstore(add(ptr, 32), to)
            mstore(add(ptr, 64), value)
            mstore(add(ptr, 96), calldataHash)
            mstore(add(ptr, 128), operation)
            mstore(add(ptr, 160), safeTxGas)
            mstore(add(ptr, 192), baseGas)
            mstore(add(ptr, 224), gasPrice)
            mstore(add(ptr, 256), gasToken)
            mstore(add(ptr, 288), refundReceiver)
            mstore(add(ptr, 320), _nonce)

            // Hash the SafeTX struct and store it at the end of the result
            // Hashing first so we can re-use the same memory block for the result
            mstore(add(ptr, 34), keccak256(ptr, 352))
            // Store the EIP-712 prefix (0x1901), note that strings are right-padded.
            // We write it before the domain separator and hash to use the remaining space.
            mstore(ptr, "\x19\x01")
            // Store the domain separator
            mstore(add(ptr, 2), separatorHash)

            // Point the result to the memory block
            txHash := keccak256(ptr, 66)
        }
    }

Gas results:

 ERC20 - transfer
           Used 51800n gas for >transfer<
    ✔ with an EOA (76ms)
           Used 82482n gas for >transfer<
    ✔ with a single owner Safe
           Used 88375n gas for >transfer<
    ✔ with a single owner and guard Safe
           Used 89547n gas for >transfer<
    ✔ with a 2 out of 2 Safe
           Used 96589n gas for >transfer<
    ✔ with a 3 out of 3 Safe
           Used 96589n gas for >transfer<
    ✔ with a 3 out of 5 Safe

If that sounds better, I can adjust the PR

return keccak256(encodeTransactionData(to, value, data, operation, safeTxGas, baseGas, gasPrice, gasToken, refundReceiver, _nonce));
) public view override returns (bytes32 txHash) {
bytes32 separatorHash = domainSeparator();
/* solhint-disable no-inline-assembly */
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Can you add a comment here justifying the use of inline assembly? I think it makes sense to add these kinds of justifications so that auditors and other people reviewing the code can understand our rationale behind not just using plain Solidity for this.

Comment on lines 425 to 435
// Hash the SafeTX struct and store it at the end of the result
// Hashing first so we can re-use the same memory block for the result
mstore(add(ptr, 34), keccak256(ptr, 352))
// Store the EIP-712 prefix (0x1901), note that strings are right-padded.
// We write it before the domain separator and hash to use the remaining space.
mstore(ptr, "\x19\x01")
// Store the domain separator
mstore(add(ptr, 2), separatorHash)

// Calculate the hash
txHash := keccak256(ptr, 66)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for completeness, can we compare with something like this:

Suggested change
// Hash the SafeTX struct and store it at the end of the result
// Hashing first so we can re-use the same memory block for the result
mstore(add(ptr, 34), keccak256(ptr, 352))
// Store the EIP-712 prefix (0x1901), note that strings are right-padded.
// We write it before the domain separator and hash to use the remaining space.
mstore(ptr, "\x19\x01")
// Store the domain separator
mstore(add(ptr, 2), separatorHash)
// Calculate the hash
txHash := keccak256(ptr, 66)
// Hash the SafeTX struct and store it at the end of the result
// Hashing first so we can re-use the same memory block for the result
mstore(add(ptr, 64), keccak256(ptr, 352))
// Store the EIP-712 prefix (0x1901), note that integers are left-padded
// so the EIP-712 encoded data starts at add(ptr, 30).
mstore(ptr, 0x1901)
// Store the domain separator
mstore(add(ptr, 32), separatorHash)
// Calculate the hash
txHash := keccak256(add(ptr, 30), 66)

Copy link
Collaborator

@nlordell nlordell Oct 23, 2024

Choose a reason for hiding this comment

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

Rationale: Solidity won't emit a push32 0x19010000..00 but instead a push2 0x1901 at the cost of an extra addition. Just want to see if we prefer code size savings or runtime savings for this particular case...

Copy link
Member Author

Choose a reason for hiding this comment

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

Gas:

  ERC20 - transfer
           Used 51800n gas for >transfer<
    ✔ with an EOA (73ms)
           Used 82500n gas for >transfer<
    ✔ with a single owner Safe
           Used 88381n gas for >transfer<
    ✔ with a single owner and guard Safe
           Used 89553n gas for >transfer<
    ✔ with a 2 out of 2 Safe
           Used 96607n gas for >transfer<
    ✔ with a 3 out of 3 Safe
           Used 96595n gas for >transfer<
    ✔ with a 3 out of 5 Safe

codesize:

Safe 21502 bytes (limit is 24576)
SafeL2 22282 bytes (limit is 24576)

Copy link
Collaborator

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

Looks good! Just one nit (around adding a comment justifying the use of assembly), and an optional thing to try out and compare gas characteristics.

@nlordell
Copy link
Collaborator

I was a bit more reluctant at first for this change, but now that we got rid of the encodeTransactionData function, I find the assembly is very straightforward (allocation in the previous version was the more "unclear" part of the implementation) and am definitely more and more convinced of the change.

Awesome assembly 🧙 work!

contracts/Safe.sol Outdated Show resolved Hide resolved
@mmv08 mmv08 changed the title optimize encodeTransactionData by implementing it in assembly optimize getTransactionHash by implementing it in assembly Oct 28, 2024
@mmv08 mmv08 merged commit b55fd8f into main Oct 28, 2024
25 checks passed
@mmv08 mmv08 deleted the assembly-encodeSafeTransactionData branch October 28, 2024 14:05
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants