fix: bad contract txs publishing contract data (#2673)

This PR fixes an issue where a failed transaction's contract still had
its contract data published on chain by the sequencer.

The issue only happens in blocks that have more than one transaction
where one of the transaction's contracts fails to deploy due to a bad
public call. In order to get into this state the PXE must not simulate
the public call (otherwise the tx wouldn't be added to the mempool).

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [x] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [x] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).

yarn-project/boxes/blank/src/index.ts

@@ -14,6 +14,7 @@ import {
 } from '@aztec/aztec.js';
 import { ContractArtifact, FunctionArtifact, encodeArguments } from '@aztec/foundation/abi';
 import { FieldsOf } from '@aztec/foundation/types';
+
 // docs:end:imports
 
 export const contractArtifact: ContractArtifact = BlankContractArtifact;

yarn-project/end-to-end/src/e2e_deploy_contract.test.ts

@@ -1,7 +1,8 @@
 import { AztecAddress, Contract, ContractDeployer, EthAddress, Fr, Wallet, isContractDeployed } from '@aztec/aztec.js';
 import { CompleteAddress, getContractDeploymentInfo } from '@aztec/circuits.js';
 import { DebugLogger } from '@aztec/foundation/log';
-import { TestContractArtifact } from '@aztec/noir-contracts/artifacts';
+import { TestContractArtifact, TokenContractArtifact } from '@aztec/noir-contracts/artifacts';
+import { SequencerClient } from '@aztec/sequencer-client';
 import { PXE, TxStatus } from '@aztec/types';
 
 import { setup } from './fixtures/utils.js';
@@ -11,10 +12,11 @@ describe('e2e_deploy_contract', () => {
   let accounts: CompleteAddress[];
   let logger: DebugLogger;
   let wallet: Wallet;
+  let sequencer: SequencerClient | undefined;
   let teardown: () => Promise<void>;
 
   beforeEach(async () => {
-    ({ teardown, pxe, accounts, logger, wallet } = await setup());
+    ({ teardown, pxe, accounts, logger, wallet, sequencer } = await setup());
   }, 100_000);
 
   afterEach(() => teardown());
@@ -123,4 +125,47 @@ describe('e2e_deploy_contract', () => {
       portalContract.toString(),
     );
   });
+
+  it('it should not deploy a contract which failed the public part of the execution', async () => {
+    sequencer?.updateSequencerConfig({
+      minTxsPerBlock: 2,
+    });
+
+    try {
+      // This test requires at least another good transaction to go through in the same block as the bad one.
+      // I deployed the same contract again but it could really be any valid transaction here.
+      const goodDeploy = new ContractDeployer(TokenContractArtifact, wallet).deploy(AztecAddress.random());
+      const badDeploy = new ContractDeployer(TokenContractArtifact, wallet).deploy(AztecAddress.ZERO);
+
+      await Promise.all([
+        goodDeploy.simulate({ skipPublicSimulation: true }),
+        badDeploy.simulate({ skipPublicSimulation: true }),
+      ]);
+
+      const [goodTx, badTx] = [
+        goodDeploy.send({ skipPublicSimulation: true }),
+        badDeploy.send({ skipPublicSimulation: true }),
+      ];
+
+      const [goodTxPromiseResult, badTxReceiptResult] = await Promise.allSettled([goodTx.wait(), badTx.wait()]);
+
+      expect(goodTxPromiseResult.status).toBe('fulfilled');
+      expect(badTxReceiptResult.status).toBe('rejected');
+
+      const [goodTxReceipt, badTxReceipt] = await Promise.all([goodTx.getReceipt(), badTx.getReceipt()]);
+
+      expect(goodTxReceipt.blockNumber).toEqual(expect.any(Number));
+      expect(badTxReceipt.blockNumber).toBeUndefined();
+
+      await expect(pxe.getExtendedContractData(goodDeploy.completeAddress!.address)).resolves.toBeDefined();
+      await expect(pxe.getExtendedContractData(goodDeploy.completeAddress!.address)).resolves.toBeDefined();
+
+      await expect(pxe.getContractData(badDeploy.completeAddress!.address)).resolves.toBeUndefined();
+      await expect(pxe.getExtendedContractData(badDeploy.completeAddress!.address)).resolves.toBeUndefined();
+    } finally {
+      sequencer?.updateSequencerConfig({
+        minTxsPerBlock: 1,
+      });
+    }
+  });
 });

yarn-project/end-to-end/src/fixtures/utils.ts

@@ -28,6 +28,7 @@ import {
   RollupBytecode,
 } from '@aztec/l1-artifacts';
 import { PXEService, createPXEService, getPXEServiceConfig } from '@aztec/pxe';
+import { SequencerClient } from '@aztec/sequencer-client';
 import { AztecNode, L2BlockL2Logs, LogType, PXE, createAztecNodeRpcClient } from '@aztec/types';
 
 import * as path from 'path';
