From 7d89b371a9306c9f8fa102c43c250a6fa121dc37 Mon Sep 17 00:00:00 2001 From: Alexander Movchan Date: Fri, 10 Apr 2020 21:29:18 +0300 Subject: [PATCH 1/4] 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/4] 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 16489be9ff11f09d47a6173964ebb8227c99f1f5 Mon Sep 17 00:00:00 2001 From: Alexander Movchan Date: Mon, 13 Apr 2020 14:13:08 +0300 Subject: [PATCH 3/4] 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 4/4] 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;