From 56af7536643bd64f1c7af919082d4cc34b967fbc Mon Sep 17 00:00:00 2001 From: Alex Gherghisan Date: Sat, 16 Mar 2024 19:16:38 +0000 Subject: [PATCH] feat: sequencer checks fee balance --- yarn-project/end-to-end/src/e2e_fees.test.ts | 28 +++ .../src/client/sequencer-client.ts | 1 + .../src/sequencer/sequencer.test.ts | 6 + .../src/sequencer/sequencer.ts | 4 + .../src/sequencer/tx_validator.test.ts | 189 ++++++++++++++++-- .../src/sequencer/tx_validator.ts | 81 +++++++- 6 files changed, 286 insertions(+), 23 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_fees.test.ts b/yarn-project/end-to-end/src/e2e_fees.test.ts index 0f455f840d4..794a359d775 100644 --- a/yarn-project/end-to-end/src/e2e_fees.test.ts +++ b/yarn-project/end-to-end/src/e2e_fees.test.ts @@ -506,6 +506,34 @@ describe('e2e_fees', () => { addPendingShieldNoteToPXE(0, RefundAmount, computeMessageSecretHash(RefundSecret), tx.txHash), ).resolves.toBeUndefined(); }); + + it("rejects txs that don't have enough balance to cover gas costs", async () => { + // deploy a copy of bananaFPC but don't fund it! + const bankruptFPC = await FPCContract.deploy(aliceWallet, bananaCoin.address, gasTokenContract.address) + .send() + .deployed(); + + await expectMapping(gasBalances, [bankruptFPC.address], [0n]); + + await expect( + bananaCoin.methods + .privately_mint_private_note(10) + .send({ + // we need to skip public simulation otherwise the PXE refuses to accept the TX + skipPublicSimulation: true, + fee: { + maxFee: MaxFee, + paymentMethod: new PrivateFeePaymentMethod( + bananaCoin.address, + bankruptFPC.address, + aliceWallet, + RefundSecret, + ), + }, + }) + .wait(), + ).rejects.toThrow('Tx dropped by P2P node.'); + }); }); it('fails transaction that error in setup', async () => { diff --git a/yarn-project/sequencer-client/src/client/sequencer-client.ts b/yarn-project/sequencer-client/src/client/sequencer-client.ts index 2afed68da5d..b85130d5716 100644 --- a/yarn-project/sequencer-client/src/client/sequencer-client.ts +++ b/yarn-project/sequencer-client/src/client/sequencer-client.ts @@ -95,6 +95,7 @@ export class SequencerClient { l1ToL2MessageSource, publicProcessorFactory, config, + config.l1Contracts.gasPortalAddress, ); await sequencer.start(); diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts index 0b6e9e496ce..61ccb97a346 100644 --- a/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/sequencer.test.ts @@ -92,6 +92,8 @@ describe('sequencer', () => { it('builds a block out of a single tx', async () => { const tx = mockTx(); tx.data.constants.txContext.chainId = chainId; + tx.data.needsSetup = false; + tx.data.needsTeardown = false; const block = L2Block.random(lastBlockNumber + 1); const proof = makeEmptyProof(); @@ -119,6 +121,8 @@ describe('sequencer', () => { const txs = [mockTx(0x10000), mockTx(0x20000), mockTx(0x30000)]; txs.forEach(tx => { tx.data.constants.txContext.chainId = chainId; + tx.data.needsSetup = false; + tx.data.needsTeardown = false; }); const doubleSpendTx = txs[1]; const block = L2Block.random(lastBlockNumber + 1); @@ -157,6 +161,8 @@ describe('sequencer', () => { const txs = [mockTx(0x10000), mockTx(0x20000), mockTx(0x30000)]; txs.forEach(tx => { tx.data.constants.txContext.chainId = chainId; + tx.data.needsSetup = false; + tx.data.needsTeardown = false; }); const invalidChainTx = txs[1]; const block = L2Block.random(lastBlockNumber + 1); diff --git a/yarn-project/sequencer-client/src/sequencer/sequencer.ts b/yarn-project/sequencer-client/src/sequencer/sequencer.ts index 9050ee1a983..84ce6bf3dfe 100644 --- a/yarn-project/sequencer-client/src/sequencer/sequencer.ts +++ b/yarn-project/sequencer-client/src/sequencer/sequencer.ts @@ -12,6 +12,7 @@ import { WorldStateStatus, WorldStateSynchronizer } from '@aztec/world-state'; import { BlockBuilder } from '../block_builder/index.js'; import { GlobalVariableBuilder } from '../global_variable_builder/global_builder.js'; import { L1Publisher } from '../publisher/l1-publisher.js'; +import { WorldStatePublicDB } from '../simulator/public_executor.js'; import { ceilPowerOfTwo } from '../utils.js'; import { SequencerConfig } from './config.js'; import { ProcessedTx } from './processed_tx.js'; @@ -48,6 +49,7 @@ export class Sequencer { private l1ToL2MessageSource: L1ToL2MessageSource, private publicProcessorFactory: PublicProcessorFactory, config: SequencerConfig = {}, + private gasPortalAddress = EthAddress.ZERO, private log = createDebugLogger('aztec:sequencer'), ) { this.updateConfig(config); @@ -179,6 +181,8 @@ export class Sequencer { return trees.findLeafIndex(MerkleTreeId.NULLIFIER_TREE, nullifier.toBuffer()); }, }, + new WorldStatePublicDB(trees), + this.gasPortalAddress, newGlobalVariables, ); diff --git a/yarn-project/sequencer-client/src/sequencer/tx_validator.test.ts b/yarn-project/sequencer-client/src/sequencer/tx_validator.test.ts index f3ecf36691e..7076b6e1cc0 100644 --- a/yarn-project/sequencer-client/src/sequencer/tx_validator.test.ts +++ b/yarn-project/sequencer-client/src/sequencer/tx_validator.test.ts @@ -1,30 +1,59 @@ import { mockTx as baseMockTx } from '@aztec/circuit-types'; -import { Fr, GlobalVariables } from '@aztec/circuits.js'; -import { makeGlobalVariables } from '@aztec/circuits.js/testing'; +import { + AztecAddress, + CallContext, + CallRequest, + EthAddress, + Fr, + FunctionData, + FunctionSelector, + GlobalVariables, + MAX_NON_REVERTIBLE_PUBLIC_CALL_STACK_LENGTH_PER_TX, + MAX_REVERTIBLE_PUBLIC_CALL_STACK_LENGTH_PER_TX, + PublicCallRequest, +} from '@aztec/circuits.js'; +import { makeAztecAddress, makeGlobalVariables } from '@aztec/circuits.js/testing'; +import { makeTuple } from '@aztec/foundation/array'; +import { pedersenHash } from '@aztec/foundation/crypto'; +import { getCanonicalGasTokenAddress } from '@aztec/protocol-contracts/gas-token'; import { MockProxy, mock, mockFn } from 'jest-mock-extended'; -import { NullifierSource, TxValidator } from './tx_validator.js'; +import { NullifierSource, PublicStateSource, TxValidator } from './tx_validator.js'; describe('TxValidator', () => { let validator: TxValidator; let globalVariables: GlobalVariables; let nullifierSource: MockProxy; + let publicStateSource: MockProxy; + let gasPortalAddress: EthAddress; + let gasTokenAddress: AztecAddress; beforeEach(() => { + gasPortalAddress = EthAddress.random(); + gasTokenAddress = getCanonicalGasTokenAddress(gasPortalAddress); nullifierSource = mock({ getNullifierIndex: mockFn().mockImplementation(() => { return Promise.resolve(undefined); }), }); + publicStateSource = mock({ + storageRead: mockFn().mockImplementation((contractAddress: AztecAddress, _slot: Fr) => { + if (contractAddress.equals(gasTokenAddress)) { + return Promise.resolve(new Fr(1)); + } else { + return Promise.reject(Fr.ZERO); + } + }), + }); globalVariables = makeGlobalVariables(); - validator = new TxValidator(nullifierSource, globalVariables); + validator = new TxValidator(nullifierSource, publicStateSource, gasPortalAddress, globalVariables); }); describe('inspects tx metadata', () => { it('allows only transactions for the right chain', async () => { - const goodTx = mockTx(); - const badTx = mockTx(); + const goodTx = nonFeePayingTx(); + const badTx = nonFeePayingTx(); badTx.data.constants.txContext.chainId = Fr.random(); await expect(validator.validateTxs([goodTx, badTx])).resolves.toEqual([[goodTx], [badTx]]); @@ -33,45 +62,179 @@ describe('TxValidator', () => { describe('inspects tx nullifiers', () => { it('rejects duplicates in non revertible data', async () => { - const badTx = mockTx(); + const badTx = nonFeePayingTx(); badTx.data.endNonRevertibleData.newNullifiers[1] = badTx.data.endNonRevertibleData.newNullifiers[0]; await expect(validator.validateTxs([badTx])).resolves.toEqual([[], [badTx]]); }); it('rejects duplicates in revertible data', async () => { - const badTx = mockTx(); + const badTx = nonFeePayingTx(); badTx.data.end.newNullifiers[1] = badTx.data.end.newNullifiers[0]; await expect(validator.validateTxs([badTx])).resolves.toEqual([[], [badTx]]); }); it('rejects duplicates across phases', async () => { - const badTx = mockTx(); + const badTx = nonFeePayingTx(); badTx.data.end.newNullifiers[0] = badTx.data.endNonRevertibleData.newNullifiers[0]; await expect(validator.validateTxs([badTx])).resolves.toEqual([[], [badTx]]); }); it('rejects duplicates across txs', async () => { - const firstTx = mockTx(); - const secondTx = mockTx(); + const firstTx = nonFeePayingTx(); + const secondTx = nonFeePayingTx(); secondTx.data.end.newNullifiers[0] = firstTx.data.end.newNullifiers[0]; await expect(validator.validateTxs([firstTx, secondTx])).resolves.toEqual([[firstTx], [secondTx]]); }); it('rejects duplicates against history', async () => { - const badTx = mockTx(); + const badTx = nonFeePayingTx(); nullifierSource.getNullifierIndex.mockReturnValueOnce(Promise.resolve(1n)); await expect(validator.validateTxs([badTx])).resolves.toEqual([[], [badTx]]); }); }); + describe('inspects tx gas', () => { + it('allows native fee paying txs', async () => { + const sender = makeAztecAddress(); + const expectedBalanceSlot = pedersenHash([new Fr(1).toBuffer(), sender.toBuffer()]); + const tx = nativeFeePayingTx(sender); + + publicStateSource.storageRead.mockImplementation((address, slot) => { + if (address.equals(gasTokenAddress) && slot.equals(expectedBalanceSlot)) { + return Promise.resolve(new Fr(1)); + } else { + return Promise.resolve(Fr.ZERO); + } + }); + + await expect(validator.validateTxs([tx])).resolves.toEqual([[tx], []]); + }); + + it('rejects native fee paying txs if out of balance', async () => { + const sender = makeAztecAddress(); + const expectedBalanceSlot = pedersenHash([new Fr(1).toBuffer(), sender.toBuffer()]); + const tx = nativeFeePayingTx(sender); + + publicStateSource.storageRead.mockImplementation((address, slot) => { + if (address.equals(gasTokenAddress) && slot.equals(expectedBalanceSlot)) { + return Promise.resolve(Fr.ZERO); + } else { + return Promise.resolve(new Fr(1)); + } + }); + + await expect(validator.validateTxs([tx])).resolves.toEqual([[], [tx]]); + }); + + it('allows txs paying through a fee payment contract', async () => { + const fpcAddress = makeAztecAddress(); + const expectedBalanceSlot = pedersenHash([new Fr(1).toBuffer(), fpcAddress.toBuffer()]); + const tx = fxFeePayingTx(fpcAddress); + + publicStateSource.storageRead.mockImplementation((address, slot) => { + if (address.equals(gasTokenAddress) && slot.equals(expectedBalanceSlot)) { + return Promise.resolve(new Fr(1)); + } else { + return Promise.resolve(Fr.ZERO); + } + }); + + await expect(validator.validateTxs([tx])).resolves.toEqual([[tx], []]); + }); + + it('rejects txs paying through a fee payment contract out of balance', async () => { + const fpcAddress = makeAztecAddress(); + const expectedBalanceSlot = pedersenHash([new Fr(1).toBuffer(), fpcAddress.toBuffer()]); + const tx = nativeFeePayingTx(fpcAddress); + + publicStateSource.storageRead.mockImplementation((address, slot) => { + if (address.equals(gasTokenAddress) && slot.equals(expectedBalanceSlot)) { + return Promise.resolve(Fr.ZERO); + } else { + return Promise.resolve(new Fr(1)); + } + }); + + await expect(validator.validateTxs([tx])).resolves.toEqual([[], [tx]]); + }); + }); + // get unique txs that are also stable across test runs let txSeed = 1; /** Creates a mock tx for the current chain */ - function mockTx() { + function nonFeePayingTx() { const tx = baseMockTx(txSeed++, false); + tx.data.constants.txContext.chainId = globalVariables.chainId; tx.data.constants.txContext.version = globalVariables.version; + // clear public call stacks as it's mocked data but the arrays are not correlated + tx.data.endNonRevertibleData.publicCallStack = makeTuple( + MAX_NON_REVERTIBLE_PUBLIC_CALL_STACK_LENGTH_PER_TX, + CallRequest.empty, + ); + tx.data.end.publicCallStack = makeTuple(MAX_REVERTIBLE_PUBLIC_CALL_STACK_LENGTH_PER_TX, CallRequest.empty); + // use splice because it's a readonly property + tx.enqueuedPublicFunctionCalls.splice(0, tx.enqueuedPublicFunctionCalls.length); + + // clear these flags because the call stack is empty now + tx.data.needsSetup = false; + tx.data.needsAppLogic = false; + tx.data.needsTeardown = false; + + return tx; + } + + /** Create a tx that pays for its cost natively */ + function nativeFeePayingTx(feePayer: AztecAddress) { + const tx = nonFeePayingTx(); + const gasTokenAddress = getCanonicalGasTokenAddress(gasPortalAddress); + const signature = FunctionSelector.random(); + + const feeExecutionFn = new PublicCallRequest( + gasTokenAddress, + new FunctionData(signature, false), + new CallContext(feePayer, gasTokenAddress, gasPortalAddress, signature, false, false, 1), + CallContext.empty(), + [], + ); + + tx.data.endNonRevertibleData.publicCallStack[0] = feeExecutionFn.toCallRequest(); + tx.enqueuedPublicFunctionCalls[0] = feeExecutionFn; + tx.data.needsTeardown = true; + + return tx; + } + + /** Create a tx that uses fee abstraction to pay for its cost */ + function fxFeePayingTx(feePaymentContract: AztecAddress) { + const tx = nonFeePayingTx(); + + // the contract calls itself. Both functions are internal + const feeSetupSelector = FunctionSelector.random(); + const feeSetupFn = new PublicCallRequest( + feePaymentContract, + new FunctionData(feeSetupSelector, true), + new CallContext(feePaymentContract, feePaymentContract, EthAddress.ZERO, feeSetupSelector, false, false, 1), + CallContext.empty(), + [], + ); + tx.data.endNonRevertibleData.publicCallStack[0] = feeSetupFn.toCallRequest(); + tx.enqueuedPublicFunctionCalls[0] = feeSetupFn; + tx.data.needsSetup = true; + + const feeExecutionSelector = FunctionSelector.random(); + const feeExecutionFn = new PublicCallRequest( + feePaymentContract, + new FunctionData(feeExecutionSelector, true), + new CallContext(feePaymentContract, feePaymentContract, EthAddress.ZERO, feeExecutionSelector, false, false, 2), + CallContext.empty(), + [], + ); + tx.data.endNonRevertibleData.publicCallStack[1] = feeExecutionFn.toCallRequest(); + tx.enqueuedPublicFunctionCalls[1] = feeExecutionFn; + tx.data.needsTeardown = true; + return tx; } }); diff --git a/yarn-project/sequencer-client/src/sequencer/tx_validator.ts b/yarn-project/sequencer-client/src/sequencer/tx_validator.ts index a11bfea27f5..271b9a2928c 100644 --- a/yarn-project/sequencer-client/src/sequencer/tx_validator.ts +++ b/yarn-project/sequencer-client/src/sequencer/tx_validator.ts @@ -1,13 +1,22 @@ import { Tx } from '@aztec/circuit-types'; -import { Fr, GlobalVariables } from '@aztec/circuits.js'; +import { AztecAddress, EthAddress, Fr, GlobalVariables } from '@aztec/circuits.js'; +import { pedersenHash } from '@aztec/foundation/crypto'; import { Logger, createDebugLogger } from '@aztec/foundation/log'; +import { getCanonicalGasTokenAddress } from '@aztec/protocol-contracts/gas-token'; +import { AbstractPhaseManager, PublicKernelPhase } from './abstract_phase_manager.js'; import { ProcessedTx } from './processed_tx.js'; +/** A source of what nullifiers have been committed to the state trees */ export interface NullifierSource { getNullifierIndex: (nullifier: Fr) => Promise; } +/** Provides a view into public contract state */ +export interface PublicStateSource { + storageRead: (contractAddress: AztecAddress, slot: Fr) => Promise; +} + // prefer symbols over booleans so it's clear what the intention is // vs returning true/false is tied to the function name // eg. isDoubleSpend vs isValidChain assign different meanings to booleans @@ -16,18 +25,27 @@ const INVALID_TX = Symbol('invalid_tx'); type TxValidationStatus = typeof VALID_TX | typeof INVALID_TX; +// the storage slot associated with "storage.balances" +const GAS_TOKEN_BALANCES_SLOT = new Fr(1); + export class TxValidator { #log: Logger; #globalVariables: GlobalVariables; #nullifierSource: NullifierSource; + #publicStateSource: PublicStateSource; + #gasPortalAddress: EthAddress; constructor( nullifierSource: NullifierSource, + publicStateSource: PublicStateSource, + gasPortalAddress: EthAddress, globalVariables: GlobalVariables, log = createDebugLogger('aztec:sequencer:tx_validator'), ) { this.#nullifierSource = nullifierSource; this.#globalVariables = globalVariables; + this.#publicStateSource = publicStateSource; + this.#gasPortalAddress = gasPortalAddress; this.#log = log; } @@ -53,6 +71,12 @@ export class TxValidator { continue; } + // skip already processed transactions + if (tx instanceof Tx && (await this.#validateFee(tx)) === INVALID_TX) { + invalidTxs.push(tx); + continue; + } + validTxs.push(tx); } @@ -89,18 +113,20 @@ export class TxValidator { * @returns Whether this is a problematic double spend that the L1 contract would reject. */ async #validateNullifiers(tx: Tx | ProcessedTx, thisBlockNullifiers: Set): Promise { - const newNullifiers = TxValidator.#extractNullifiers(tx); + const newNullifiers = [...tx.data.endNonRevertibleData.newNullifiers, ...tx.data.end.newNullifiers] + .filter(x => !x.isEmpty()) + .map(x => x.value.toBigInt()); - // Ditch this tx if it has a repeated nullifiers + // Ditch this tx if it has repeated nullifiers const uniqueNullifiers = new Set(newNullifiers); if (uniqueNullifiers.size !== newNullifiers.length) { - this.#log.warn(`Rejecting tx for emitting duplicate nullifiers, tx hash ${Tx.getHash(tx)}`); + this.#log.warn(`Rejecting tx ${Tx.getHash(tx)} for emitting duplicate nullifiers`); return INVALID_TX; } for (const nullifier of newNullifiers) { if (thisBlockNullifiers.has(nullifier)) { - this.#log.warn(`Rejecting tx for repeating a in the same block, tx hash ${Tx.getHash(tx)}`); + this.#log.warn(`Rejecting tx ${Tx.getHash(tx)} for repeating a nullifier in the same block`); return INVALID_TX; } @@ -113,16 +139,51 @@ export class TxValidator { const hasDuplicates = nullifierIndexes.some(index => index !== undefined); if (hasDuplicates) { - this.#log.warn(`Rejecting tx for repeating nullifiers from the past, tx hash ${Tx.getHash(tx)}`); + this.#log.warn(`Rejecting tx ${Tx.getHash(tx)} for repeating nullifiers present in state trees`); return INVALID_TX; } return VALID_TX; } - static #extractNullifiers(tx: Tx | ProcessedTx): bigint[] { - return [...tx.data.endNonRevertibleData.newNullifiers, ...tx.data.end.newNullifiers] - .filter(x => !x.isEmpty()) - .map(x => x.value.toBigInt()); + async #validateFee(tx: Tx): Promise { + if (!tx.data.needsTeardown) { + // TODO check if fees are mandatory and reject this tx + this.#log.debug(`Tx ${Tx.getHash(tx)} doesn't pay for gas`); + return VALID_TX; + } + + const { + // TODO what if there's more than one function call? + // if we're to enshrine that teardown = 1 function call, then we should turn this into a single function call + [PublicKernelPhase.TEARDOWN]: [teardownFn], + } = AbstractPhaseManager.extractEnqueuedPublicCallsByPhase(tx.data, tx.enqueuedPublicFunctionCalls); + + if (!teardownFn) { + this.#log.warn( + `Rejecting tx ${Tx.getHash(tx)} because it should pay for gas but has no enqueued teardown function call`, + ); + return INVALID_TX; + } + + // TODO(#1204) if a generator index is used for the derived storage slot of a map, update it here as well + const slot = pedersenHash([GAS_TOKEN_BALANCES_SLOT.toBuffer(), teardownFn.callContext.msgSender.toBuffer()]); + const gasBalance = await this.#publicStateSource.storageRead( + getCanonicalGasTokenAddress(this.#gasPortalAddress), + slot, + ); + + // TODO(#5004) calculate fee needed based on tx limits and gas prices + const gasAmountNeeded = new Fr(1); + if (gasBalance.lt(gasAmountNeeded)) { + this.#log.warn( + `Rejecting tx ${Tx.getHash( + tx, + )} because it should pay for gas but has insufficient balance ${gasBalance.toShortString()} < ${gasAmountNeeded.toShortString()}`, + ); + return INVALID_TX; + } + + return VALID_TX; } }