From 6a2dfb68d02a9066a281082ed1843b6c7ad77c90 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Wed, 27 Sep 2023 14:24:19 +0200 Subject: [PATCH 1/4] Optimize Validator timelock --- .../contracts/zksync/ValidatorTimelock.sol | 47 ++++++++----- .../contracts/zksync/libraries/LibMap.sol | 66 +++++++++++++++++++ .../validator_timelock_test.spec.ts | 4 +- 3 files changed, 99 insertions(+), 18 deletions(-) create mode 100644 ethereum/contracts/zksync/libraries/LibMap.sol diff --git a/ethereum/contracts/zksync/ValidatorTimelock.sol b/ethereum/contracts/zksync/ValidatorTimelock.sol index d47e96f1c..a9b9041db 100644 --- a/ethereum/contracts/zksync/ValidatorTimelock.sol +++ b/ethereum/contracts/zksync/ValidatorTimelock.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.13; import "@openzeppelin/contracts/access/Ownable2Step.sol"; +import "./libraries/LibMap.sol"; import "./interfaces/IExecutor.sol"; /// @author Matter Labs @@ -15,6 +16,8 @@ import "./interfaces/IExecutor.sol"; /// timestamp is stored for it. Later, when the owner calls the batch execution, the contract checks that batch /// was committed not earlier than X time ago. contract ValidatorTimelock is IExecutor, Ownable2Step { + using LibMap for LibMap.Uint32Map; + /// @dev Part of the IBase interface. Not used in this contract. string public constant override getName = "ValidatorTimelock"; @@ -27,19 +30,19 @@ contract ValidatorTimelock is IExecutor, Ownable2Step { /// @dev The main zkSync smart contract. address public immutable zkSyncContract; - /// @dev The mapping of L2 batch number => timestamp when it was commited. - mapping(uint256 => uint256) public committedBatchTimestamp; + /// @dev The mapping of L2 batch number => timestamp when it was committed. + LibMap.Uint32Map committedBatchTimestamp; /// @dev The address that can commit/revert/validate/execute batches. address public validator; /// @dev The delay between committing and executing batches. - uint256 public executionDelay; + uint32 public executionDelay; constructor( address _initialOwner, address _zkSyncContract, - uint256 _executionDelay, + uint32 _executionDelay, address _validator ) { _transferOwnership(_initialOwner); @@ -56,7 +59,7 @@ contract ValidatorTimelock is IExecutor, Ownable2Step { } /// @dev Set the delay between committing and executing batches. - function setExecutionDelay(uint256 _executionDelay) external onlyOwner { + function setExecutionDelay(uint32 _executionDelay) external onlyOwner { executionDelay = _executionDelay; emit NewExecutionDelay(_executionDelay); } @@ -67,14 +70,24 @@ contract ValidatorTimelock is IExecutor, Ownable2Step { _; } + /// @dev Returns the timestamp when `_l2BatchNumber` was committed. + function getCommittedBatchTimestamp(uint256 _l2BatchNumber) external view returns (uint256) { + return committedBatchTimestamp.get(_l2BatchNumber); + } + /// @dev Records the timestamp for all provided committed batches and make /// a call to the zkSync contract with the same calldata. function commitBatches(StoredBatchInfo calldata, CommitBatchInfo[] calldata _newBatchesData) external onlyValidator { - for (uint256 i = 0; i < _newBatchesData.length; ++i) { - committedBatchTimestamp[_newBatchesData[i].batchNumber] = block.timestamp; + unchecked { + // This contract is only a temporary solution, that hopefully will be disabled until 2106 year, so... + // It is safe to cast. + uint32 timestamp = uint32(block.timestamp); + for (uint256 i = 0; i < _newBatchesData.length; ++i) { + committedBatchTimestamp.set(_newBatchesData[i].batchNumber, timestamp); + } } _propagateToZkSync(); @@ -101,15 +114,17 @@ contract ValidatorTimelock is IExecutor, Ownable2Step { /// @dev Check that batches were committed at least X time ago and /// make a call to the zkSync contract with the same calldata. function executeBatches(StoredBatchInfo[] calldata _newBatchesData) external onlyValidator { - for (uint256 i = 0; i < _newBatchesData.length; ++i) { - uint256 commitBatchTimestamp = committedBatchTimestamp[_newBatchesData[i].batchNumber]; - - // Note: if the `commitBatchTimestamp` is zero, that means either: - // * The batch was committed, but not though this contract. - // * The batch wasn't committed at all, so execution will fail in the zkSync contract. - // We allow executing such batches. - - require(block.timestamp > commitBatchTimestamp + executionDelay, "5c"); // The delay is not passed + uint256 delay = executionDelay; // uint32 + unchecked { + for (uint256 i = 0; i < _newBatchesData.length; ++i) { + uint256 commitBlockTimestamp = committedBatchTimestamp.get(_newBatchesData[i].batchNumber); + + // Note: if the `commitBlockTimestamp` is zero, that means either: + // * The block was committed, but not through this contract. + // * The block wasn't committed at all, so execution will fail in the zkSync contract. + // We allow executing such blocks. + require(block.timestamp >= commitBlockTimestamp + delay, "5c"); // The delay is not passed + } } _propagateToZkSync(); diff --git a/ethereum/contracts/zksync/libraries/LibMap.sol b/ethereum/contracts/zksync/libraries/LibMap.sol new file mode 100644 index 000000000..8c8f78161 --- /dev/null +++ b/ethereum/contracts/zksync/libraries/LibMap.sol @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +/// @notice Library for storage of packed unsigned integers. +/// @author Solady (https://github.com/vectorized/solady/blob/main/src/utils/LibMap.sol) +library LibMap { + /// @dev A uint32 map in storage. + struct Uint32Map { + mapping(uint256 => uint256) map; + } + + /// @dev Retrieves the uint32 value at a specific index from the Uint32Map. + /// @param _map The Uint32Map instance containing the packed uint32 values. + /// @param _index The index of the uint32 value to retrieve. + /// @return result The uint32 value at the specified index. + function get(Uint32Map storage _map, uint256 _index) internal view returns (uint32 result) { + unchecked { + // Each storage slot can store 256 bits of data. + // As uint32 is 32 bits long, 8 uint32s can be packed into one storage slot. + // Hence, `_index / 8` is done to find the storage slot that contains the required uint32. + uint256 mapValue = _map.map[_index / 8]; + + // First three bits of the original `_index` denotes the position of the uint32 in that slot. + // So, '(_index & 7) * 32' is done to find the bit position of the uint32 in that storage slot. + uint256 bitOffset = (_index & 7) * 32; + + // Shift the bits to the right and retrieve the uint32 value. + result = uint32(mapValue >> bitOffset); + } + } + + /// @dev Updates the uint32 value at `_index` in `map`. + /// @param _map The Uint32Map instance containing the packed uint32 values. + /// @param _index The index of the uint32 value to retrieve. + /// @param _value The new value at the specified index. + function set( + Uint32Map storage _map, + uint256 _index, + uint32 _value + ) internal { + unchecked { + // Each storage slot can store 256 bits of data. + // As uint32 is 32 bits long, 8 uint32s can be packed into one storage slot. + // Hence, `_index / 8` is done to find the storage slot that contains the required uint32. + uint256 mapIndex = _index / 8; + uint256 mapValue = _map.map[mapIndex]; + + // First three bits of the original `_index` denotes the position of the uint32 in that slot. + // So, '(_index & 7) * 32' is done to find the bit position of the uint32 in that storage slot. + uint256 bitOffset = (_index & 7) * 32; + + // XORing a value A with B, and then with A again, gives the original value B. + // We will use this property to update the uint32 value in the slot. + + // Shift the bits to the right and retrieve the uint32 value. + uint32 oldValue = uint32(mapValue >> bitOffset); + + // Calculate the XOR of the new value and the existing value. + uint256 newValueXorOldValue = uint256(oldValue ^ _value); + + // Finally, we XOR the slot with the XOR of the new value and the existing value, + // shifted to its proper position. The XOR operation will effectively replace the old value with the new value. + _map.map[mapIndex] = (newValueXorOldValue << bitOffset) ^ mapValue; + } + } +} diff --git a/ethereum/test/unit_tests/validator_timelock_test.spec.ts b/ethereum/test/unit_tests/validator_timelock_test.spec.ts index 313216c09..2aa56d4e7 100644 --- a/ethereum/test/unit_tests/validator_timelock_test.spec.ts +++ b/ethereum/test/unit_tests/validator_timelock_test.spec.ts @@ -157,7 +157,7 @@ describe(`ValidatorTimelock tests`, function () { }); it(`Should successfully overwrite the committing timestamp on the reverted batches timestamp`, async () => { - const revertedBatchesTimestamp = Number(await validatorTimelock.committedBatchTimestamp(1)); + const revertedBatchesTimestamp = Number(await validatorTimelock.getCommittedBatchTimestamp(1)); await validatorTimelock .connect(validator) @@ -167,7 +167,7 @@ describe(`ValidatorTimelock tests`, function () { .connect(validator) .proveBatches(getMockStoredBatchInfo(0), [getMockStoredBatchInfo(1)], MOCK_PROOF_INPUT); - const newBatchesTimestamp = Number(await validatorTimelock.committedBatchTimestamp(1)); + const newBatchesTimestamp = Number(await validatorTimelock.getCommittedBatchTimestamp(1)); expect(newBatchesTimestamp).greaterThanOrEqual(revertedBatchesTimestamp); }); From 4796b68af8df4f5d42a86c3823b10d07e78ad731 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Wed, 27 Sep 2023 14:29:37 +0200 Subject: [PATCH 2/4] Block -> Batch --- ethereum/contracts/zksync/ValidatorTimelock.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ethereum/contracts/zksync/ValidatorTimelock.sol b/ethereum/contracts/zksync/ValidatorTimelock.sol index a9b9041db..e04150930 100644 --- a/ethereum/contracts/zksync/ValidatorTimelock.sol +++ b/ethereum/contracts/zksync/ValidatorTimelock.sol @@ -117,13 +117,13 @@ contract ValidatorTimelock is IExecutor, Ownable2Step { uint256 delay = executionDelay; // uint32 unchecked { for (uint256 i = 0; i < _newBatchesData.length; ++i) { - uint256 commitBlockTimestamp = committedBatchTimestamp.get(_newBatchesData[i].batchNumber); + uint256 commitBatchTimestamp = committedBatchTimestamp.get(_newBatchesData[i].batchNumber); - // Note: if the `commitBlockTimestamp` is zero, that means either: - // * The block was committed, but not through this contract. - // * The block wasn't committed at all, so execution will fail in the zkSync contract. - // We allow executing such blocks. - require(block.timestamp >= commitBlockTimestamp + delay, "5c"); // The delay is not passed + // Note: if the `commitBatchTimestamp` is zero, that means either: + // * The batch was committed, but not through this contract. + // * The batch wasn't committed at all, so execution will fail in the zkSync contract. + // We allow executing such batches. + require(block.timestamp >= commitBatchTimestamp + delay, "5c"); // The delay is not passed } } From c50f533b64d4e006bd37c80c5c264a276b6ca849 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Thu, 28 Sep 2023 02:52:47 +0200 Subject: [PATCH 3/4] Prettier --- .../contracts/zksync/ValidatorTimelock.sol | 10 ++++----- ethereum/yarn.lock | 22 ++++--------------- 2 files changed, 9 insertions(+), 23 deletions(-) diff --git a/ethereum/contracts/zksync/ValidatorTimelock.sol b/ethereum/contracts/zksync/ValidatorTimelock.sol index 3ae725660..71222228c 100644 --- a/ethereum/contracts/zksync/ValidatorTimelock.sol +++ b/ethereum/contracts/zksync/ValidatorTimelock.sol @@ -74,10 +74,10 @@ contract ValidatorTimelock is IExecutor, Ownable2Step { /// @dev Records the timestamp for all provided committed batches and make /// a call to the zkSync contract with the same calldata. - function commitBatches(StoredBatchInfo calldata, CommitBatchInfo[] calldata _newBatchesData) - external - onlyValidator - { + function commitBatches( + StoredBatchInfo calldata, + CommitBatchInfo[] calldata _newBatchesData + ) external onlyValidator { unchecked { // This contract is only a temporary solution, that hopefully will be disabled until 2106 year, so... // It is safe to cast. @@ -115,7 +115,7 @@ contract ValidatorTimelock is IExecutor, Ownable2Step { unchecked { for (uint256 i = 0; i < _newBatchesData.length; ++i) { uint256 commitBatchTimestamp = committedBatchTimestamp.get(_newBatchesData[i].batchNumber); - + // Note: if the `commitBatchTimestamp` is zero, that means either: // * The batch was committed, but not through this contract. // * The batch wasn't committed at all, so execution will fail in the zkSync contract. diff --git a/ethereum/yarn.lock b/ethereum/yarn.lock index 7b5c7a290..cb865c5d6 100644 --- a/ethereum/yarn.lock +++ b/ethereum/yarn.lock @@ -904,7 +904,7 @@ resolved "https://registry.yarnpkg.com/@sindresorhus/is/-/is-4.6.0.tgz#3c7c9c46e678feefe7a2e5bb609d3dbd665ffb3f" integrity sha512-t09vSN3MdfsyCHoFcTRCH/iUtG7OJ0CsjzB8cjAmKc/va/kIgeDI/TxsigdncE/4be734m0cvIYwNaV4i2XqAw== -"@solidity-parser/parser@^0.14.0", "@solidity-parser/parser@^0.14.1": +"@solidity-parser/parser@^0.14.0": version "0.14.5" resolved "https://registry.yarnpkg.com/@solidity-parser/parser/-/parser-0.14.5.tgz#87bc3cc7b068e08195c219c91cd8ddff5ef1a804" integrity sha512-6dKnHZn7fg/iQATVEzqyUOyEidbn05q7YA2mQ9hC0MMXhhV3/JrsxmFSYZAcr7j1yUP700LLhTruvJ3MiQmjJg== @@ -6979,7 +6979,7 @@ node-environment-flags@1.0.6: object.getownpropertydescriptors "^2.0.3" semver "^5.7.0" -node-fetch@^2.6.0, node-fetch@^2.6.1, node-fetch@^2.6.7: +node-fetch@^2.6.1, node-fetch@^2.6.7: version "2.6.7" resolved "https://registry.yarnpkg.com/node-fetch/-/node-fetch-2.6.7.tgz#24de9fba827e3b4ae44dc8b20256a379160052ad" integrity sha512-ZjMPFEfVx5j+y2yF35Kzx5sF7kDzxuDj6ziH4FFbOp87zKDZNx8yExJIb05OGF4Nlt9IHFIMBkRl41VdvcNdbQ== @@ -7011,13 +7011,6 @@ nopt@3.x: dependencies: abbrev "1" -nopt@3.x: - version "3.0.6" - resolved "https://registry.yarnpkg.com/nopt/-/nopt-3.0.6.tgz#c6465dbf08abcd4db359317f79ac68a646b28ff9" - integrity sha512-4GUt3kSEYmk4ITxzB/b9vaIDfUVWN/Ml1Fwl11IlnIG2iaJ9O6WXZ9SrYM9NLI8OCBieN2Y8SWC2oJV0RQ7qYg== - dependencies: - abbrev "1" - normalize-package-data@^2.3.2: version "2.5.0" resolved "https://registry.yarnpkg.com/normalize-package-data/-/normalize-package-data-2.5.0.tgz#e66db1838b200c1dfc233225d12cb36520e234a8" @@ -8233,7 +8226,7 @@ semaphore@>=1.0.1, semaphore@^1.0.3, semaphore@^1.1.0: resolved "https://registry.yarnpkg.com/semaphore/-/semaphore-1.1.0.tgz#aaad8b86b20fe8e9b32b16dc2ee682a8cd26a8aa" integrity sha512-O4OZEaNtkMd/K0i6js9SL+gqy0ZCBMgUvlSqHKi4IBdjhe7wB8pwztUk1BbZ1fmrvpwFrPbHzqd2w5pTcJH6LA== -"semver@2 || 3 || 4 || 5", semver@^5.3.0, semver@^5.5.0, semver@^5.6.0, semver@^5.7.0: +"semver@2 || 3 || 4 || 5", semver@^5.3.0, semver@^5.5.0, semver@^5.6.0: version "5.7.1" resolved "https://registry.yarnpkg.com/semver/-/semver-5.7.1.tgz#a954f931aeba508d307bbf069eff0c01c96116f7" integrity sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ== @@ -8248,14 +8241,7 @@ semver@^6.3.0: resolved "https://registry.yarnpkg.com/semver/-/semver-6.3.0.tgz#ee0a64c8af5e8ceea67687b133761e1becbd1d3d" integrity sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw== -semver@^7.3.4: - version "7.5.4" - resolved "https://registry.yarnpkg.com/semver/-/semver-7.5.4.tgz#483986ec4ed38e1c6c48c34894a9182dbff68a6e" - integrity sha512-1bCSESV6Pv+i21Hvpxp3Dx+pSD8lIPt8uVjRrxAUt/nbswYc+tK6Y2btiULjd4+fnq15PX+nqQDC7Oft7WkwcA== - dependencies: - lru-cache "^6.0.0" - -semver@^7.3.8, semver@^7.5.2: +semver@^7.3.4, semver@^7.3.8, semver@^7.5.2: version "7.5.4" resolved "https://registry.yarnpkg.com/semver/-/semver-7.5.4.tgz#483986ec4ed38e1c6c48c34894a9182dbff68a6e" integrity sha512-1bCSESV6Pv+i21Hvpxp3Dx+pSD8lIPt8uVjRrxAUt/nbswYc+tK6Y2btiULjd4+fnq15PX+nqQDC7Oft7WkwcA== From 66cbe30b1a8d2aa15fedb52d6a4caf9d3cffe6e0 Mon Sep 17 00:00:00 2001 From: vladbochok Date: Thu, 28 Sep 2023 05:26:40 +0200 Subject: [PATCH 4/4] Increase number of allowed warnings --- ethereum/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/package.json b/ethereum/package.json index 65560f0b5..9667d62cc 100644 --- a/ethereum/package.json +++ b/ethereum/package.json @@ -53,7 +53,7 @@ "test:foundry": "hardhat solpp && forge test", "test:fork": "CONTRACT_TESTS=1 TEST_CONTRACTS_FORK=1 yarn run hardhat test test/unit_tests/*.fork.ts --network hardhat", "lint": "yarn lint:sol && yarn prettier:check", - "lint:sol-contracts": "solhint --max-warnings 30 contracts/**/*.sol", + "lint:sol-contracts": "solhint --max-warnings 31 contracts/**/*.sol", "lint:sol-tests": "solhint --max-warnings 0 test/**/*.sol", "lint:sol": "yarn lint:sol-contracts && yarn lint:sol-tests", "prettier:check-contracts": "prettier --check contracts/**/*.sol",