Merge branch 'next' into merge-train/barretenberg

Author: AztecBot

barretenberg/cpp/pil/vm2/tx.pil

@@ -525,7 +525,7 @@ namespace tx;
         note_hash_tree_check.next_root
     };
 
-    // If reverted, the next state is unconstrained. This is okay since we are going to restore the state at the end of setup or fail proving.
+    // If reverted, the next state is unconstrained as `should_note_hash_append = 0`. This is okay since we are going to restore the state at the end of setup or fail proving.
     // Namely, #[RESTORE_STATE_ON_REVERT] restores the tree state to its value at the end of setup.
     should_note_hash_append * (prev_note_hash_tree_size + 1 - next_note_hash_tree_size) = 0;
     should_note_hash_append * (prev_num_note_hashes_emitted + 1 - next_num_note_hashes_emitted) = 0;
@@ -556,12 +556,12 @@ namespace tx;
     pol commit nullifier_tree_height;
     should_nullifier_append * (nullifier_tree_height - constants.NULLIFIER_TREE_HEIGHT) = 0;
 
-    // We revert if the nullifier already exists.
+    // Collisions are disallowed, if the nullifier already exists the transaction is unprovable
     #[NULLIFIER_APPEND]
     should_nullifier_append {
         leaf_value,
         prev_nullifier_tree_root,
-        reverted,
+        precomputed.zero, // Nullifiers emitted in tx trace cannot have existed
         next_nullifier_tree_root,
         prev_nullifier_tree_size,
         discard, // from tx_discard.pil virtual trace
@@ -580,10 +580,10 @@ namespace tx;
         indexed_tree_check.sel_silo
     };
 
-    // If reverted, the next state is unconstrained. This is okay since we are going to restore the state at the end of setup.
+    // If reverted, the next state is unconstrained since `should_nullifier_append = 0`. This is okay since we are going to restore the state at the end of setup.
     // Namely, #[RESTORE_STATE_ON_REVERT] restores the tree state to its value at the end of setup.
-    should_nullifier_append * (1 - reverted) * (prev_nullifier_tree_size + 1 - next_nullifier_tree_size) = 0;
-    should_nullifier_append * (1 - reverted) * (prev_num_nullifiers_emitted + 1 - next_num_nullifiers_emitted) = 0;
+    should_nullifier_append * (prev_nullifier_tree_size + 1 - next_nullifier_tree_size) = 0;
+    should_nullifier_append * (prev_num_nullifiers_emitted + 1 - next_num_nullifiers_emitted) = 0;
 
     // ===== L2 - L1 Messages =====
     pol commit should_try_l2_l1_msg_append; // @boolean (follows from definition)

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/tx.test.cpp

@@ -849,4 +849,38 @@ TEST_F(TxExecutionConstrainingWithCalldataTest, SimpleHandleCalldata)
     check_relation<bb::avm2::calldata_hashing<FF>>(trace);
     check_all_interactions<tracegen::CalldataTraceBuilder>(trace);
 }
+
+// Verify that the nullifier state increment is unconditional when should_nullifier_append = 1.
+// A malicious prover cannot set reverted = 1 to skip the state increment.
+TEST(TxExecutionConstrainingTest, NegativeNullifierStateIncrementIsUnconditional)
+{
+    // Subrelation 42: should_nullifier_append * (prev_nullifier_tree_size + 1 - next_nullifier_tree_size) = 0
+    // Subrelation 43: should_nullifier_append * (prev_num_nullifiers_emitted + 1 - next_num_nullifiers_emitted) = 0
+    TestTraceContainer trace({
+        {
+            // Row 0
+            { C::precomputed_first_row, 1 },
+        },
+        {
+            // Row 1: Nullifier append with should_nullifier_append = 1 but state not incremented.
+            { C::tx_sel, 1 },
+            { C::tx_should_nullifier_append, 1 },
+            { C::tx_reverted, 1 }, // Prover tries to cheat by setting reverted = 1.
+            { C::tx_prev_nullifier_tree_size, 5 },
+            { C::tx_next_nullifier_tree_size, 5 }, // Should be 6.
+            { C::tx_prev_num_nullifiers_emitted, 3 },
+            { C::tx_next_num_nullifiers_emitted, 3 }, // Should be 4.
+        },
+    });
+
+    // Subrelation 42: tree size must increment.
+    EXPECT_THROW_WITH_MESSAGE(check_relation<tx>(trace, static_cast<size_t>(42)), "subrelation 42");
+    // Fix tree size, break emitted count.
+    trace.set(C::tx_next_nullifier_tree_size, 1, 6);
+    EXPECT_THROW_WITH_MESSAGE(check_relation<tx>(trace, static_cast<size_t>(43)), "subrelation 43");
+    // Fix emitted count — both should pass now.
+    trace.set(C::tx_next_num_nullifiers_emitted, 1, 4);
+    check_relation<tx>(trace, static_cast<size_t>(42), static_cast<size_t>(43));
+}
+
 } // namespace bb::avm2::constraining

