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

Constrain timestamp in global variables #948

Merged
merged 12 commits into from
Jul 3, 2023
18 changes: 16 additions & 2 deletions l1-contracts/src/core/Rollup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ contract Rollup is IRollup {
uint256 public immutable VERSION;

bytes32 public rollupStateHash;
uint256 public lastBlockTs;

constructor(IRegistry _registry) {
VERIFIER = new MockVerifier();
Expand Down Expand Up @@ -63,6 +64,7 @@ contract Rollup is IRollup {
}

rollupStateHash = newStateHash;
lastBlockTs = block.timestamp;

// @todo (issue #605) handle fee collector
// @todo: (issue #624) handle different versions
Expand All @@ -77,14 +79,14 @@ contract Rollup is IRollup {
}

function _constrainGlobals(bytes calldata _l2Block) internal view {
// @todo issue #830 Constrain timestamp

uint256 chainId;
uint256 version;
uint256 ts;
// block number already constrained by start state hash
assembly {
chainId := calldataload(_l2Block.offset)
version := calldataload(add(_l2Block.offset, 0x20))
ts := calldataload(add(_l2Block.offset, 0x60))
}

if (block.chainid != chainId) {
Expand All @@ -94,5 +96,17 @@ contract Rollup is IRollup {
if (version != VERSION) {
revert Errors.Rollup__InvalidVersion(version, VERSION);
}

if (ts > block.timestamp) {
revert Errors.Rollup__TimestampInFuture();
}

// @todo @LHerskind consider if this is too strict
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a relevant issue for us to come back to this later or is the plan to hash this out now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have not created an issue for it. Will add it to #824.

// This will make multiple l2 blocks in the same l1 block impractical.
// e.g., the first block will update timestamp which will make the second fail.
// Could possibly allow multiple blocks if in same l1 block
if (ts < lastBlockTs) {
revert Errors.Rollup__TimestampTooOld();
}
}
}
2 changes: 2 additions & 0 deletions l1-contracts/src/core/libraries/Errors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,6 @@ library Errors {
error Rollup__InvalidProof(); // 0xa5b2ba17
error Rollup__InvalidChainId(uint256 expected, uint256 actual); // 0x37b5bc12
error Rollup__InvalidVersion(uint256 expected, uint256 actual); // 0x9ef30794
error Rollup__TimestampInFuture(); // 0xbc1ce916
error Rollup__TimestampTooOld(); // 0x72ed9c81
}
26 changes: 24 additions & 2 deletions l1-contracts/test/Rollup.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ contract RollupTest is DecoderTest {
assertEq(rollup.rollupStateHash(), endStateHash, "Invalid rollup state hash");
}

function testInvalidChainId() public {
function testRevertInvalidChainId() public {
bytes memory block_ = block_empty_1;
assembly {
mstore(add(block_, 0x20), 0x420)
Expand All @@ -54,7 +54,7 @@ contract RollupTest is DecoderTest {
rollup.process(bytes(""), block_);
}

function testInvalidVersion() public {
function testRevertInvalidVersion() public {
bytes memory block_ = block_empty_1;
assembly {
mstore(add(block_, 0x40), 0x420)
Expand All @@ -64,6 +64,28 @@ contract RollupTest is DecoderTest {
rollup.process(bytes(""), block_);
}

function testRevertTimestampInFuture() public {
bytes memory block_ = block_empty_1;

uint256 ts = block.timestamp + 1;
assembly {
mstore(add(block_, 0x80), ts)
}

vm.expectRevert(abi.encodeWithSelector(Errors.Rollup__TimestampInFuture.selector));
rollup.process(bytes(""), block_);
}

function testRevertTimestampTooOld() public {
bytes memory block_ = block_empty_1;

// Overwrite in the rollup contract
vm.store(address(rollup), bytes32(uint256(1)), bytes32(uint256(block.timestamp)));

vm.expectRevert(abi.encodeWithSelector(Errors.Rollup__TimestampTooOld.selector));
rollup.process(bytes(""), block_);
}

function testMixBlock() public override(DecoderTest) {
(,, bytes32 endStateHash,, bytes32[] memory l2ToL1Msgs, bytes32[] memory l1ToL2Msgs) =
helper.decode(block_mixed_1);
Expand Down
5 changes: 2 additions & 3 deletions yarn-project/end-to-end/src/integration_l1_publisher.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -267,12 +267,11 @@ describe('L1Publisher integration', () => {
await makeBloatedProcessedTx(128 * i + 96),
await makeBloatedProcessedTx(128 * i + 128),
];
// @todo @LHerskind fix time.
const globalVariables = new GlobalVariables(
new Fr(config.chainId),
new Fr(config.version),
new Fr(1 + i),
Fr.ZERO,
new Fr(await rollup.read.lastBlockTs()),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want the ts of the last block here? Do we not want the current timestamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ts of the last block is the minimum value that will work. With the current timestamp, you have the issue of dealing with anvil not progressing time as "outside" so this was just an easy way to pick a meaningful value.

);
const [block] = await builder.buildL2Block(globalVariables, txs, l1ToL2Messages);

Expand Down Expand Up @@ -363,7 +362,7 @@ describe('L1Publisher integration', () => {
new Fr(config.chainId),
new Fr(config.version),
new Fr(1 + i),
Fr.ZERO,
new Fr(await rollup.read.lastBlockTs()),
);
const [block] = await builder.buildL2Block(globalVariables, txs, l1ToL2Messages);

Expand Down

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions yarn-project/sequencer-client/src/client/sequencer-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { getL1Publisher, getVerificationKeys, Sequencer } from '../index.js';
import { EmptyRollupProver } from '../prover/empty.js';
import { PublicProcessorFactory } from '../sequencer/public_processor.js';
import { WasmRollupCircuitSimulator } from '../simulator/rollup.js';
import { getGlobalVariableBuilder } from '../global_variable_builder/index.js';

/**
* Encapsulates the full sequencer and publisher.
Expand All @@ -34,6 +35,7 @@ export class SequencerClient {
l1ToL2MessageSource: L1ToL2MessageSource,
) {
const publisher = getL1Publisher(config);
const globalsBuilder = getGlobalVariableBuilder(config);
const merkleTreeDb = worldStateSynchroniser.getLatest();

const blockBuilder = new SoloBlockBuilder(
Expand All @@ -47,6 +49,7 @@ export class SequencerClient {

const sequencer = new Sequencer(
publisher,
globalsBuilder,
p2pClient,
worldStateSynchroniser,
blockBuilder,
Expand Down
3 changes: 2 additions & 1 deletion yarn-project/sequencer-client/src/config.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { SequencerConfig } from './sequencer/config.js';
import { PublisherConfig, TxSenderConfig } from './publisher/config.js';
import { EthAddress } from '@aztec/foundation/eth-address';
import { GlobalReaderConfig } from './global_variable_builder/index.js';

/**
* Configuration settings for the SequencerClient.
*/
export type SequencerClientConfig = PublisherConfig & TxSenderConfig & SequencerConfig;
export type SequencerClientConfig = PublisherConfig & TxSenderConfig & SequencerConfig & GlobalReaderConfig;

/**
* Creates an instance of SequencerClientConfig out of environment variables using sensible defaults for integration testing if not set.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { EthAddress } from '@aztec/circuits.js';

/**
* Configuration of the L1GlobalReader.
*/
export interface GlobalReaderConfig {
/**
* Rollup contract address.
*/
rollupContract: EthAddress;
/**
* The RPC Url of the ethereum host.
*/
rpcUrl: string;
/**
* The API key of the ethereum host.
*/
apiKey?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this on sequencer config already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so it is just to not require the entire config to be passed to the builder. If you look at the other configs, you will see similar with values that are "shared" between them, so the full config makes up the union of them.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { Fr, GlobalVariables } from '@aztec/circuits.js';
import { createDebugLogger } from '@aztec/foundation/log';

/**
* Reads values from L1 state that is used for the global values.
*/
export interface L1GlobalReader {
getLastTimestamp(): Promise<bigint>;
getVersion(): Promise<bigint>;
getChainId(): Promise<bigint>;
}

/**
* Builds global variables from L1 state.
*/
export interface GlobalVariableBuilder {
buildGlobalVariables(blockNumber: Fr): Promise<GlobalVariables>;
}

/**
* Simple implementation of a builder that uses the minimum time possible for the global variables.
*/
export class SimpleGlobalVariableBuilder implements GlobalVariableBuilder {
private log = createDebugLogger('aztec:sequencer:simple_global_variable_builder');
constructor(private readonly reader: L1GlobalReader) {}

/**
* Simple builder of global variables that use the minimum time possible.
* @param blockNumber - The block number to build global variables for.
* @returns The global variables for the given block number.
*/
public async buildGlobalVariables(blockNumber: Fr): Promise<GlobalVariables> {
const lastTimestamp = new Fr(await this.reader.getLastTimestamp());
Copy link
Member

Choose a reason for hiding this comment

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

ibid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above, also hinted in the comments above the function.

const version = new Fr(await this.reader.getVersion());
const chainId = new Fr(await this.reader.getChainId());

this.log(
`Built global variables for block ${blockNumber}: (${chainId}, ${version}, ${blockNumber}, ${lastTimestamp})`,
);

return new GlobalVariables(chainId, version, blockNumber, lastTimestamp);
}
}
15 changes: 15 additions & 0 deletions yarn-project/sequencer-client/src/global_variable_builder/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { GlobalReaderConfig } from './config.js';
import { GlobalVariableBuilder, SimpleGlobalVariableBuilder } from './global_builder.js';
import { ViemReader } from './viem-reader.js';

export { SimpleGlobalVariableBuilder } from './global_builder.js';
export { GlobalReaderConfig } from './config.js';

/**
* Returns a new instance of the global variable builder.
* @param config - Configuration to initialize the builder.
* @returns A new instance of the global variable builder.
*/
export function getGlobalVariableBuilder(config: GlobalReaderConfig): GlobalVariableBuilder {
return new SimpleGlobalVariableBuilder(new ViemReader(config));
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { createEthereumChain } from '@aztec/ethereum';
import { RollupAbi } from '@aztec/l1-artifacts';
import {
GetContractReturnType,
PublicClient,
HttpTransport,
createPublicClient,
http,
getContract,
getAddress,
} from 'viem';
import * as chains from 'viem/chains';
import { GlobalReaderConfig } from './config.js';
import { L1GlobalReader } from './global_builder.js';

/**
* Reads values from L1 state using viem.
*/
export class ViemReader implements L1GlobalReader {
private rollupContract: GetContractReturnType<typeof RollupAbi, PublicClient<HttpTransport, chains.Chain>>;
private publicClient: PublicClient<HttpTransport, chains.Chain>;

constructor(config: GlobalReaderConfig) {
const { rpcUrl, apiKey, rollupContract: rollupContractAddress } = config;

const chain = createEthereumChain(rpcUrl, apiKey);

this.publicClient = createPublicClient({
chain: chain.chainInfo,
transport: http(chain.rpcUrl),
});

this.rollupContract = getContract({
address: getAddress(rollupContractAddress.toString()),
abi: RollupAbi,
publicClient: this.publicClient,
});
}

/**
* Fetches the last timestamp that a block was processed by the contract.
* @returns The last timestamp that a block was processed by the contract.
*/
public async getLastTimestamp(): Promise<bigint> {
return BigInt(await this.rollupContract.read.lastBlockTs());
}

/**
* Fetches the version of the rollup contract.
* @returns The version of the rollup contract.
*/
public async getVersion(): Promise<bigint> {
return BigInt(await this.rollupContract.read.VERSION());
}

/**
* Gets the chain id.
* @returns The chain id.
*/
public async getChainId(): Promise<bigint> {
return await Promise.resolve(BigInt(this.publicClient.chain.id));
}
}
10 changes: 10 additions & 0 deletions yarn-project/sequencer-client/src/sequencer/sequencer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ import { L1Publisher, makeTx } from '../index.js';
import { makeEmptyProcessedTx, makeProcessedTx } from './processed_tx.js';
import { PublicProcessor, PublicProcessorFactory } from './public_processor.js';
import { Sequencer } from './sequencer.js';
import { GlobalVariableBuilder } from '../global_variable_builder/global_builder.js';

describe('sequencer', () => {
let publisher: MockProxy<L1Publisher>;
let globalVariableBuilder: MockProxy<GlobalVariableBuilder>;
let p2p: MockProxy<P2P>;
let worldState: MockProxy<WorldStateSynchroniser>;
let blockBuilder: MockProxy<BlockBuilder>;
Expand All @@ -38,6 +40,7 @@ describe('sequencer', () => {
lastBlockNumber = 0;

publisher = mock<L1Publisher>();
globalVariableBuilder = mock<GlobalVariableBuilder>();
merkleTreeOps = mock<MerkleTreeOperations>();
blockBuilder = mock<BlockBuilder>();

Expand Down Expand Up @@ -69,6 +72,7 @@ describe('sequencer', () => {

sequencer = new TestSubject(
publisher,
globalVariableBuilder,
p2p,
worldState,
blockBuilder,
Expand All @@ -90,6 +94,9 @@ describe('sequencer', () => {
p2p.getTxs.mockResolvedValueOnce([tx]);
blockBuilder.buildL2Block.mockResolvedValueOnce([block, proof]);
publisher.processL2Block.mockResolvedValueOnce(true);
globalVariableBuilder.buildGlobalVariables.mockResolvedValueOnce(
new GlobalVariables(chainId, version, new Fr(lastBlockNumber + 1), Fr.ZERO),
);

await sequencer.initialSync();
await sequencer.work();
Expand All @@ -113,6 +120,9 @@ describe('sequencer', () => {
p2p.getTxs.mockResolvedValueOnce(txs);
blockBuilder.buildL2Block.mockResolvedValueOnce([block, proof]);
publisher.processL2Block.mockResolvedValueOnce(true);
globalVariableBuilder.buildGlobalVariables.mockResolvedValueOnce(
new GlobalVariables(chainId, version, new Fr(lastBlockNumber + 1), Fr.ZERO),
);

// We make a nullifier from tx1 a part of the nullifier tree, so it gets rejected as double spend
const doubleSpendNullifier = doubleSpendTx.data.end.newNullifiers[0].toBuffer();
Expand Down
6 changes: 3 additions & 3 deletions yarn-project/sequencer-client/src/sequencer/sequencer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { SequencerConfig } from './config.js';
import { ProcessedTx } from './processed_tx.js';
import { PublicProcessorFactory } from './public_processor.js';
import { GlobalVariables } from '@aztec/circuits.js';
import { GlobalVariableBuilder } from '../global_variable_builder/global_builder.js';

/**
* Sequencer client
Expand All @@ -42,6 +43,7 @@ export class Sequencer {

constructor(
private publisher: L1Publisher,
private globalsBuilder: GlobalVariableBuilder,
private p2pClient: P2P,
private worldState: WorldStateSynchroniser,
private blockBuilder: BlockBuilder,
Expand Down Expand Up @@ -136,9 +138,8 @@ export class Sequencer {
this.log(`Processing ${validTxs.length} txs...`);
this.state = SequencerState.CREATING_BLOCK;

// @todo @LHerskind Fetch meaningful timestamp in here.
const blockNumber = (await this.l2BlockSource.getBlockHeight()) + 1;
const globalVariables = new GlobalVariables(this.chainId, this.version, new Fr(blockNumber), Fr.ZERO);
const globalVariables = await this.globalsBuilder.buildGlobalVariables(new Fr(blockNumber));

// Process public txs and drop the ones that fail processing
// We create a fresh processor each time to reset any cached state (eg storage writes)
Expand All @@ -163,7 +164,6 @@ export class Sequencer {
this.log(`Assembling block with txs ${processedTxs.map(tx => tx.hash).join(', ')}`);
const emptyTx = await processor.makeEmptyProcessedTx(this.chainId, this.version);

// @todo @LHerskind We need to pass in the globals here as well to build a block with the correct data.
const block = await this.buildBlock(processedTxs, l1ToL2Messages, emptyTx, globalVariables);
this.log(`Assembled block ${block.number}`);

Expand Down