Skip to content

Conversation

@Saw-mon-and-Natalie
Copy link

This PR applies some gas optimizations to ClonesWithImmutableArgs.sol and Clone.sol.

  • ClonesWithImmutableArgs.sol has been rewritten to optimize gas and also consolidate all the assembly blocks.
  • The edits to Clone.sol include using more assembly for _getArgUint256Array method and also together with ClonesWithImmutableArgs.sol edits, we are assuming the 2 bytes at the end of the clone/proxy contract is the data length plus 2. In this way we would save subtracting 2 in ClonesWithImmutableArgs.sol and adding it back in Clone.sol to calculate the extra data offset in the calldata.

In the 1st commit 30958a4d3a39c31293353dee7c41519df9ca133b, we update the .gas-snapshot since it was not previously updated.

Here is the result of running forge snapshot ... with the new optimization edits:

$ forge snapshot --include-fuzz-tests --desc --diff
[⠒] Compiling...
[⠃] Compiling 8 files with 0.8.13
[⠊] Solc 0.8.13 finished in 1.79s
Compiler run successful

Running 4 tests for src\test\ExampleClone.t.sol:ExampleCloneTest
[PASS] testGas_param1() (gas: 8144)
[PASS] testGas_param2() (gas: 8074)
[PASS] testGas_param3() (gas: 8132)
[PASS] testGas_param4() (gas: 8177)
Test result: ok. 4 passed; 0 failed; finished in 2.97ms

Running 2 tests for src\test\ExampleCloneFactory.t.sol:ExampleCloneFactoryTest
[PASS] testCorrectness_clone(address,uint256,uint64,uint8) (runs: 256, μ: 69927, ~: 69927)
[PASS] testGas_clone(address,uint256,uint64,uint8) (runs: 256, μ: 64003, ~: 64003)
Test result: ok. 2 passed; 0 failed; finished in 170.58ms
testGas_clone(address,uint256,uint64,uint8) (gas: -576 (-0.009%))
testGas_param2() (gas: -105 (-0.013%))
testGas_param4() (gas: -117 (-0.014%))
testGas_param3() (gas: -117 (-0.014%))
testCorrectness_clone(address,uint256,uint64,uint8) (gas: -1038 (-0.015%))
testGas_param1() (gas: -123 (-0.015%))
Overall gas change: -2076 (-0.080%)

@z0r0z
Copy link
Contributor

z0r0z commented Jul 12, 2022

do you need unchecked here?

@Saw-mon-and-Natalie
Copy link
Author

do you need unchecked here?

@z0r0z , This unchecked block was carried over from the current implementation.

The potential overflows that might be problematic would be:

let extraLength := add(mload(data), 2) // +2 bytes for telling how much data there is appended to the call
let creationSize := add(extraLength, BOOTSTRAP_LENGTH)

and

mstore(FREE_MEMORY_POINTER_SLOT, add(ptr, creationSize))

Basically for the overflow to happen we would need add(ptr, creationSize) or

$$ \tt{ptr} + \tt{BOOTSTRAPLENGTH} + \tt{mload}(\tt{data}) + 2 \geq 2^{256} $$

$$ \tt{ptr} + \tt{mload}(\tt{data}) \geq 2^{256} - 65 $$

From EIP-170, we know that the MAX_CODE_SIZE is a capped at $2^{14} + 2^{13}$. That means

$$ \tt{mload}(\tt{data}) \lt 2^{14} + 2^{13} $$

otherwise, the create instruction should fail:

instance := create(0, ptr, creationSize)

So combing everything so far, we should have in case of an overflow as a rough estimate:

$$ \tt{ptr} \geq 2^{256} - 2^{14} - 2^{13} - 65 \gt 2^{255} $$

This would mean the memory has been expanded quite a lot, plugging the lower band in the memory expansion cost function would roughly give us

$$ C = 3 \cdot 2^{255} + \lceil \frac{2^{510}}{512} \rceil \gt 2^{500} \gg 30,000,000 $$

So the memory expansion gas cost would be astronomically high. Based on these calculations the function call would run out of gas before it would overflow.

@wminshew
Copy link

I am under the impression that unchecked doesn't apply w/i an assembly block and so may be safely removed here

wminshew added a commit to 0xSplits/clones-with-immutable-args that referenced this pull request Aug 12, 2022
0xca11 added a commit to RollaProject/clones-with-immutable-args that referenced this pull request Aug 15, 2022
0xca11 added a commit to RollaProject/clones-with-immutable-args that referenced this pull request Aug 15, 2022
0xca11 added a commit to RollaProject/clones-with-immutable-args that referenced this pull request Aug 15, 2022
@Saw-mon-and-Natalie
Copy link
Author

@wminshew , you are right. I updated the PR.

@z0r0z, @wighawag , please have a look. It know includes more optimizations compared to a few months back.

@BlinkyStitt
Copy link

anything blocking this? I'm wanting to deploy a clone soon and these changes look helpful

@wminshew
Copy link

@wysenynja believe solady's clone has these updates + others baked in

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.

4 participants