TimelockController contract's constructor takes input parameters as memory instead of calldata

[luislucena16]

• Location of the smart contract:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/TimelockController.sol#L81

Reason:

• Optimize the smart contract as follows:

constructor(
    uint256 minDelay,
    address[] calldata proponents,
    address[] calldata executors,
    address admin
)

Replacing memory by calldata

[ernestognw]

From @barakman, adding more information:

Following this discussion on OZ Forum, here is the prototype of the TimelockController contract's constructor:

constructor(
    uint256 minDelay,
    address[] memory proposers,
    address[] memory executors,
    address admin
)

You can somewhat reduce the gas consumption of this function, by changing each occurrence of memory to calldata.

Note that this may pose an API-breaking change for:

• Anyone inheriting from TimelockController
• Anyone creating an instance of this contract onchain (i.e., via new TimelockController)

[0xIchigo]

@ernestognw I'd be interested in opening up a PR to implement this change!

In addition to setting the contract's constructor input parameters to calldata, there are a few other simple optimizations that can be made to TimelockController.sol to help reduce gas consumption, including:

• Cache the array's length for the loop condition

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 96 in 3ec4307

    for (uint256 i = 0; i < proposers.length; ++i) {

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 102 in 3ec4307

    for (uint256 i = 0; i < executors.length; ++i) {

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 257 in 3ec4307

    for (uint256 i = 0; i < targets.length; ++i) {

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 340 in 3ec4307

    for (uint256 i = 0; i < targets.length; ++i) {

• With for loops, increment the variable i in an unchecked block
  • Same instances as above
  • Reasonable to assume that these arrays of addresses provided will not overflow before exceeding the block gas limit
• Use custom errors to save ~50 gas each time they are reached

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 252 in 3ec4307

    require(targets.length == values.length, "TimelockController: length mismatch");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 253 in 3ec4307

    require(targets.length == payloads.length, "TimelockController: length mismatch");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 269 in 3ec4307

    require(!isOperation(id), "TimelockController: operation already scheduled");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 270 in 3ec4307

    require(delay >= getMinDelay(), "TimelockController: insufficient delay");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 282 in 3ec4307

    require(isOperationPending(id), "TimelockController: operation cannot be cancelled");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 334 in 3ec4307

    require(targets.length == values.length, "TimelockController: length mismatch");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 335 in 3ec4307

    require(targets.length == payloads.length, "TimelockController: length mismatch");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 355 in 3ec4307

    require(success, "TimelockController: underlying transaction reverted");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 362 in 3ec4307

    require(isOperationReady(id), "TimelockController: operation is not ready");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 363 in 3ec4307

    require(predecessor == bytes32(0) || isOperationDone(predecessor), "TimelockController: missing dependency");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 370 in 3ec4307

    require(isOperationReady(id), "TimelockController: operation is not ready");

  • openzeppelin-contracts/contracts/governance/TimelockController.sol

    Line 385 in 3ec4307

    require(msg.sender == address(this), "TimelockController: caller must be timelock");

These are pretty low-hanging fruit and won't pose huge change to, or a complete overhaul of, TimelockController. Please let me know what you think about these optimizations! I am very eager to help out

[ernestognw]

Hey @0xIchigo, you can go ahead and create a PR for solving the issue this thread is about: calldata vs memory in the TimelockController constructor, the others might require more discussion from our side.

However, this is my perspective on each one of them:

• Cache the array's length for the loop condition
  ...

I don't see reasons for not doing these and it's not breaking so it should be fine.

With for loops, increment the variable i in an unchecked block

• Same instances as above
• Reasonable to assume that these arrays of addresses provided will not overflow before exceeding the block gas limit

These are a little bit risky since they involve readability, I think we'd overall prefer not to do them since:

1. The operations are not called a significant amount of times over the lifecycle of the contract
2. The gas savings might be very small unless you're looping thousands of proposals/proposers/executors, which seems unlikely

Correct me if I'm wrong on number 2 but both reasons together make a good reason to make a discussion first.

Use custom errors to save ~50 gas each time they are reached
...

We're already working on a major migration to custom errors. You can see the discussion in #2839. The TLDR is that we proposed EIP (6093) for standard tokens and we'll use the same rationale to design the custom errors for the whole library.

A big nuance we're currently facing is deciding how many parameters to add to a custom error so that they're useful enough without sacrificing the optimization itself. Consider that a change in parameters to a custom error could be considered a breaking change, so once Contracts 5.0 is released, we may be locked. We'd appreciate feedback (if any) on how to design the errors. The discussion for that is here.

Will wait for input from @frangio on this before giving you the green light. Thank you for your willingness to contribute!

[0xIchigo]

Hey @ernestognw, I appreciate your quick response! I'll go ahead and create a PR for solving the calldata vs memory issue in the TimelockController constructor.

With respect to incrementing the variable i in an unchecked block, I've drawn up this test contract in Remix. Incrementing in an unchecked block can save roughly 30-40 gas per iteration, which can be quite significant for lengthy loops. Even with smaller loops, like in the test case of five iterations, incrementing in an unchecked block cut gas costs by 28.7%. Incrementing in an unchecked block for 100 iterations cut gas costs by 32.2%. Incrementing in an unchecked block for 1000 iterations cut gas costs by 32.6%.

With respect to readability, the unchecked block can be placed at the bottom of the for loop instead of being placed inline with the initialization and condition. Instead of having a hard to read for loop header, the complexity is abstracted away with the unchecked block at bottom of the loop. I'd argue that a reduction of roughly 28.7% - 32.2% for iterations <= 100 would warrant this optimization, although I see how this is not 100% ideal in terms of readability since you'd be splitting up the loop's initialization, condition, and iteration. Regardless, I am a firm believer that any optimizations, irrespective of their size, that can be easily implemented without posing too great of an impact to the contract's structure, functionality, and adoption should be implemented.

I'll checkout the discussion regarding EIP 6093, and see if I have any feedback on designing the errors, thanks!

[0xIchigo]

Hey @ernestognw, sorry for the double ping but I just realized that we cannot optimize the TimelockController contract's constructor as the input parameters must either be of type storage or memory, given that the contract is written to the chain immediately followed by the constructor parameter data. The constructor then reads the values by looking them up at that offset, so constructor parameters can not be of type calldata.

If possible, I'd still like to go ahead with creating a PR to optimize the for loops found within TimelockController

[barakman]

With for loops, increment the variable i in an unchecked block

• Same instances as above
• Reasonable to assume that these arrays of addresses provided will not overflow before exceeding the block gas limit

FYI, the length of dynamic arrays is bounded by 2**64-1 (following this memory-creation-overflow-bug).

So an overflow in the loop counter could not occur here even theoretically, regardless of the block gas limit.

[hiroshitashir]

https://ethereum.stackexchange.com/questions/125100/the-reason-why-cant-i-use-calldata-as-a-data-location-for-constructor-parameter#:~:text=It's%20because%20the%20constructor%20is,them%20up%20at%20that%20offset.

[frangio]

Closing as explained above constructors can't have calldata arguments.

For other optimizations please open an issue sharing benchmarks that demonstrate an improvement.