@@ -193,6 +194,7 @@ async function setupWithSandbox(account: Account, config: AztecNodeConfig, logge
   const teardown = () => Promise.resolve();
   return {
     aztecNode,
+    sequencer: undefined,
     pxe: pxeClient,
     deployL1ContractsValues,
     accounts: await pxeClient!.getRegisteredAccounts(),
@@ -212,6 +214,8 @@ type SetupOptions = { /** State load */ stateLoad?: string } & Partial<AztecNode
 export type EndToEndContext = {
   /** The Aztec Node service or client a connected to it. */
   aztecNode: AztecNode | undefined;
+  /** A client to the sequencer service */
+  sequencer: SequencerClient | undefined;
   /** The Private eXecution Environment (PXE). */
   pxe: PXE;
   /** Return values from deployL1Contracts function. */
@@ -273,6 +277,7 @@ export async function setup(numberOfAccounts = 1, opts: SetupOptions = {}): Prom
 
   logger('Creating and synching an aztec node...');
   const aztecNode = await AztecNodeService.createAndSync(config);
+  const sequencer = aztecNode.getSequencer();
 
   const { pxe, accounts, wallets } = await setupPXEService(numberOfAccounts, aztecNode!, logger);
 
@@ -293,6 +298,7 @@ export async function setup(numberOfAccounts = 1, opts: SetupOptions = {}): Prom
     wallets,
     logger,
     cheatCodes,
+    sequencer,
     teardown,
   };
 }

yarn-project/noir-contracts/Nargo.toml

@@ -1,8 +1,8 @@
 [workspace]
 members = [
-    "src/contracts/benchmarking_contract", 
-    "src/contracts/card_game_contract", 
-    "src/contracts/child_contract", 
+    "src/contracts/benchmarking_contract",
+    "src/contracts/card_game_contract",
+    "src/contracts/child_contract",
     "src/contracts/docs_example_contract",
     "src/contracts/easy_private_token_contract",
     "src/contracts/ecdsa_account_contract",

yarn-project/noir-contracts/src/contracts/token_contract/src/main.nr

@@ -36,8 +36,8 @@ contract Token {
     // docs:start:import_authwit
     use dep::authwit::{
         auth::{
-            assert_current_call_valid_authwit, 
-            assert_current_call_valid_authwit_public, 
+            assert_current_call_valid_authwit,
+            assert_current_call_valid_authwit_public,
         },
     };
     // docs:end:import_authwit
@@ -363,6 +363,7 @@ contract Token {
     internal fn _initialize(
         new_admin: AztecAddress,
     ) {
+        assert(new_admin.address != 0, "invalid admin");
         storage.admin.write(new_admin);
         storage.minters.at(new_admin.address).write(true);
     }

yarn-project/sequencer-client/src/sequencer/processed_tx.ts

@@ -6,13 +6,13 @@ import {
   PublicKernelPublicInputs,
   makeEmptyProof,
 } from '@aztec/circuits.js';
-import { Tx, TxHash, TxL2Logs } from '@aztec/types';
+import { ExtendedContractData, Tx, TxHash, TxL2Logs } from '@aztec/types';
 
 /**
  * Represents a tx that has been processed by the sequencer public processor,
  * so its kernel circuit public inputs are filled in.
  */
-export type ProcessedTx = Pick<Tx, 'proof' | 'encryptedLogs' | 'unencryptedLogs'> & {
+export type ProcessedTx = Pick<Tx, 'proof' | 'encryptedLogs' | 'unencryptedLogs' | 'newContracts'> & {
   /**
    * Output of the public kernel circuit for this tx.
    */
@@ -78,6 +78,7 @@ export async function makeProcessedTx(
     proof: proof ?? tx.proof,
     encryptedLogs: tx.encryptedLogs,
     unencryptedLogs: tx.unencryptedLogs,
+    newContracts: tx.newContracts,
     isEmpty: false,
   };
 }
@@ -104,6 +105,7 @@ export function makeEmptyProcessedTx(
     unencryptedLogs: new TxL2Logs([]),
     data: emptyKernelOutput,
     proof: emptyProof,
+    newContracts: [ExtendedContractData.empty()],
     isEmpty: true,
   });
 }

yarn-project/sequencer-client/src/sequencer/public_processor.test.ts

@@ -97,6 +97,7 @@ describe('public_processor', () => {
           proof: tx.proof,
           encryptedLogs: tx.encryptedLogs,
           unencryptedLogs: tx.unencryptedLogs,
+          newContracts: tx.newContracts,
         },
       ]);
       expect(failed).toEqual([]);

yarn-project/sequencer-client/src/sequencer/sequencer.ts

@@ -201,7 +201,7 @@ export class Sequencer {
 
       await assertBlockHeight();
 
-      await this.publishExtendedContractData(validTxs, block);
+      await this.publishExtendedContractData(processedValidTxs, block);
 
       await assertBlockHeight();
 
@@ -218,16 +218,13 @@ export class Sequencer {
    * @param validTxs - The set of real transactions being published as part of the block.
    * @param block - The L2Block to be published.
    */
-  protected async publishExtendedContractData(validTxs: Tx[], block: L2Block) {
+  protected async publishExtendedContractData(validTxs: ProcessedTx[], block: L2Block) {
     // Publishes contract data for txs to the network and awaits the tx to be mined
     this.state = SequencerState.PUBLISHING_CONTRACT_DATA;
     const newContractData = validTxs
       .map(tx => {
         // Currently can only have 1 new contract per tx
-        const newContract = tx.data?.end.newContracts[0];
-        if (newContract) {
-          return tx.newContracts[0];
-        }
+        return tx.newContracts[0];
       })
       .filter((cd): cd is Exclude<typeof cd, undefined> => cd !== undefined);