barretenberg/cpp/src/barretenberg/vm2/generated/relations/lookups_tx.hpp

@@ -181,7 +181,7 @@ struct lookup_tx_nullifier_append_settings_ {
     static constexpr std::array<ColumnAndShifts, LOOKUP_TUPLE_SIZE> SRC_COLUMNS = {
         ColumnAndShifts::tx_leaf_value,
         ColumnAndShifts::tx_prev_nullifier_tree_root,
-        ColumnAndShifts::tx_reverted,
+        ColumnAndShifts::precomputed_zero,
         ColumnAndShifts::tx_next_nullifier_tree_root,
         ColumnAndShifts::tx_prev_nullifier_tree_size,
         ColumnAndShifts::tx_discard,

barretenberg/cpp/src/barretenberg/vm2/generated/relations/tx.hpp

@@ -16,7 +16,7 @@ template <typename FF_> class txImpl {
 
     static constexpr std::array<size_t, 64> SUBRELATION_PARTIAL_LENGTHS = {
         3, 3, 3, 3, 3, 3, 2, 3, 5, 3, 3, 3, 5, 4, 3, 3, 5, 3, 3, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4, 2, 3, 5,
-        3, 3, 3, 3, 3, 5, 4, 3, 3, 3, 4, 4, 3, 5, 4, 3, 4, 3, 2, 4, 4, 4, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3
+        3, 3, 3, 3, 3, 5, 4, 3, 3, 3, 3, 3, 3, 5, 4, 3, 4, 3, 2, 4, 4, 4, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3
     };
 
     template <typename AllEntities> inline static bool skip(const AllEntities& in)

barretenberg/cpp/src/barretenberg/vm2/generated/relations/tx_impl.hpp

@@ -342,15 +342,13 @@ void txImpl<FF_>::accumulate(ContainerOverSubrelations& evals,
     {
         using View = typename std::tuple_element_t<42, ContainerOverSubrelations>::View;
         auto tmp = static_cast<View>(in.get(C::tx_should_nullifier_append)) *
-                   (FF(1) - static_cast<View>(in.get(C::tx_reverted))) *
                    ((static_cast<View>(in.get(C::tx_prev_nullifier_tree_size)) + FF(1)) -
                     static_cast<View>(in.get(C::tx_next_nullifier_tree_size)));
         std::get<42>(evals) += (tmp * scaling_factor);
     }
     {
         using View = typename std::tuple_element_t<43, ContainerOverSubrelations>::View;
         auto tmp = static_cast<View>(in.get(C::tx_should_nullifier_append)) *
-                   (FF(1) - static_cast<View>(in.get(C::tx_reverted))) *
                    ((static_cast<View>(in.get(C::tx_prev_num_nullifiers_emitted)) + FF(1)) -
                     static_cast<View>(in.get(C::tx_next_num_nullifiers_emitted)));
         std::get<43>(evals) += (tmp * scaling_factor);

barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/tx_execution.cpp

@@ -58,9 +58,12 @@ std::string get_halting_information(const EnqueuedCallResult& result)
  * - Cleanup (11)
  *
  * If an error occurs during non-revertible insertions or a Setup phase enqueued call fails,
- * the transaction is considered unprovable and an unrecoverable TxExecutionException is thrown.
- * If an error occurs during revertible insertions or App Logic phase, all the state changes are reverted
- * to the post-setup state and we continue with the Teardown phase.
+ * the transaction is considered unprovable and an unrecoverable exception is thrown.
+ * If a side-effect limit is reached during revertible insertions or App Logic phase fails, all the state
+ * changes are reverted to the post-setup state and we continue with the Teardown phase.
+ * A nullifier collision during revertible insertions is ALSO unprovable (not revertible): the nullifier
+ * originated from private, so a collision indicates the tx should never have been proposed. The
+ * NullifierCollisionException propagates out of simulate() as-is.
  * If an error occurs during Teardown phase, all the state changes are reverted to the post-setup state and
  * we continue with the Collect Gas Fees phase.
  *
@@ -69,9 +72,11 @@ std::string get_halting_information(const EnqueuedCallResult& result)
  *
  * @param tx The transaction to simulate.
  * @return The result of the transaction simulation.
+ * @throws NullifierCollisionException if a private nullifier collision occurs in either the non-revertible
+ *         or revertible insertion phase.
  * @throws TxExecutionException if
- *         - there is a nullifier collision or the maximum number of
- *           nullifiers, note hashes, or L2 to L1 messages is reached as part of the non-revertible insertions.
+ *         - the maximum number of nullifiers, note hashes, or L2 to L1 messages is reached as part of the
+ *           non-revertible insertions.
  *         - a Setup phase enqueued call fails.
  *         - the fee payer does not have enough balance to pay the fee.
  * Note: Other low-level exceptions of other types are not caught and will be thrown.
@@ -161,8 +166,9 @@ TxExecutionResult TxExecution::simulate(const Tx& tx)
     call_stack_metadata_collector.set_phase(CoarseTransactionPhase::APP_LOGIC);
 
     try {
-        // Insert revertibles. This can throw if there is a nullifier collision.
-        // Such an exception should be handled and the tx be provable.
+        // Insert revertibles. This can throw TxExecutionException on a side-effect limit error
+        // (handled here, tx is provable) or NullifierCollisionException on a collision
+        // (not handled here - propagates as unrecoverable, since the tx is unprovable).
         // We catch separately here to record the revert reason in call stack metadata,
         // since no calls have populated the metadata yet at this point.
         try {
@@ -366,7 +372,8 @@ void TxExecution::emit_public_call_request(const PublicCallRequestWithCalldata&
  *
  * @param revertible Whether the nullifier is revertible.
  * @param nullifier The nullifier to insert.
- * @throws TxExecutionException if the maximum number of nullifiers is reached or a nullifier collision occurs.
+ * @throws TxExecutionException if the maximum number of nullifiers is reached.
+ * @throws NullifierCollisionException if the nullifier collides with an existing one (unrecoverable).
  */
 void TxExecution::emit_nullifier(bool revertible, const FF& nullifier)
 {
@@ -383,7 +390,13 @@ void TxExecution::emit_nullifier(bool revertible, const FF& nullifier)
         try {
             merkle_db.siloed_nullifier_write(nullifier);
         } catch (const NullifierCollisionException& e) {
-            throw TxExecutionException(e.what());
+            // Do not handle the NullifierCollisionException as any collision at the tx execution
+            // level is unprovable (i.e. this is an unrecoverable error).
+            // Rethrow with more information - note that this exception isn't being caught in this file
+            throw NullifierCollisionException(format("[",
+                                                     revertible ? "R" : "NR",
+                                                     "_NULLIFIER_INSERTION] UNRECOVERABLE ERROR! Nullifier collision: ",
+                                                     e.what()));
         }
 
         events.emit(TxPhaseEvent{ .phase = phase,
@@ -489,11 +502,11 @@ void TxExecution::emit_l2_to_l1_message(bool revertible, const ScopedL2ToL1Messa
 /**
  * @brief Insert the non-revertible accumulated data into the Merkle DB and emit corresponding events.
  *        It might error if the limits for the number of allowable inserts are exceeded or a nullifier collision occurs,
- *        but this results in an unprovable tx.
+ *        either of which results in an unprovable tx.
  *
  * @param tx The transaction to insert the non-revertible accumulated data into.
- * @throws TxExecutionException if the maximum number of nullifiers, note hashes, L2 to L1 messages is reached, or a
- *         nullifier collision occurs.
+ * @throws TxExecutionException if the maximum number of nullifiers, note hashes, or L2 to L1 messages is reached.
+ * @throws NullifierCollisionException if a nullifier collision occurs (unrecoverable).
  */
 void TxExecution::insert_non_revertibles(const Tx& tx)
 {
@@ -541,11 +554,12 @@ void TxExecution::insert_non_revertibles(const Tx& tx)
 
 /**
  * @brief Insert the revertible accumulated data into the Merkle DB and emit corresponding events.
- *        It might error if the limits for the number of allowable inserts are exceeded or a nullifier collision occurs.
+ *        A side-effect limit error is recoverable (the caller reverts to post-setup); a nullifier
+ *        collision is unrecoverable (the tx is unprovable).
  *
  * @param tx The transaction to insert the revertible accumulated data into.
- * @throws TxExecutionException if the maximum number of nullifiers, note hashes, L2 to L1 messages is reached, or a
- *         nullifier collision occurs.
+ * @throws TxExecutionException if the maximum number of nullifiers, note hashes, or L2 to L1 messages is reached.
+ * @throws NullifierCollisionException if a nullifier collision occurs (unrecoverable).
  */
 void TxExecution::insert_revertibles(const Tx& tx)
 {

barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/tx_execution.hpp

@@ -76,9 +76,11 @@ class TxExecution final {
     TxContext tx_context;
     bool skip_fee_enforcement;
 
-    // This function can throw if there is a nullifier collision.
+    // Throws TxExecutionException on a side-effect limit error, or NullifierCollisionException on a
+    // nullifier collision. Both are unrecoverable — they result in an unprovable tx.
     void insert_non_revertibles(const Tx& tx);
-    // This function can throw if there is a nullifier collision.
+    // Throws TxExecutionException on a side-effect limit error (recoverable — caller reverts to
+    // post-setup), or NullifierCollisionException on a nullifier collision (unrecoverable — unprovable tx).
     void insert_revertibles(const Tx& tx);
     void emit_public_call_request(const PublicCallRequestWithCalldata& call,
                                   TransactionPhase phase,

barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/tx_execution.test.cpp

@@ -450,5 +450,72 @@ TEST_F(TxExecutionTest, L2ToL1MessageLimitReached)
     EXPECT_EQ(reverts, 1);
 }
 
+TEST_F(TxExecutionTest, RevertibleNullifierCollisionIsUnrecoverable)
+{
+    // A nullifier collision during revertible insertion must propagate as NullifierCollisionException
+    // (not TxExecutionException), bypassing all recovery logic and making the tx unprovable.
+    Tx tx = {
+        .hash = "0x1234567890abcdef",
+        .non_revertible_accumulated_data =
+            AccumulatedData{
+                .nullifiers = testing::random_fields(1),
+            },
+        .revertible_accumulated_data =
+            AccumulatedData{
+                .nullifiers = testing::random_fields(1),
+            },
+        .fee_payer = FF::random_element(),
+    };
+
+    AppendOnlyTreeSnapshot dummy_snapshot = {
+        .root = 0,
+        .next_available_leaf_index = 0,
+    };
+    TreeStates tree_state = {
+        .note_hash_tree = { .tree = dummy_snapshot, .counter = 0 },
+        .nullifier_tree = { .tree = dummy_snapshot, .counter = 0 },
+        .l1_to_l2_message_tree = { .tree = dummy_snapshot, .counter = 0 },
+        .public_data_tree = { .tree = dummy_snapshot, .counter = 0 },
+    };
+    ON_CALL(merkle_db, get_tree_state()).WillByDefault([&]() { return tree_state; });
+
+    // First call (non-revertible) succeeds, second call (revertible) collides.
+    EXPECT_CALL(merkle_db, siloed_nullifier_write(_))
+        .WillOnce([&](const auto& /*nullifier*/) { tree_state.nullifier_tree.counter++; })
+        .WillOnce([](const auto& /*nullifier*/) { throw NullifierCollisionException("Nullifier collision"); });
+
+    EXPECT_THROW(tx_execution.simulate(tx), NullifierCollisionException);
+}
+
+TEST_F(TxExecutionTest, NonRevertibleNullifierCollisionIsUnrecoverable)
+{
+    // A nullifier collision during non-revertible insertion must propagate as NullifierCollisionException.
+    Tx tx = {
+        .hash = "0x1234567890abcdef",
+        .non_revertible_accumulated_data =
+            AccumulatedData{
+                .nullifiers = testing::random_fields(1),
+            },
+        .fee_payer = FF::random_element(),
+    };
+
+    AppendOnlyTreeSnapshot dummy_snapshot = {
+        .root = 0,
+        .next_available_leaf_index = 0,
+    };
+    TreeStates tree_state = {
+        .note_hash_tree = { .tree = dummy_snapshot, .counter = 0 },
+        .nullifier_tree = { .tree = dummy_snapshot, .counter = 0 },
+        .l1_to_l2_message_tree = { .tree = dummy_snapshot, .counter = 0 },
+        .public_data_tree = { .tree = dummy_snapshot, .counter = 0 },
+    };
+    ON_CALL(merkle_db, get_tree_state()).WillByDefault([&]() { return tree_state; });
+    ON_CALL(merkle_db, siloed_nullifier_write(_)).WillByDefault([](const auto& /*nullifier*/) {
+        throw NullifierCollisionException("Nullifier collision");
+    });
+
+    EXPECT_THROW(tx_execution.simulate(tx), NullifierCollisionException);
+}
+
 } // namespace
 } // namespace bb::avm2::simulation

yarn-project/bb-prover/src/avm_proving_tests/avm_check_circuit2.test.ts

@@ -1,3 +1,4 @@
+import { MAX_NULLIFIERS_PER_TX } from '@aztec/constants';
 import { Fr } from '@aztec/foundation/curves/bn254';
 import { AvmTestContractArtifact } from '@aztec/noir-test-contracts.js/AvmTest';
 import { AztecAddress } from '@aztec/stdlib/aztec-address';
@@ -77,6 +78,10 @@ describe('AVM check-circuit – unhappy paths 2', () => {
   });
 
   it('error during revertible insertions - skips to teardown', async () => {
+    // Exceed MAX_NULLIFIERS_PER_TX during revertible insertions so that the revertible phase
+    // errors out and the tx skips to teardown. Note: nullifier *collisions* during revertible
+    // insertions are unprovable (not revertible), so we use the side-effect limit path instead.
+    const revertibleNullifiers = Array.from({ length: MAX_NULLIFIERS_PER_TX }, (_, i) => new Fr(100_000 + i));
     await tester.simProveVerify(
       sender,
       /*setupCalls=*/ [],
@@ -96,9 +101,9 @@ describe('AVM check-circuit – unhappy paths 2', () => {
       },
       /*expectRevert=*/ true,
       /*feePayer=*/ sender,
-      // duplicate nullifiers during revertible insertions!
       /*privateInsertions=*/ {
-        revertible: { nullifiers: [new Fr(100_000), /*duplicate*/ new Fr(100_000)] },
+        // Total nullifiers = 1 non-revertible + MAX_NULLIFIERS_PER_TX revertible = over the limit.
+        revertible: { nullifiers: revertibleNullifiers },
         nonRevertible: { nullifiers: [/*firstNullifier=*/ new Fr(66000)] },
       },
     );