From 7d89b371a9306c9f8fa102c43c250a6fa121dc37 Mon Sep 17 00:00:00 2001 From: Alexander Movchan Date: Fri, 10 Apr 2020 21:29:18 +0300 Subject: [PATCH 1/9] Store validators' ids, use it in Block --- contracts/contracts/Franklin.sol | 9 +++-- contracts/contracts/Governance.sol | 58 ++++++++++++++++++++++++++---- contracts/contracts/Storage.sol | 2 +- 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/contracts/contracts/Franklin.sol b/contracts/contracts/Franklin.sol index 2be6567620..79f3bd1023 100644 --- a/contracts/contracts/Franklin.sol +++ b/contracts/contracts/Franklin.sol @@ -393,8 +393,10 @@ contract Franklin is UpgradeableMaster, Storage, Config, Events { _publicData ); + uint24 validatorId = governance.getValidatorId(msg.sender); + blocks[_blockNumber] = Block( - msg.sender, // validator + validatorId, // validatorId uint32(block.number), // committed at _firstOnchainOpId, // blocks' onchain ops start id in global operations _nOnchainOpsProcessed, // total number of onchain ops in block @@ -600,9 +602,12 @@ contract Franklin is UpgradeableMaster, Storage, Config, Events { consummateOnchainOps(_blockNumber); + uint24 blockValidatorId = blocks[_blockNumber].validatorId; + address blockValidatorAddress = governance.getValidatorAddress(blockValidatorId); + collectValidatorsFeeAndDeleteRequests( blocks[_blockNumber].priorityOperations, - blocks[_blockNumber].validator + blockValidatorAddress ); totalBlocksVerified += 1; diff --git a/contracts/contracts/Governance.sol b/contracts/contracts/Governance.sol index 04279faf33..7085cf063a 100644 --- a/contracts/contracts/Governance.sol +++ b/contracts/contracts/Governance.sol @@ -25,8 +25,20 @@ contract Governance is Config { /// @notice List of registered tokens by address mapping(address => uint16) public tokenIds; + /// @notice Validator information + struct Validator { + uint24 id; + bool isActive; + } + /// @notice List of permitted validators - mapping(address => bool) public validators; + mapping(address => Validator) public validators; + + /// @notice Mapping from validator address to id + mapping(uint24 => address) public validatorAddresses; + + /// @notice Next validator id to insert into `validators` (0 for invalid) + uint24 nextValidatorId = 1; constructor() public {} @@ -37,7 +49,12 @@ contract Governance is Config { address _networkGovernor = abi.decode(initializationParameters, (address)); networkGovernor = _networkGovernor; - validators[_networkGovernor] = true; + + Validator memory validator = Validator(nextValidatorId, true); + validators[_networkGovernor] = validator; + validatorAddresses[nextValidatorId] = _networkGovernor; + + nextValidatorId += 1; } /// @notice Change current governor @@ -63,11 +80,22 @@ contract Governance is Config { } /// @notice Change validator status (active or not active) - /// @param _validator Validator address + /// @param _validatorAddress Validator address /// @param _active Active flag - function setValidator(address _validator, bool _active) external { + function setValidator(address _validatorAddress, bool _active) external { requireGovernor(msg.sender); - validators[_validator] = _active; + + Validator memory validator = validators[_validatorAddress]; + + if (validator.id == 0) { + validator.id = nextValidatorId; + validatorAddresses[validator.id] = _validatorAddress; + nextValidatorId += 1; + } + + validator.isActive = _active; + + validators[_validatorAddress] = validator; } /// @notice Check if specified address is is governor @@ -79,7 +107,25 @@ contract Governance is Config { /// @notice Checks if validator is active /// @param _address Validator address function requireActiveValidator(address _address) external view { - require(validators[_address], "grr21"); // validator is not active + require(validators[_address].isActive, "grr21"); // validator is not active + } + + /// @notice Get validator's id, checking that _address is known validator's address + /// @param _address Validator's address + /// @return validator's id + function getValidatorId(address _address) external view returns (uint24) { + uint24 validatorId = validators[_address].id; + require(validatorId != 0, "gvi10"); // _address is not a validator's address + return validatorId; + } + + /// @notice Get validator's address, checking that _validatorId is known validator's id + /// @param _validatorId Validator's id + /// @return validator's address + function getValidatorAddress(uint24 _validatorId) external view returns (address) { + address validatorAddress = validatorAddresses[_validatorId]; + require(validatorAddress != address(0), "gva10"); // _validatorId is invalid + return validatorAddress; } /// @notice Validate token id (must be less than or equal total tokens amount) diff --git a/contracts/contracts/Storage.sol b/contracts/contracts/Storage.sol index 5dc39cce16..b37ca642cc 100644 --- a/contracts/contracts/Storage.sol +++ b/contracts/contracts/Storage.sol @@ -54,7 +54,7 @@ contract Storage { /// @member commitment Hash of the block circuit commitment /// @member stateRoot New tree root hash struct Block { - address validator; + uint24 validatorId; uint32 committedAtBlock; uint64 operationStartId; uint64 onchainOperations; From 6afe806aedefa97c5502b62d163e4c2f4f4c04b1 Mon Sep 17 00:00:00 2001 From: Alexander Movchan Date: Fri, 10 Apr 2020 22:39:39 +0300 Subject: [PATCH 2/9] Store only cumulative onchain ops in block's metadata --- contracts/contracts/Franklin.sol | 33 ++++++++++++++++---------------- contracts/contracts/Storage.sol | 10 +++++----- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/contracts/contracts/Franklin.sol b/contracts/contracts/Franklin.sol index 79f3bd1023..d810d589df 100644 --- a/contracts/contracts/Franklin.sol +++ b/contracts/contracts/Franklin.sol @@ -359,9 +359,8 @@ contract Franklin is UpgradeableMaster, Storage, Config, Events { collectOnchainOps(_publicData, _ethWitness, _ethWitnessSizes); uint64 nPriorityRequestProcessed = totalCommittedPriorityRequests - prevTotalCommittedPriorityRequests; - uint64 nOnchainOpsProcessed = totalOnchainOps - firstOnchainOpId; - createCommittedBlock(_blockNumber, _feeAccount, _newRoot, _publicData, firstOnchainOpId, nOnchainOpsProcessed, nPriorityRequestProcessed); + createCommittedBlock(_blockNumber, _feeAccount, _newRoot, _publicData, totalOnchainOps, nPriorityRequestProcessed); totalBlocksCommitted++; emit BlockCommitted(_blockNumber); @@ -369,15 +368,14 @@ contract Franklin is UpgradeableMaster, Storage, Config, Events { } /// @notice Store committed block structure to the storage. - /// @param _firstOnchainOpId - blocks' onchain ops start id in global operations - /// @param _nOnchainOpsProcessed - total number of onchain ops in block - /// @param _nCommittedPriorityRequests - total number of priority requests in block + /// @param _nCumulativeOnchainOpsProcessed - cumulative number of onchain ops + /// @param _nCommittedPriorityRequests - number of priority requests in block function createCommittedBlock( uint32 _blockNumber, uint24 _feeAccount, bytes32 _newRoot, bytes memory _publicData, - uint64 _firstOnchainOpId, uint64 _nOnchainOpsProcessed, uint64 _nCommittedPriorityRequests + uint64 _nCumulativeOnchainOpsProcessed, uint64 _nCommittedPriorityRequests ) internal { require(_publicData.length % 8 == 0, "cbb10"); // Public data size is not multiple of 8 @@ -398,12 +396,11 @@ contract Franklin is UpgradeableMaster, Storage, Config, Events { blocks[_blockNumber] = Block( validatorId, // validatorId uint32(block.number), // committed at - _firstOnchainOpId, // blocks' onchain ops start id in global operations - _nOnchainOpsProcessed, // total number of onchain ops in block - _nCommittedPriorityRequests, // total number of priority onchain ops in block + _nCumulativeOnchainOpsProcessed, // cumulative number of onchain ops + _nCommittedPriorityRequests, // number of priority onchain ops in block + blockChunks, commitment, // blocks' commitment - _newRoot, // new root - blockChunks + _newRoot // new root ); } @@ -630,8 +627,14 @@ contract Franklin is UpgradeableMaster, Storage, Config, Events { /// @notice (user must have possibility to withdraw funds if withdrawed) /// @param _blockNumber Number of block function consummateOnchainOps(uint32 _blockNumber) internal { - uint64 start = blocks[_blockNumber].operationStartId; - uint64 end = start + blocks[_blockNumber].onchainOperations; + uint64 start; + if (_blockNumber == 0) { + start = 0; + } else { + start = blocks[_blockNumber - 1].cumulativeOnchainOperations; + } + uint64 end = blocks[_blockNumber].cumulativeOnchainOperations; + for (uint64 current = start; current < end; ++current) { OnchainOperation memory op = onchainOps[current]; if (op.opType == Operations.OpType.PartialExit) { @@ -667,20 +670,18 @@ contract Franklin is UpgradeableMaster, Storage, Config, Events { uint32 blocksToRevert = minU32(_maxBlocksToRevert, totalBlocksCommitted - totalBlocksVerified); uint64 revertedPriorityRequests = 0; - uint64 revertedOnchainOps = 0; for (uint32 i = totalBlocksCommitted - blocksToRevert + 1; i <= totalBlocksCommitted; i++) { Block memory revertedBlock = blocks[i]; require(revertedBlock.committedAtBlock > 0, "frk11"); // block not found - revertedOnchainOps += revertedBlock.onchainOperations; revertedPriorityRequests += revertedBlock.priorityOperations; delete blocks[i]; } totalBlocksCommitted -= blocksToRevert; - totalOnchainOps -= revertedOnchainOps; + totalOnchainOps = blocks[totalBlocksCommitted].cumulativeOnchainOperations; totalCommittedPriorityRequests -= revertedPriorityRequests; emit BlocksReverted(totalBlocksVerified, totalBlocksCommitted); diff --git a/contracts/contracts/Storage.sol b/contracts/contracts/Storage.sol index b37ca642cc..e664c1d279 100644 --- a/contracts/contracts/Storage.sol +++ b/contracts/contracts/Storage.sol @@ -48,20 +48,20 @@ contract Storage { /// @notice Rollup block data (once per block) /// @member validator Block producer /// @member committedAtBlock ETH block number at which this block was committed - /// @member operationStartId Index of the first operation to process for this block - /// @member onchainOperations Total number of operations to process for this block + /// @member cumulativeOnchainOperations Total number of operations in this and all previous blocks /// @member priorityOperations Total number of priority operations for this block /// @member commitment Hash of the block circuit commitment /// @member stateRoot New tree root hash + /// + /// Consider memory alignment when changing field order: https://solidity.readthedocs.io/en/v0.4.21/miscellaneous.html struct Block { uint24 validatorId; uint32 committedAtBlock; - uint64 operationStartId; - uint64 onchainOperations; + uint64 cumulativeOnchainOperations; uint64 priorityOperations; + uint32 chunks; bytes32 commitment; bytes32 stateRoot; - uint32 chunks; } /// @notice Blocks by Franklin block id From 26e9e7d020326d31de766ff702aad885c1260158 Mon Sep 17 00:00:00 2001 From: Igor Aleksanov Date: Mon, 13 Apr 2020 11:50:05 +0300 Subject: [PATCH 3/9] Add check for disabled dummy verifier to CI --- .drone.yml | 1 + bin/dummy-prover | 30 +++++++++++++++++++++++++----- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/.drone.yml b/.drone.yml index 71317cb145..ff35c0a33d 100644 --- a/.drone.yml +++ b/.drone.yml @@ -54,6 +54,7 @@ steps: - export ZKSYNC_HOME=`pwd` - export PATH=$ZKSYNC_HOME/bin:$PATH - export CARGO_HOME=$ZKSYNC_HOME/target/cargo + - zksync dummy-prover ensure-disabled - zksync circuit-tests - zksync prover-tests - zksync db-test diff --git a/bin/dummy-prover b/bin/dummy-prover index 33423445a4..fb7d3ea1cb 100755 --- a/bin/dummy-prover +++ b/bin/dummy-prover @@ -7,11 +7,12 @@ NC='\033[0m' # No Color USAGE='Usage: zksync dummy-prover [-h|--help|run|status|enable|disable] where: - -h | --help show this message - run run the Dummy Prover (default) - status get the status of the Dummy Prover (enabled/disabled) - enable enables the Dummy Prover support - disable disables the Dummy Prover support' + -h | --help show this message + run run the Dummy Prover (default) + status get the status of the Dummy Prover (enabled/disabled) + enable enables the Dummy Prover support + disable disables the Dummy Prover support + ensure-disabled checks that Dummy Prover is disabled and exits with code 1 otherwise' if [ -z $ZKSYNC_ENV ]; then @@ -72,6 +73,22 @@ function disable_dummy_prover { exit 0 } +function ensure_dummy_prover_disabled { + # Checks for the `DUMMY_VERIFIER` constant to be `false`. + # This is mandatory for CI and deploy since we don't want to accidentally obtain + # a dummy verifier deployed. + + if f grep -lq 'constant DUMMY_VERIFIER = true' $ZKSYNC_HOME/contracts/contracts/Verifier.sol; then + # Dummy verifier enabled, not allowed on CI. + echo "DUMMY_VERIFIER constant in Verifier.sol is set to 'true', which is not allowed." + echo "Change the DUMMY_VERIFIER constant value to 'false'" + exit 1 + else + # Dummy verifier disabled, it's OK. + exit 0 + fi +} + case $COMMAND in run) run_dummy_prover @@ -85,6 +102,9 @@ case $COMMAND in disable) disable_dummy_prover ;; + ensure-disabled) + ensure_dummy_prover_disabled + ;; -h | --help) echo "$USAGE" exit 0 From 081c7bf40e5714e967e31198375c3ca7e5bc85db Mon Sep 17 00:00:00 2001 From: Igor Aleksanov Date: Mon, 13 Apr 2020 12:01:40 +0300 Subject: [PATCH 4/9] Reword comments in script --- bin/dummy-prover | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/dummy-prover b/bin/dummy-prover index fb7d3ea1cb..5eed475569 100755 --- a/bin/dummy-prover +++ b/bin/dummy-prover @@ -75,11 +75,11 @@ function disable_dummy_prover { function ensure_dummy_prover_disabled { # Checks for the `DUMMY_VERIFIER` constant to be `false`. - # This is mandatory for CI and deploy since we don't want to accidentally obtain + # This is mandatory e.g. for CI and deploy since we don't want to accidentally obtain # a dummy verifier deployed. if f grep -lq 'constant DUMMY_VERIFIER = true' $ZKSYNC_HOME/contracts/contracts/Verifier.sol; then - # Dummy verifier enabled, not allowed on CI. + # Dummy verifier enabled, the restriction is violated. echo "DUMMY_VERIFIER constant in Verifier.sol is set to 'true', which is not allowed." echo "Change the DUMMY_VERIFIER constant value to 'false'" exit 1 From 16489be9ff11f09d47a6173964ebb8227c99f1f5 Mon Sep 17 00:00:00 2001 From: Alexander Movchan Date: Mon, 13 Apr 2020 14:13:08 +0300 Subject: [PATCH 5/9] Minor code style fix --- contracts/contracts/Franklin.sol | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/contracts/contracts/Franklin.sol b/contracts/contracts/Franklin.sol index 8d762c90aa..bcad908c92 100644 --- a/contracts/contracts/Franklin.sol +++ b/contracts/contracts/Franklin.sol @@ -659,12 +659,11 @@ contract Franklin is UpgradeableMaster, Storage, Config, Events { /// @notice (user must have possibility to withdraw funds if withdrawed) /// @param _blockNumber Number of block function consummateOnchainOps(uint32 _blockNumber) internal { - uint64 start; - if (_blockNumber == 0) { - start = 0; - } else { + uint64 start = 0; + if (_blockNumber != 0) { start = blocks[_blockNumber - 1].cumulativeOnchainOperations; } + uint64 end = blocks[_blockNumber].cumulativeOnchainOperations; for (uint64 current = start; current < end; ++current) { From db85c451f1f4d70ecaea646d4cdc6bb2c11aee9b Mon Sep 17 00:00:00 2001 From: Alexander Movchan Date: Mon, 13 Apr 2020 14:48:39 +0300 Subject: [PATCH 6/9] Minor refactoring: nextValidatorId => totalValidators --- contracts/contracts/Governance.sol | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/contracts/contracts/Governance.sol b/contracts/contracts/Governance.sol index 7085cf063a..87f384b22d 100644 --- a/contracts/contracts/Governance.sol +++ b/contracts/contracts/Governance.sol @@ -38,7 +38,7 @@ contract Governance is Config { mapping(uint24 => address) public validatorAddresses; /// @notice Next validator id to insert into `validators` (0 for invalid) - uint24 nextValidatorId = 1; + uint24 totalValidators; constructor() public {} @@ -50,11 +50,12 @@ contract Governance is Config { networkGovernor = _networkGovernor; - Validator memory validator = Validator(nextValidatorId, true); + uint24 validatorId = totalValidators + 1; + Validator memory validator = Validator(validatorId, true); validators[_networkGovernor] = validator; - validatorAddresses[nextValidatorId] = _networkGovernor; + validatorAddresses[validatorId] = _networkGovernor; - nextValidatorId += 1; + totalValidators += 1; } /// @notice Change current governor @@ -88,9 +89,9 @@ contract Governance is Config { Validator memory validator = validators[_validatorAddress]; if (validator.id == 0) { - validator.id = nextValidatorId; + validator.id = totalValidators + 1; validatorAddresses[validator.id] = _validatorAddress; - nextValidatorId += 1; + totalValidators += 1; } validator.isActive = _active; From 8aab1cbd8d77314bb503e5750bec6a521c827c2b Mon Sep 17 00:00:00 2001 From: Igor Aleksanov Date: Tue, 14 Apr 2020 08:10:23 +0300 Subject: [PATCH 7/9] Add git hook for dummy verifier & cargo fmt checks --- .githooks/pre-commit | 25 +++++++++++++++++++++++++ bin/.setup_env | 6 ++++++ 2 files changed, 31 insertions(+) create mode 100755 .githooks/pre-commit diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 0000000000..0843b5b607 --- /dev/null +++ b/.githooks/pre-commit @@ -0,0 +1,25 @@ +#!/bin/sh +# +# Pre-commit hook verifying that inappropriate code will not be committed. + +# Colors for the terminal output +RED='\033[0;31m' +NC='\033[0m' # No Color + +# Check that `rustfmt` rules are not violated. +if ! cargo fmt -- --check; then + echo "${RED}Commit error!${NC}" + echo "Please format the code via 'cargo fmt', cannot commit unformatted code" + exit 1 +fi + +VERFIER_CONTRACT_FILE="contracts/contracts/Verifier.sol" + +# Check if diff for contract contains setting the `DUMMY_VERIFIER` to the true. +if git diff --cached $VERFIER_CONTRACT_FILE | grep -lq 'constant DUMMY_VERIFIER = true'; then + echo "${RED}Commit error!${NC}" + echo "It seems that line 'constant DUMMY_VERIFIER = true' is staged to be committed" + echo "Cannot commit the code with enabled DUMMY_VERIFIER" + echo "Please disable the DUMMY_VERIFIER and try to commit changes again" + exit 1 +fi diff --git a/bin/.setup_env b/bin/.setup_env index e32ce64b47..1b7e82ad72 100755 --- a/bin/.setup_env +++ b/bin/.setup_env @@ -1,5 +1,6 @@ #!/bin/bash +# Setup env itself if [ -z "$ZKSYNC_ENV" ] then @@ -30,3 +31,8 @@ then popd > /dev/null fi + +# Setup the git hooks folder. +if ! git config --local core.hooksPath; then + git config --local core.hooksPath $ZKSYNC_HOME/.githooks/ +fi From 0229e2531c40cd71f700217ba37855a669d9969e Mon Sep 17 00:00:00 2001 From: Igor Aleksanov Date: Tue, 14 Apr 2020 08:16:13 +0300 Subject: [PATCH 8/9] Improve wording in githook --- .githooks/pre-commit | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 0843b5b607..2710abb596 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -18,7 +18,7 @@ VERFIER_CONTRACT_FILE="contracts/contracts/Verifier.sol" # Check if diff for contract contains setting the `DUMMY_VERIFIER` to the true. if git diff --cached $VERFIER_CONTRACT_FILE | grep -lq 'constant DUMMY_VERIFIER = true'; then echo "${RED}Commit error!${NC}" - echo "It seems that line 'constant DUMMY_VERIFIER = true' is staged to be committed" + echo "It seems that line 'constant DUMMY_VERIFIER = true' in 'Verifier.sol' is staged to be committed" echo "Cannot commit the code with enabled DUMMY_VERIFIER" echo "Please disable the DUMMY_VERIFIER and try to commit changes again" exit 1 From af564380721e3f6ff8b800b777990b36a6ec214d Mon Sep 17 00:00:00 2001 From: Igor Aleksanov Date: Tue, 14 Apr 2020 08:16:27 +0300 Subject: [PATCH 9/9] Add a note in the readme about git hooks --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index c48bd04726..eccf9f59b5 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,17 @@ zksync dockerhub-push # Development +## Committing changes + +`zksync` uses pre-commit git hooks for basic code integrity checks. Hooks are set up automatically +within the workspace initialization process. These hooks will not allow to commit the code which does +not pass several checks. + +Currently the following criteria are checked: + +- Code should always be formatted via `cargo fmt`. +- Dummy Prover should not be staged for commit (see below for the explanation). + ## Database migrations -