diff --git a/barretenberg/cpp/src/barretenberg/vm2/common/aztec_types.hpp b/barretenberg/cpp/src/barretenberg/vm2/common/aztec_types.hpp index cfec3e054959..950626595ee2 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/common/aztec_types.hpp +++ b/barretenberg/cpp/src/barretenberg/vm2/common/aztec_types.hpp @@ -29,6 +29,7 @@ enum TransactionPhase { COLLECT_GAS_FEES = 10, TREE_PADDING = 11, CLEANUP = 12, + LAST = CLEANUP, }; using InternalCallId = uint32_t; diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/events/tx_events.hpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/events/tx_events.hpp index 7d3022287aeb..e802d97b9c6c 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/simulation/events/tx_events.hpp +++ b/barretenberg/cpp/src/barretenberg/vm2/simulation/events/tx_events.hpp @@ -48,12 +48,15 @@ struct PadTreesEvent {}; struct CleanupEvent {}; +struct EmptyPhaseEvent {}; + using TxPhaseEventType = std::variant; + CleanupEvent, + EmptyPhaseEvent>; struct TxPhaseEvent { TransactionPhase phase; diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.cpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.cpp index 26b467922dc6..4a2a1c330c79 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.cpp @@ -83,50 +83,11 @@ void TxExecution::simulate(const Tx& tx) insert_non_revertibles(tx); // Setup. - for (const auto& call : tx.setupEnqueuedCalls) { - info("[SETUP] Executing enqueued call to ", call.request.contractAddress); - TxContextEvent state_before = tx_context.serialize_tx_context_event(); - Gas start_gas = tx_context.gas_used; - auto context = context_provider.make_enqueued_context(call.request.contractAddress, - call.request.msgSender, - /*transaction_fee=*/FF(0), - call.calldata, - call.request.isStaticCall, - gas_limit, - start_gas, - tx_context.side_effect_states, - TransactionPhase::SETUP); - // This call should not throw unless it's an unexpected unrecoverable failure. - ExecutionResult result = call_execution.execute(std::move(context)); - tx_context.side_effect_states = result.side_effect_states; - tx_context.gas_used = result.gas_used; - emit_public_call_request(call, - TransactionPhase::SETUP, - /*transaction_fee=*/FF(0), - result.success, - start_gas, - tx_context.gas_used, - state_before, - tx_context.serialize_tx_context_event()); - if (!result.success) { - // This will result in an unprovable tx. - throw TxExecutionException( - format("[SETUP] UNRECOVERABLE ERROR! Enqueued call to ", call.request.contractAddress, " failed")); - } - } - SideEffectStates end_setup_side_effect_states = tx_context.side_effect_states; - - // The checkpoint we should go back to if anything from now on reverts. - merkle_db.create_checkpoint(); - - 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(tx); - - // App logic. - for (const auto& call : tx.appLogicEnqueuedCalls) { - info("[APP_LOGIC] Executing enqueued call to ", call.request.contractAddress); + if (tx.setupEnqueuedCalls.empty()) { + emit_empty_phase(TransactionPhase::SETUP); + } else { + for (const auto& call : tx.setupEnqueuedCalls) { + info("[SETUP] Executing enqueued call to ", call.request.contractAddress); TxContextEvent state_before = tx_context.serialize_tx_context_event(); Gas start_gas = tx_context.gas_used; auto context = context_provider.make_enqueued_context(call.request.contractAddress, @@ -137,13 +98,13 @@ void TxExecution::simulate(const Tx& tx) gas_limit, start_gas, tx_context.side_effect_states, - TransactionPhase::APP_LOGIC); + TransactionPhase::SETUP); // This call should not throw unless it's an unexpected unrecoverable failure. ExecutionResult result = call_execution.execute(std::move(context)); tx_context.side_effect_states = result.side_effect_states; tx_context.gas_used = result.gas_used; emit_public_call_request(call, - TransactionPhase::APP_LOGIC, + TransactionPhase::SETUP, /*transaction_fee=*/FF(0), result.success, start_gas, @@ -151,9 +112,56 @@ void TxExecution::simulate(const Tx& tx) state_before, tx_context.serialize_tx_context_event()); if (!result.success) { - // This exception should be handled and the tx be provable. + // This will result in an unprovable tx. throw TxExecutionException( - format("[APP_LOGIC] Enqueued call to ", call.request.contractAddress, " failed")); + format("[SETUP] UNRECOVERABLE ERROR! Enqueued call to ", call.request.contractAddress, " failed")); + } + } + } + SideEffectStates end_setup_side_effect_states = tx_context.side_effect_states; + + // The checkpoint we should go back to if anything from now on reverts. + merkle_db.create_checkpoint(); + + 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(tx); + + // App logic. + if (tx.appLogicEnqueuedCalls.empty()) { + emit_empty_phase(TransactionPhase::APP_LOGIC); + } else { + for (const auto& call : tx.appLogicEnqueuedCalls) { + info("[APP_LOGIC] Executing enqueued call to ", call.request.contractAddress); + TxContextEvent state_before = tx_context.serialize_tx_context_event(); + Gas start_gas = tx_context.gas_used; + auto context = context_provider.make_enqueued_context(call.request.contractAddress, + call.request.msgSender, + /*transaction_fee=*/FF(0), + call.calldata, + call.request.isStaticCall, + gas_limit, + start_gas, + tx_context.side_effect_states, + TransactionPhase::APP_LOGIC); + // This call should not throw unless it's an unexpected unrecoverable failure. + ExecutionResult result = call_execution.execute(std::move(context)); + tx_context.side_effect_states = result.side_effect_states; + tx_context.gas_used = result.gas_used; + emit_public_call_request(call, + TransactionPhase::APP_LOGIC, + /*transaction_fee=*/FF(0), + result.success, + start_gas, + tx_context.gas_used, + state_before, + tx_context.serialize_tx_context_event()); + if (!result.success) { + // This exception should be handled and the tx be provable. + throw TxExecutionException( + format("[APP_LOGIC] Enqueued call to ", call.request.contractAddress, " failed")); + } } } } catch (const TxExecutionException& e) { @@ -175,7 +183,9 @@ void TxExecution::simulate(const Tx& tx) // Teardown. try { - if (tx.teardownEnqueuedCall) { + if (!tx.teardownEnqueuedCall) { + emit_empty_phase(TransactionPhase::TEARDOWN); + } else { info("[TEARDOWN] Executing enqueued call to ", tx.teardownEnqueuedCall->request.contractAddress); // Teardown has its own gas limit and usage. Gas start_gas = { 0, 0 }; @@ -334,18 +344,30 @@ void TxExecution::insert_non_revertibles(const Tx& tx) tx.hash); // 1. Write the already siloed nullifiers. - for (const auto& nullifier : tx.nonRevertibleAccumulatedData.nullifiers) { - emit_nullifier(false, nullifier); + if (tx.nonRevertibleAccumulatedData.nullifiers.empty()) { + emit_empty_phase(TransactionPhase::NR_NULLIFIER_INSERTION); + } else { + for (const auto& nullifier : tx.nonRevertibleAccumulatedData.nullifiers) { + emit_nullifier(false, nullifier); + } } // 2. Write already unique note hashes. - for (const auto& unique_note_hash : tx.nonRevertibleAccumulatedData.noteHashes) { - emit_note_hash(false, unique_note_hash); + if (tx.nonRevertibleAccumulatedData.noteHashes.empty()) { + emit_empty_phase(TransactionPhase::NR_NOTE_INSERTION); + } else { + for (const auto& unique_note_hash : tx.nonRevertibleAccumulatedData.noteHashes) { + emit_note_hash(false, unique_note_hash); + } } // 3. Write l2_l1 messages - for (const auto& l2_to_l1_msg : tx.nonRevertibleAccumulatedData.l2ToL1Messages) { - emit_l2_to_l1_message(false, l2_to_l1_msg); + if (tx.nonRevertibleAccumulatedData.l2ToL1Messages.empty()) { + emit_empty_phase(TransactionPhase::NR_L2_TO_L1_MESSAGE); + } else { + for (const auto& l2_to_l1_msg : tx.nonRevertibleAccumulatedData.l2ToL1Messages) { + emit_l2_to_l1_message(false, l2_to_l1_msg); + } } } @@ -362,18 +384,30 @@ void TxExecution::insert_revertibles(const Tx& tx) tx.hash); // 1. Write the already siloed nullifiers. - for (const auto& siloed_nullifier : tx.revertibleAccumulatedData.nullifiers) { - emit_nullifier(true, siloed_nullifier); + if (tx.revertibleAccumulatedData.nullifiers.empty()) { + emit_empty_phase(TransactionPhase::R_NULLIFIER_INSERTION); + } else { + for (const auto& siloed_nullifier : tx.revertibleAccumulatedData.nullifiers) { + emit_nullifier(true, siloed_nullifier); + } } // 2. Write the siloed non uniqued note hashes - for (const auto& siloed_note_hash : tx.revertibleAccumulatedData.noteHashes) { - emit_note_hash(true, siloed_note_hash); + if (tx.revertibleAccumulatedData.noteHashes.empty()) { + emit_empty_phase(TransactionPhase::R_NOTE_INSERTION); + } else { + for (const auto& siloed_note_hash : tx.revertibleAccumulatedData.noteHashes) { + emit_note_hash(true, siloed_note_hash); + } } // 3. Write L2 to L1 messages. - for (const auto& l2_to_l1_msg : tx.revertibleAccumulatedData.l2ToL1Messages) { - emit_l2_to_l1_message(true, l2_to_l1_msg); + if (tx.revertibleAccumulatedData.l2ToL1Messages.empty()) { + emit_empty_phase(TransactionPhase::R_L2_TO_L1_MESSAGE); + } else { + for (const auto& l2_to_l1_msg : tx.revertibleAccumulatedData.l2ToL1Messages) { + emit_l2_to_l1_message(true, l2_to_l1_msg); + } } } @@ -426,4 +460,14 @@ void TxExecution::cleanup() .event = CleanupEvent{} }); } +void TxExecution::emit_empty_phase(TransactionPhase phase) +{ + TxContextEvent current_state = tx_context.serialize_tx_context_event(); + events.emit(TxPhaseEvent{ .phase = phase, + .state_before = current_state, + .state_after = current_state, + .reverted = false, + .event = EmptyPhaseEvent{} }); +} + } // namespace bb::avm2::simulation diff --git a/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.hpp b/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.hpp index 99cc93701eb6..f9e56d2a4494 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.hpp +++ b/barretenberg/cpp/src/barretenberg/vm2/simulation/tx_execution.hpp @@ -66,6 +66,8 @@ class TxExecution final { void pad_trees(); void cleanup(); + + void emit_empty_phase(TransactionPhase phase); }; } // namespace bb::avm2::simulation diff --git a/barretenberg/cpp/src/barretenberg/vm2/tracegen/tx_trace.cpp b/barretenberg/cpp/src/barretenberg/vm2/tracegen/tx_trace.cpp index 747aa4f2f3c6..41555a84469b 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/tracegen/tx_trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/tracegen/tx_trace.cpp @@ -28,7 +28,7 @@ template struct overloaded : Ts... { // explicit deduction guide (not needed as of C++20) template overloaded(Ts...) -> overloaded; -constexpr size_t NUM_PHASES = 12; // See TransactionPhase enum +constexpr size_t NUM_PHASES = static_cast(TransactionPhase::LAST); bool is_revertible(TransactionPhase phase) { @@ -529,7 +529,6 @@ void TxTraceBuilder::process(const simulation::EventEmitterInterface(phase_events.size()); @@ -591,7 +578,7 @@ void TxTraceBuilder::process(const simulation::EventEmitterInterface(tx_phase_event->phase) }, { Column::tx_setup_phase_value, static_cast(TransactionPhase::SETUP) }, - { C::tx_is_padded, 0 }, + { C::tx_is_padded, 0 }, // overidden below if this is a skipped phase event { C::tx_start_phase, phase_counter == 0 ? 1 : 0 }, { C::tx_sel_read_phase_length, phase_counter == 0 && !is_one_shot_phase(tx_phase_event->phase) }, { C::tx_is_revertible, is_revertible(tx_phase_event->phase) ? 1 : 0 }, @@ -640,40 +627,23 @@ void TxTraceBuilder::process(const simulation::EventEmitterInterfacephase, 1, 0)); trace.set(row, handle_cleanup()); + }, + [&](const simulation::EmptyPhaseEvent&) { + // EmptyPhaseEvent represents a phase that is not explicitly skipped because of a + // revert, but just has no contents to process, like when app logic starts but has no + // enqueued calls. + trace.set(row, handle_pi_read(tx_phase_event->phase, 0, 0)); + trace.set(row, handle_padded_row(tx_phase_event->phase, gas_used, discard)); } }, tx_phase_event->event); trace.set(row, handle_next_gas_used(gas_used)); trace.set(row, handle_gas_limit(current_gas_limit)); - // Handle a potential phase jump due to a revert, we dont need to check if we are in a revertible phase - // since our witgen will have exited for any reverts in a non-revertible phase. - // If we revert in a phase that isnt TEARDOWN, we jump to TEARDOWN - if (tx_phase_event->reverted && tx_phase_event->phase != TransactionPhase::TEARDOWN) { - // Jump to the TEARDOWN phase - // we need to -2 because of the loop increment and because the enum is 1-indexed - i = static_cast(TransactionPhase::TEARDOWN) - 2; - } phase_counter++; row++; } // In case we encounter another skip row propagated_state = phase_events.back()->state_after; - - if (phase == TransactionPhase::SETUP) { - // Store off the state at the end of setup to rollback to later on revert - end_setup_snapshot = phase_events.back()->state_after; - } - if (phase_events.back()->reverted) { - // On revert, roll back to end-setup-snapshot - // Even though tx-execution events should already do this, - // we need to update propagated state here so that any padded rows - // get the correct rolled-back state rather then the pre-rollback state. - propagated_state.tree_states = end_setup_snapshot.tree_states; - propagated_state.written_public_data_slots_tree_snapshot = - end_setup_snapshot.written_public_data_slots_tree_snapshot; - propagated_state.side_effect_states = end_setup_snapshot.side_effect_states; - // Note: we only rollback tree/side-effect states, not gas used or next_context_id. - } } } diff --git a/barretenberg/cpp/src/barretenberg/vm2/tracegen/tx_trace.test.cpp b/barretenberg/cpp/src/barretenberg/vm2/tracegen/tx_trace.test.cpp index 5d6ff1322dea..c00987578c20 100644 --- a/barretenberg/cpp/src/barretenberg/vm2/tracegen/tx_trace.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm2/tracegen/tx_trace.test.cpp @@ -53,24 +53,10 @@ TEST(TxTraceGenTest, EnqueuedCallEvent) builder.process({ startup_event, tx_event }, trace); auto rows = trace.as_rows(); - ASSERT_EQ(rows.size(), 13); // 12 tx trace rows + 1 precomputed row - EXPECT_THAT(rows[static_cast(TransactionPhase::NR_NOTE_INSERTION)], - AllOf(ROW_FIELD_EQ(tx_sel, 1), - ROW_FIELD_EQ(tx_phase_value, static_cast(TransactionPhase::NR_NOTE_INSERTION)), - ROW_FIELD_EQ(tx_is_padded, 1), - ROW_FIELD_EQ(tx_is_tree_insert_phase, 1), - ROW_FIELD_EQ(tx_sel_non_revertible_append_note_hash, 1), - ROW_FIELD_EQ( - tx_read_pi_length_offset, - AVM_PUBLIC_INPUTS_PREVIOUS_NON_REVERTIBLE_ACCUMULATED_DATA_ARRAY_LENGTHS_NOTE_HASHES_ROW_IDX), - ROW_FIELD_EQ(tx_read_pi_offset, - AVM_PUBLIC_INPUTS_PREVIOUS_NON_REVERTIBLE_ACCUMULATED_DATA_NOTE_HASHES_ROW_IDX), - ROW_FIELD_EQ(tx_start_phase, 1), - ROW_FIELD_EQ(tx_end_phase, 1), - ROW_FIELD_EQ(tx_is_static, false))); + ASSERT_EQ(rows.size(), 2); // 0th precomputed, setup // Setup - EXPECT_THAT(rows[static_cast(TransactionPhase::SETUP)], + EXPECT_THAT(rows[1], AllOf(ROW_FIELD_EQ(tx_sel, 1), ROW_FIELD_EQ(tx_phase_value, static_cast(TransactionPhase::SETUP)), ROW_FIELD_EQ(tx_start_phase, 1), @@ -83,7 +69,8 @@ TEST(TxTraceGenTest, EnqueuedCallEvent) ROW_FIELD_EQ(tx_msg_sender, msg_sender), ROW_FIELD_EQ(tx_contract_addr, contract_address), ROW_FIELD_EQ(tx_calldata_hash, calldata_hash), - ROW_FIELD_EQ(tx_end_phase, 1))); + ROW_FIELD_EQ(tx_end_phase, 1), + ROW_FIELD_EQ(tx_is_static, false))); }; TEST(TxTraceGenTest, CollectFeeEvent) @@ -121,9 +108,9 @@ TEST(TxTraceGenTest, CollectFeeEvent) builder.process({ startup_event, tx_event }, trace); auto rows = trace.as_rows(); - ASSERT_EQ(rows.size(), 13); // 12 tx trace rows + 1 precomputed row + ASSERT_EQ(rows.size(), 2); // 0th precomputed, collect-gas-fees - EXPECT_THAT(rows[static_cast(TransactionPhase::COLLECT_GAS_FEES)], + EXPECT_THAT(rows[1], AllOf(ROW_FIELD_EQ(tx_sel, 1), ROW_FIELD_EQ(tx_phase_value, static_cast(TransactionPhase::COLLECT_GAS_FEES)), ROW_FIELD_EQ(tx_is_padded, 0), @@ -203,9 +190,9 @@ TEST(TxTraceGenTest, PadTreesEvent) builder.process({ startup_event, tx_event }, trace); auto rows = trace.as_rows(); - ASSERT_EQ(rows.size(), 13); // 12 tx trace rows + 1 precomputed row + ASSERT_EQ(rows.size(), 2); // 0th precomputed, tree-padding - EXPECT_THAT(rows[static_cast(TransactionPhase::TREE_PADDING)], + EXPECT_THAT(rows[1], AllOf(ROW_FIELD_EQ(tx_sel, 1), ROW_FIELD_EQ(tx_phase_value, static_cast(TransactionPhase::TREE_PADDING)), ROW_FIELD_EQ(tx_start_phase, 1), @@ -246,10 +233,10 @@ TEST(TxTraceGenTest, CleanupEvent) builder.process({ startup_event, tx_event }, trace); auto rows = trace.as_rows(); - ASSERT_EQ(rows.size(), 13); // 12 tx trace rows + 1 precomputed row + ASSERT_EQ(rows.size(), 2); // 0th precomputed, cleanup EXPECT_THAT( - rows[static_cast(TransactionPhase::CLEANUP)], + rows[1], AllOf(ROW_FIELD_EQ(tx_sel, 1), ROW_FIELD_EQ(tx_phase_value, static_cast(TransactionPhase::CLEANUP)), ROW_FIELD_EQ(tx_start_phase, 1), diff --git a/yarn-project/bb-prover/src/avm_proving_tests/avm_check_circuit2.test.ts b/yarn-project/bb-prover/src/avm_proving_tests/avm_check_circuit2.test.ts index 70a1d274f941..870a8062f938 100644 --- a/yarn-project/bb-prover/src/avm_proving_tests/avm_check_circuit2.test.ts +++ b/yarn-project/bb-prover/src/avm_proving_tests/avm_check_circuit2.test.ts @@ -69,6 +69,34 @@ describe('AVM check-circuit – unhappy paths 2', () => { ); }); + it('error during revertible insertions - skips to teardown', async () => { + await tester.simProveVerify( + sender, + /*setupCalls=*/ [], + /*appCalls=*/ [ + { + address: avmTestContractInstance.address, + fnName: 'add_args_return', + args: [new Fr(1), new Fr(2)], + contractArtifact: AvmTestContractArtifact, + }, + ], + /*teardownCall=*/ { + address: avmTestContractInstance.address, + fnName: 'add_args_return', + args: [new Fr(1), new Fr(2)], + contractArtifact: AvmTestContractArtifact, + }, + /*expectRevert=*/ true, + /*feePayer=*/ sender, + // duplicate nullifiers during revertible insertions! + /*privateInsertions=*/ { + revertible: { nullifiers: [new Fr(100_000), /*duplicate*/ new Fr(100_000)] }, + nonRevertible: { nullifiers: [/*firstNullifier=*/ new Fr(66000)] }, + }, + ); + }); + it( 'enqueued calls in every phase, with enqueued calls that depend on each other', async () => {