Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set of fixes for boojum integration #73

Merged
merged 21 commits into from
Nov 1, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion SystemConfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"BATCH_OVERHEAD_L2_GAS": 1200000,
"BATCH_OVERHEAD_L1_GAS": 1000000,
"MAX_TRANSACTIONS_IN_BATCH": 1024,
"BOOTLOADER_TX_ENCODING_SPACE": 273132,
"BOOTLOADER_TX_ENCODING_SPACE": 8740224,
"L1_TX_INTRINSIC_L2_GAS": 167157,
"L1_TX_INTRINSIC_PUBDATA": 88,
"L1_TX_MIN_L2_GAS_BASE": 173484,
Expand Down
3 changes: 2 additions & 1 deletion ethereum/.solhint.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
"prettier/prettier": ["error"],
"func-visibility": ["error", { "ignoreConstructors": true }],
"compiler-version": ["error", ">=0.8.0"],
"max-line-length": ["error", 120],
"max-line-length": ["error", 125],
"var-name-mixedcase": "off",
"func-name-mixedcase": "off",
"no-inline-assembly": "off",
"custom-errors": "off",
"no-global-import": "off",
"no-complex-fallback": "off",
"avoid-low-level-calls": "off",
"immutable-vars-naming": ["warn", { "immutablesAsConstants": false }]
}
}
6 changes: 3 additions & 3 deletions ethereum/contracts/governance/Governance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ contract Governance is IGovernance, Ownable2Step {
/// @dev Cancel the scheduled operation.
/// @dev Both the owner and security council may cancel an operation.
/// @param _id Proposal id value (see `hashOperation`)
function cancel(bytes32 _id) external onlyOwnerOrSecurityCouncil {
function cancel(bytes32 _id) external onlyOwner {
require(isOperationPending(_id), "Operation must be pending");
delete timestamps[_id];
emit OperationCancelled(_id);
Expand All @@ -164,7 +164,7 @@ contract Governance is IGovernance, Ownable2Step {
/// @notice Executes the scheduled operation after the delay passed.
/// @dev Both the owner and security council may execute delayed operations.
/// @param _operation The operation parameters will be executed with the upgrade.
function execute(Operation calldata _operation) external onlyOwnerOrSecurityCouncil {
function execute(Operation calldata _operation) external payable onlyOwnerOrSecurityCouncil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for this function we may skip it, since it would be basically testing that payable works

bytes32 id = hashOperation(_operation);
// Check if the predecessor operation is completed.
_checkPredecessorDone(_operation.predecessor);
Expand All @@ -183,7 +183,7 @@ contract Governance is IGovernance, Ownable2Step {
/// @notice Executes the scheduled operation with the security council instantly.
/// @dev Only the security council may execute an operation instantly.
/// @param _operation The operation parameters will be executed with the upgrade.
function executeInstant(Operation calldata _operation) external onlySecurityCouncil {
function executeInstant(Operation calldata _operation) external payable onlySecurityCouncil {
bytes32 id = hashOperation(_operation);
// Check if the predecessor operation is completed.
_checkPredecessorDone(_operation.predecessor);
Expand Down
4 changes: 2 additions & 2 deletions ethereum/contracts/governance/IGovernance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ interface IGovernance {

function cancel(bytes32 _id) external;

function execute(Operation calldata _operation) external;
function execute(Operation calldata _operation) external payable;

function executeInstant(Operation calldata _operation) external;
function executeInstant(Operation calldata _operation) external payable;

function hashOperation(Operation calldata _operation) external pure returns (bytes32);

Expand Down
6 changes: 5 additions & 1 deletion ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import "../zksync/interfaces/IMailbox.sol";
import "../zksync/interfaces/IVerifier.sol";
import "../common/libraries/L2ContractHelper.sol";
import "../zksync/libraries/TransactionValidator.sol";
import {SYSTEM_UPGRADE_L2_TX_TYPE, MAX_NEW_FACTORY_DEPS} from "../zksync/Config.sol";
import {SYSTEM_UPGRADE_L2_TX_TYPE, MAX_NEW_FACTORY_DEPS, MAX_ALLOWED_PROTOCOL_VERSION_DELTA} from "../zksync/Config.sol";

/// @author Matter Labs
/// @custom:security-contact [email protected]
Expand Down Expand Up @@ -217,6 +217,10 @@ abstract contract BaseZkSyncUpgrade is Base {
_newProtocolVersion > previousProtocolVersion,
"New protocol version is not greater than the current one"
);
require(
_newProtocolVersion - previousProtocolVersion <= MAX_ALLOWED_PROTOCOL_VERSION_DELTA,
StanislavBreadless marked this conversation as resolved.
Show resolved Hide resolved
"Too big protocol version difference"
);

// If the previous upgrade had an L2 system upgrade transaction, we require that it is finalized.
require(s.l2SystemContractsUpgradeTxHash == bytes32(0), "Previous upgrade has not been finalized");
Expand Down
5 changes: 5 additions & 0 deletions ethereum/contracts/zksync/Config.sol
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@ uint256 constant PRIORITY_OPERATION_L2_TX_TYPE = 255;
/// @dev Denotes the type of the zkSync transaction that is used for system upgrades.
uint256 constant SYSTEM_UPGRADE_L2_TX_TYPE = 254;

/// @dev The maximal allowed difference between protocol versions in an upgrade. The 100 gap is needed
/// in case a protocol version has been tested on testnet, but then not launched on mainnet, e.g.
/// due to a bug found.
uint256 constant MAX_ALLOWED_PROTOCOL_VERSION_DELTA = 100;

/// @dev The amount of time in seconds the validator has to process the priority transaction
/// NOTE: The constant is set to zero for the Alpha release period
uint256 constant PRIORITY_EXPIRATION = 0 days;
Expand Down
2 changes: 0 additions & 2 deletions ethereum/contracts/zksync/facets/Base.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ import "../Storage.sol";
import "../../common/ReentrancyGuard.sol";
import "../../common/AllowListed.sol";

import "@openzeppelin/contracts/access/Ownable.sol";

/// @title Base contract containing functions accessible to the other facets.
/// @author Matter Labs
/// @custom:security-contact [email protected]
Expand Down
29 changes: 20 additions & 9 deletions ethereum/contracts/zksync/facets/Executor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ contract ExecutorFacet is Base, IExecutor {

uint256 lastL2BlockTimestamp = _packedBatchAndL2BlockTimestamp & PACKED_L2_BLOCK_TIMESTAMP_MASK;

// All L2 blocks have timestamps within the range of [batchTimestamp, lastL2BatchTimestamp].
// All L2 blocks have timestamps within the range of [batchTimestamp, lastL2BlockTimestamp].
// So here we need to only double check that:
// - The timestamp of the batch is not too small.
// - The timestamp of the last L2 block is not too big.
Expand Down Expand Up @@ -352,26 +352,37 @@ contract ExecutorFacet is Base, IExecutor {
// We allow skipping the zkp verification for the test(net) environment
// If the proof is not empty, verify it, otherwise, skip the verification
if (_proof.serializedProof.length > 0) {
bool successVerifyProof = s.verifier.verify(
_verifyProof(
proofPublicInput,
_proof.serializedProof,
_proof.recursiveAggregationInput
_proof
);
require(successVerifyProof, "p"); // Proof verification fail
}
// #else
bool successVerifyProof = s.verifier.verify(
_verifyProof(
proofPublicInput,
_proof.serializedProof,
_proof.recursiveAggregationInput
_proof
);
require(successVerifyProof, "p"); // Proof verification fail
// #endif

emit BlocksVerification(s.totalBatchesVerified, currentTotalBatchesVerified);
s.totalBatchesVerified = currentTotalBatchesVerified;
}

function _verifyProof(
uint256[] memory proofPublicInput,
ProofInput calldata _proof
) internal view {
// We can only process 1 batch proof at a time.
require(_proof.serializedProof.length == 1, "t4");

bool successVerifyProof = s.verifier.verify(
proofPublicInput,
_proof.serializedProof,
_proof.recursiveAggregationInput
);
require(successVerifyProof, "p"); // Proof verification fail
}

/// @dev Gets zk proof public input
function _getBatchProofPublicInput(
bytes32 _prevBatchCommitment,
Expand Down
1 change: 1 addition & 0 deletions ethereum/contracts/zksync/interfaces/IMailbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ interface IMailbox is IBase {
/// @param contractAddressL2 The address of the contract on L2 to call.
/// @param expirationTimestamp The timestamp by which the priority operation must be processed by the operator.
/// @param l2GasLimit The limit of the L2 gas for the L2 transaction
/// @param l2GasPrice The price of the L2 gas in Wei to be used for this transaction.
/// @param l2GasPricePerPubdata The price for a single pubdata byte in L2 gas.
/// @param valueToMint The amount of ether that should be minted on L2 as the result of this transaction.
/// @param refundRecipient The recipient of the refund for the transaction on L2. If the transaction fails, then
Expand Down
10 changes: 8 additions & 2 deletions ethereum/contracts/zksync/libraries/Diamond.sol
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,10 @@ library Diamond {
) private {
DiamondStorage storage ds = getDiamondStorage();

require(_facet != address(0), "G"); // facet with zero address cannot be added
// Facet with no code cannot be added.
// This check also verifies that the facet does not have zero address, since it is the
// address with which 0x00000000 selector is associated.
require(_facet.code.length > 0, "G");

// Add facet to the list of facets if the facet address is new one
_saveFacetIfNew(_facet);
Expand All @@ -153,7 +156,10 @@ library Diamond {
) private {
DiamondStorage storage ds = getDiamondStorage();

require(_facet != address(0), "K"); // cannot replace facet with zero address
// Facet with no code cannot be added.
// This check also verifies that the facet does not have zero address, since it is the
// address with which 0x00000000 selector is associated.
require(_facet.code.length > 0, "K");
StanislavBreadless marked this conversation as resolved.
Show resolved Hide resolved

uint256 selectorsLength = _selectors.length;
for (uint256 i = 0; i < selectorsLength; i = i.uncheckedInc()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ library TransactionValidator {
_encoded.length,
_transaction.factoryDeps.length,
_transaction.gasPerPubdataByteLimit
) <= _transaction.gasLimit,
) <= l2GasForTxBody,
"up"
);
}
Expand All @@ -51,6 +51,8 @@ library TransactionValidator {
require(_transaction.to <= type(uint160).max, "ub");
require(_transaction.paymaster == 0, "uc");
require(_transaction.value == 0, "ud");
require(_transaction.maxFeePerGas == 0, "uq");
require(_transaction.maxPriorityFeePerGas == 0, "ux");
require(_transaction.reserved[0] == 0, "ue");
require(_transaction.reserved[1] <= type(uint160).max, "uf");
require(_transaction.reserved[2] == 0, "ug");
Expand Down Expand Up @@ -156,7 +158,7 @@ library TransactionValidator {
// }
// batchOverheadForTransaction = Math.max(batchOverheadForTransaction, overheadForPublicData);

// The overhead for ergs that could be used to use single-instance circuits
// The overhead for gas that could be used to use single-instance circuits
uint256 overheadForGas;
{
uint256 numerator = batchOverheadGas * _totalGasLimit + L2_TX_MAX_GAS_LIMIT;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ contract FacetCutTest is DiamondCutTest {

Diamond.FacetCut[] memory facetCuts1 = new Diamond.FacetCut[](1);
facetCuts1[0] = Diamond.FacetCut({
facet: address(0x000000000000000000000000000000000000000A),
facet: address(executorFacet2),
action: Diamond.Action.Add,
isFreezable: true,
selectors: selectors
Expand All @@ -251,7 +251,7 @@ contract FacetCutTest is DiamondCutTest {

Diamond.FacetCut[] memory facetCuts2 = new Diamond.FacetCut[](1);
facetCuts2[0] = Diamond.FacetCut({
facet: address(0x000000000000000000000000000000000000000A),
facet: address(executorFacet2),
action: Diamond.Action.Replace,
isFreezable: false,
selectors: selectors
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ contract Authorization is GovernanceTest {

function test_RevertWhen_CancelByUnauthorisedAddress() public {
vm.prank(randomSigner);
vm.expectRevert("Only the owner and security council are allowed to call this function");
vm.expectRevert("Ownable: caller is not the owner");
governance.cancel(bytes32(0));
}

Expand Down
2 changes: 1 addition & 1 deletion ethereum/test/unit_tests/l2-upgrade.test.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -824,7 +824,7 @@ function buildL2CanonicalTransaction(tx: Partial<L2CanonicalTransaction>): L2Can
txType: SYSTEM_UPGRADE_TX_TYPE,
from: ethers.constants.AddressZero,
to: ethers.constants.AddressZero,
gasLimit: 3000000,
gasLimit: 5000000,
gasPerPubdataByteLimit: REQUIRED_L1_TO_L2_GAS_PER_PUBDATA_LIMIT,
maxFeePerGas: 0,
maxPriorityFeePerGas: 0,
Expand Down
Loading