fix case when there are duplicate writes across phases

AztecProtocol · Feb 26, 2024 · 15f1d82 · 15f1d82
1 parent 9ca1ed6
commit 15f1d82
Show file tree

Hide file tree

Showing 5 changed files with 155 additions and 95 deletions.
diff --git a/yarn-project/circuits.js/src/structs/kernel/combined_accumulated_data.ts b/yarn-project/circuits.js/src/structs/kernel/combined_accumulated_data.ts
@@ -1,7 +1,6 @@
 import { makeTuple } from '@aztec/foundation/array';
 import { padArrayEnd } from '@aztec/foundation/collection';
 import { Fr } from '@aztec/foundation/fields';
-import { createDebugLogger } from '@aztec/foundation/log';
 import { BufferReader, Tuple, serializeToBuffer } from '@aztec/foundation/serialize';
 
 import {
@@ -33,7 +32,7 @@ import { NullifierKeyValidationRequestContext } from '../nullifier_key_validatio
 import { SideEffect, SideEffectLinkedToNoteHash } from '../side_effects.js';
 import { NewContractData } from './new_contract_data.js';
 
-const log = createDebugLogger('aztec:combined_accumulated_data');
+// const log = createDebugLogger('aztec:combined_accumulated_data');
 
 /**
  * Read operations from the public state tree.
@@ -87,6 +86,10 @@ export class PublicDataRead {
   toFriendlyJSON() {
     return `Leaf=${this.leafSlot.toFriendlyJSON()}: ${this.value.toFriendlyJSON()}`;
   }
+
+  equals(other: PublicDataRead) {
+    return this.leafSlot.equals(other.leafSlot) && this.value.equals(other.value);
+  }
 }
 
 /**
@@ -129,6 +132,10 @@ export class PublicDataUpdateRequest {
     return this.leafSlot.isZero() && this.newValue.isZero();
   }
 
+  static isEmpty(x: PublicDataUpdateRequest) {
+    return x.isEmpty();
+  }
+
   equals(other: PublicDataUpdateRequest) {
     return this.leafSlot.equals(other.leafSlot) && this.newValue.equals(other.newValue);
   }
@@ -320,14 +327,14 @@ export class CombinedAccumulatedData {
     );
 
     const nonSquashedWrites = [
-      ...nonRevertible.publicDataUpdateRequests,
       ...revertible.publicDataUpdateRequests,
+      ...nonRevertible.publicDataUpdateRequests,
     ].filter(x => !x.isEmpty());
 
-    log.debug('nonSquashedWrites');
-    for (const write of nonSquashedWrites) {
-      log.debug(write.toFriendlyJSON());
-    }
+    // log.debug('nonSquashedWrites');
+    // for (const write of nonSquashedWrites) {
+    //   log.debug(write.toFriendlyJSON());
+    // }
 
     const squashedWrites = Array.from(
       nonSquashedWrites
@@ -338,10 +345,10 @@ export class CombinedAccumulatedData {
         .values(),
     );
 
-    log.debug('squashedWrites');
-    for (const write of squashedWrites) {
-      log.debug(write.toFriendlyJSON());
-    }
+    // log.debug('squashedWrites');
+    // for (const write of squashedWrites) {
+    //   log.debug(write.toFriendlyJSON());
+    // }
 
     const publicDataUpdateRequests = padArrayEnd(
       squashedWrites,

diff --git a/yarn-project/end-to-end/src/e2e_fees.test.ts b/yarn-project/end-to-end/src/e2e_fees.test.ts
@@ -48,7 +48,7 @@ describe('e2e_fees', () => {
       walletClient: deployL1ContractsValues.walletClient,
       wallet: wallets[0],
       logger,
-      mockL1: true,
+      mockL1: false,
     });
 
     gasTokenContract = gasBridgeTestHarness.l2Token;
@@ -133,23 +133,24 @@ describe('e2e_fees', () => {
     /**
      * PRIVATE SETUP
      * check authwit
-     * reduce alice BC.private by FeeAmount
-     * enqueue public call to increase FPC BC.public by FeeAmount
+     * reduce alice BC.private by MaxFee
+     * enqueue public call to increase FPC BC.public by MaxFee
      * enqueue public call for fpc.pay_fee
      *
      * PUBLIC SETUP
-     * increase FPC BC.public by FeeAmount
+     * increase FPC BC.public by MaxFee
      *
      * PUBLIC APP LOGIC
      * increase alice BC.public by MintedBananasAmount
+     * increase BC total supply by MintedBananasAmount
      *
      * PUBLIC TEARDOWN
      * call gas.pay_fee
      *   decrease FPC AZT by FeeAmount
      *   increase sequencer AZT by FeeAmount
      * call banana.transfer_public
-     *   decrease FPC BC.public by 0n
-     *   increase alice BC.public by 0n
+     *   decrease FPC BC.public by RefundAmount
+     *   increase alice BC.public by RefundAmount
      *
      */
     await bananaCoin.methods
@@ -170,12 +171,12 @@ describe('e2e_fees', () => {
     await expectMapping(
       bananaPublicBalances,
       [aliceAddress, bananaFPC.address, sequencerAddress],
-      [MintedBananasAmount + RefundAmount, MaxFee, 0n],
+      [MintedBananasAmount + RefundAmount, MaxFee - RefundAmount, 0n],
     );
     await expectMapping(
       gasBalances,
       [aliceAddress, bananaFPC.address, sequencerAddress],
-      [0n, InitialFPCGas - MaxFee, FeeAmount],
+      [0n, InitialFPCGas - FeeAmount, FeeAmount],
     );
   }, 100_000);
 

diff --git a/yarn-project/sequencer-client/src/sequencer/abstract_phase_manager.ts b/yarn-project/sequencer-client/src/sequencer/abstract_phase_manager.ts
@@ -401,73 +401,69 @@ export abstract class AbstractPhaseManager {
       publicDataUpdateRequests: nonRevertiblePublicDataUpdateRequests,
     } = publicInputs.endNonRevertibleData; // from kernel
 
-    // Convert ContractStorage* objects to PublicData* objects and sort them in execution order
+    // Convert ContractStorage* objects to PublicData* objects and sort them in execution order.
+    // Note, this only pulls simulated reads/writes from the current phase,
+    // so the returned result will be a subset of the public kernel output.
+
     const simPublicDataReads = collectPublicDataReads(execResult);
+    // verify that each read is in the kernel output
+    for (const read of simPublicDataReads) {
+      if (
+        !revertiblePublicDataReads.find(item => item.equals(read)) &&
+        !nonRevertiblePublicDataReads.find(item => item.equals(read))
+      ) {
+        throw new Error(
+          `Public data reads from simulator do not match those from public kernel.\nFrom simulator: ${simPublicDataReads
+            .map(p => p.toFriendlyJSON())
+            .join(', ')}\nFrom public kernel revertible: ${revertiblePublicDataReads
+            .map(i => i.toFriendlyJSON())
+            .join(', ')}\nFrom public kernel non-revertible: ${nonRevertiblePublicDataReads
+            .map(i => i.toFriendlyJSON())
+            .join(', ')}`,
+        );
+      }
+    }
+
     const simPublicDataUpdateRequests = collectPublicDataUpdateRequests(execResult);
+    for (const update of simPublicDataUpdateRequests) {
+      if (
+        !revertiblePublicDataUpdateRequests.find(item => item.equals(update)) &&
+        !nonRevertiblePublicDataUpdateRequests.find(item => item.equals(update))
+      ) {
+        throw new Error(
+          `Public data update requests from simulator do not match those from public kernel.\nFrom simulator: ${simPublicDataUpdateRequests
+            .map(p => p.toFriendlyJSON())
+            .join(', ')}\nFrom public kernel revertible: ${revertiblePublicDataUpdateRequests
+            .map(i => i.toFriendlyJSON())
+            .join(', ')}\nFrom public kernel non-revertible: ${nonRevertiblePublicDataUpdateRequests
+            .map(i => i.toFriendlyJSON())
+            .join(', ')}`,
+        );
+      }
+    }
 
+    // Now grab the simulated public data reads and updates that are also in the kernel output
     const simRevertiblePublicDataReads = simPublicDataReads.filter(read =>
-      revertiblePublicDataReads.find(item => item.leafSlot.equals(read.leafSlot) && item.value.equals(read.value)),
-    );
-    const simRevertiblePublicDataUpdateRequests = simPublicDataUpdateRequests.filter(update =>
-      revertiblePublicDataUpdateRequests.find(
-        item => item.leafSlot.equals(update.leafSlot) && item.newValue.equals(update.newValue),
-      ),
+      revertiblePublicDataReads.find(item => item.equals(read)),
     );
-
     const simNonRevertiblePublicDataReads = simPublicDataReads.filter(read =>
-      nonRevertiblePublicDataReads.find(item => item.leafSlot.equals(read.leafSlot) && item.value.equals(read.value)),
+      nonRevertiblePublicDataReads.find(item => item.equals(read)),
+    );
+    const simRevertiblePublicDataUpdateRequests = simPublicDataUpdateRequests.filter(update =>
+      revertiblePublicDataUpdateRequests.find(item => item.equals(update)),
     );
     const simNonRevertiblePublicDataUpdateRequests = simPublicDataUpdateRequests.filter(update =>
-      nonRevertiblePublicDataUpdateRequests.find(
-        item => item.leafSlot.equals(update.leafSlot) && item.newValue.equals(update.newValue),
-      ),
+      nonRevertiblePublicDataUpdateRequests.find(item => item.equals(update)),
     );
 
-    // Assume that kernel public inputs has the right number of items.
     // We only want to reorder the items from the public inputs of the
     // most recently processed top/enqueued call.
-    const numRevertibleReadsInKernel = arrayNonEmptyLength(publicInputs.end.publicDataReads, f => f.isEmpty());
-    const numRevertibleUpdatesInKernel = arrayNonEmptyLength(publicInputs.end.publicDataUpdateRequests, f =>
-      f.isEmpty(),
-    );
-    const numNonRevertibleReadsInKernel = arrayNonEmptyLength(publicInputs.endNonRevertibleData.publicDataReads, f =>
+    const numRevertibleReadsInKernel = arrayNonEmptyLength(revertiblePublicDataReads, f => f.isEmpty());
+    const numRevertibleUpdatesInKernel = arrayNonEmptyLength(revertiblePublicDataUpdateRequests, f => f.isEmpty());
+    const numNonRevertibleReadsInKernel = arrayNonEmptyLength(nonRevertiblePublicDataReads, f => f.isEmpty());
+    const numNonRevertibleUpdatesInKernel = arrayNonEmptyLength(nonRevertiblePublicDataUpdateRequests, f =>
       f.isEmpty(),
     );
-    const numNonRevertibleUpdatesInKernel = arrayNonEmptyLength(
-      publicInputs.endNonRevertibleData.publicDataUpdateRequests,
-      f => f.isEmpty(),
-    );
-
-    // Validate all items in enqueued public calls are in the kernel emitted stack
-    const readsAreEqual =
-      simRevertiblePublicDataReads.length + simNonRevertiblePublicDataReads.length === simPublicDataReads.length;
-
-    const updatesAreEqual =
-      simRevertiblePublicDataUpdateRequests.length + simNonRevertiblePublicDataUpdateRequests.length ===
-      simPublicDataUpdateRequests.length;
-
-    if (!readsAreEqual) {
-      throw new Error(
-        `Public data reads from simulator do not match those from public kernel.\nFrom simulator: ${simPublicDataReads
-          .map(p => p.toFriendlyJSON())
-          .join(', ')}\nFrom public kernel revertible: ${revertiblePublicDataReads
-          .map(i => i.toFriendlyJSON())
-          .join(', ')}\nFrom public kernel non-revertible: ${nonRevertiblePublicDataReads
-          .map(i => i.toFriendlyJSON())
-          .join(', ')}`,
-      );
-    }
-    if (!updatesAreEqual) {
-      throw new Error(
-        `Public data update requests from simulator do not match those from public kernel.\nFrom simulator: ${simPublicDataUpdateRequests
-          .map(p => p.toFriendlyJSON())
-          .join(', ')}\nFrom public kernel revertible: ${revertiblePublicDataUpdateRequests
-          .map(i => i.toFriendlyJSON())
-          .join(', ')}\nFrom public kernel non-revertible: ${nonRevertiblePublicDataUpdateRequests
-          .map(i => i.toFriendlyJSON())
-          .join(', ')}`,
-      );
-    }
 
     const numRevertibleReadsBeforeThisEnqueuedCall = numRevertibleReadsInKernel - simRevertiblePublicDataReads.length;
     const numRevertibleUpdatesBeforeThisEnqueuedCall =