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

Raunak/sequencer client l1 origin check #227

Merged
merged 1 commit into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
33 changes: 32 additions & 1 deletion bindings/go/sequencersoloclient/SequencerSoloClient.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 17 additions & 4 deletions contracts/core/SequencerSignatureVerifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,30 +23,43 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import {AppStateVerifier} from "../base/AppStateVerifier.sol";

/**
* @title OptimisticProofVerifier
* @notice Verifies proofs related to Optimistic Rollup state updates
* @title SequencerSignatureVerifier
* @notice Verifies ECDSA signatures from a sequencer for client updates. Is used by the SequencerSoloClient to verify
* signatures on client updates.
* @author Polymer Labs
*/
contract SequencerSignatureVerifier is AppStateVerifier, ISignatureVerifier {
using RLPReader for RLPReader.RLPItem;
using RLPReader for bytes;

// @notice known L2 Output Oracle contract address to verify state update proofs against
address public immutable SEQUENCER;
address public immutable SEQUENCER; // The trusted sequencer address that polymer p2p signer holds the private key
// to
bytes32 public immutable CHAIN_ID; // Chain ID of the L2 chain for which the sequencer signs over

constructor(address sequencer_, bytes32 chainId_) {
SEQUENCER = sequencer_;
CHAIN_ID = chainId_;
}

/**
* @notice Verifies that the sequencer signature is valid for a given l1 origin. This is used by the
* SequencerSoloClient update client method.
* @param l2BlockNumber The block number of the L2 block that the state update is for
* @param appHash The app hash of the state update to be saved in the parent soloClient contract
* @param l1BlockHash The hash of the L1 origin that the peptide height corresponds to
* @param signature The sequencer's ECDSA over the state update
*/
function verifyStateUpdate(uint256 l2BlockNumber, bytes32 appHash, bytes32 l1BlockHash, bytes calldata signature)
external
view
{
_verifySequencerSignature(l2BlockNumber, appHash, l1BlockHash, signature);
}

/**
* @notice Verify the ECDSA signature of the sequencer over the given l2BLockNumber, peptideAppHash, and origin
* l1BlockHash
*/
function _verifySequencerSignature(
uint256 l2BlockNumber,
bytes32 appHash,
Expand Down
48 changes: 34 additions & 14 deletions contracts/core/SequencerSoloClient.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,45 +20,51 @@ pragma solidity 0.8.15;
import {Ics23Proof} from "../interfaces/IProofVerifier.sol";
import {ISignatureVerifier} from "../interfaces/ISignatureVerifier.sol";
import {ILightClient, LightClientType} from "../interfaces/ILightClient.sol";
import {L1Block} from "optimism/L2/L1Block.sol";

/**
* @title SubfinalityLightClient
* @title SequencerSoloClient
* @author Polymer Labs
* @dev This specific light client implementation uses the same client that is used in the op-stack
* @dev This light client implementation verifies a single signature by the polymer p2p-signer, and updates the light
* client if valid signatures are provided.
*/
contract SequencerSoloClient is ILightClient {
LightClientType public constant LIGHT_CLIENT_TYPE = LightClientType.SequencerLightClient; // Stored as a constant
// for cheap on-chain use

ISignatureVerifier public immutable verifier;
L1Block public immutable l1BlockProvider;

uint8 private _LightClientType = uint8(LIGHT_CLIENT_TYPE); // Also redundantly stored as a private mutable type in
// case it needs to be accessed in any proofs

// consensusStates maps from the height to the appHash.
// consensusStates maps from the height to the appHash, and is modified during state updates if it is a valid state
// update.
mapping(uint256 => uint256) public consensusStates;

ISignatureVerifier public immutable verifier;

error CannotUpdatePendingOptimisticConsensusState();
error InvalidL1Origin();
error CannotUpdateClientWithDifferentAppHash();
error AppHashHasNotPassedFraudProofWindow();
error NonMembershipProofsNotYetImplemented();
error NoConsensusStateAtHeight(uint256 height);

constructor(ISignatureVerifier verifier_) {
constructor(ISignatureVerifier verifier_, L1Block _l1BlockProvider) {
verifier = verifier_;
l1BlockProvider = _l1BlockProvider;
RnkSngh marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @inheritdoc ILightClient
* @param proof An array of bytes that contain the l1blockhash and the sequencer's signature. The first 32 bytes of
* this argument should be
* the l1BlockHash, and the remaining bytes should be the sequencer signature which attests to the peptide AppHash
* this argument should be the l1BlockHash, and the remaining bytes should be the sequencer signature which attests
* to the peptide AppHash
* for that l1BlockHash
*/
function updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) external override {
if (consensusStates[peptideHeight] != 0) {
return;
if (l1BlockProvider.hash() != bytes32(proof[:32])) {
revert InvalidL1Origin();
}
verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]);
consensusStates[peptideHeight] = peptideAppHash;
_updateClient(proof, peptideHeight, peptideAppHash);
RnkSngh marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -67,10 +73,10 @@ contract SequencerSoloClient is ILightClient {
function getState(uint256 height) external view returns (uint256 appHash) {
return _getState(height);
}

/**
* @inheritdoc ILightClient
*/

function verifyMembership(Ics23Proof calldata proof, bytes calldata key, bytes calldata expectedValue)
external
view
Expand All @@ -88,6 +94,20 @@ contract SequencerSoloClient is ILightClient {
revert NonMembershipProofsNotYetImplemented();
}

function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal {
if (consensusStates[peptideHeight] != 0) {
// Note: we don't cache consensusStates[peptideHeight] in mem for gas savings here because this if statement
// would not be triggered too often, and would make the more frequent branch more expensive due to mem
// allocation.
if (consensusStates[peptideHeight] != peptideAppHash) {
revert CannotUpdateClientWithDifferentAppHash();
}
return;
}
verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]);
consensusStates[peptideHeight] = peptideAppHash;
}
Comment on lines +97 to +109
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider gas optimizations in _updateClient.

The function could be optimized for gas efficiency:

  1. Cache consensusStates[peptideHeight] in memory to avoid multiple storage reads
  2. Consider using unchecked blocks for arithmetic operations where overflow is impossible
 function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal {
-    if (consensusStates[peptideHeight] != 0) {
-        if (consensusStates[peptideHeight] != peptideAppHash) {
+    uint256 existingState = consensusStates[peptideHeight];
+    if (existingState != 0) {
+        if (existingState != peptideAppHash) {
             revert CannotUpdateClientWithDifferentAppHash();
         }
         return;
     }
     verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]);
     consensusStates[peptideHeight] = peptideAppHash;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal {
if (consensusStates[peptideHeight] != 0) {
// Note: we don't cache consensusStates[peptideHeight] in mem for gas savings here because this if statement
// would not be triggered too often, and would make the more frequent branch more expensive due to mem
// allocation.
if (consensusStates[peptideHeight] != peptideAppHash) {
revert CannotUpdateClientWithDifferentAppHash();
}
return;
}
verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]);
consensusStates[peptideHeight] = peptideAppHash;
}
function _updateClient(bytes calldata proof, uint256 peptideHeight, uint256 peptideAppHash) internal {
uint256 existingState = consensusStates[peptideHeight];
if (existingState != 0) {
if (existingState != peptideAppHash) {
revert CannotUpdateClientWithDifferentAppHash();
}
return;
}
verifier.verifyStateUpdate(peptideHeight, bytes32(peptideAppHash), bytes32(proof[:32]), proof[32:]);
consensusStates[peptideHeight] = peptideAppHash;
}


/**
* @dev Returns the internal state of the light client at a given height.
*/
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@open-ibc/vibc-core-smart-contracts",
"version": "4.0.4",
"version": "4.0.5",
"main": "dist/index.js",
"bin": {
"verify-vibc-core-smart-contracts": "./dist/scripts/verify-contract-script.js",
Expand Down
1 change: 1 addition & 0 deletions specs/update.spec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
deployer: 'KEY_DEPLOYER'
deployArgs:
- '{{SequencerSigVerifier}}'
- '{{L1BlockAddress}}'

- name: DummyLightClient
description: 'DummyLightClient'
Expand Down
14 changes: 14 additions & 0 deletions src/evm/contracts/SequencerSoloClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export interface SequencerSoloClientInterface extends Interface {
| "LIGHT_CLIENT_TYPE"
| "consensusStates"
| "getState"
| "l1BlockProvider"
| "updateClient"
| "verifier"
| "verifyMembership"
Expand All @@ -80,6 +81,10 @@ export interface SequencerSoloClientInterface extends Interface {
functionFragment: "getState",
values: [BigNumberish]
): string;
encodeFunctionData(
functionFragment: "l1BlockProvider",
values?: undefined
): string;
encodeFunctionData(
functionFragment: "updateClient",
values: [BytesLike, BigNumberish, BigNumberish]
Expand All @@ -103,6 +108,10 @@ export interface SequencerSoloClientInterface extends Interface {
data: BytesLike
): Result;
decodeFunctionResult(functionFragment: "getState", data: BytesLike): Result;
decodeFunctionResult(
functionFragment: "l1BlockProvider",
data: BytesLike
): Result;
decodeFunctionResult(
functionFragment: "updateClient",
data: BytesLike
Expand Down Expand Up @@ -167,6 +176,8 @@ export interface SequencerSoloClient extends BaseContract {

getState: TypedContractMethod<[height: BigNumberish], [bigint], "view">;

l1BlockProvider: TypedContractMethod<[], [string], "view">;

updateClient: TypedContractMethod<
[
proof: BytesLike,
Expand Down Expand Up @@ -204,6 +215,9 @@ export interface SequencerSoloClient extends BaseContract {
getFunction(
nameOrSignature: "getState"
): TypedContractMethod<[height: BigNumberish], [bigint], "view">;
getFunction(
nameOrSignature: "l1BlockProvider"
): TypedContractMethod<[], [string], "view">;
getFunction(
nameOrSignature: "updateClient"
): TypedContractMethod<
Expand Down
Loading
Loading