Merge pull request #325 from proto-kit/fix/tx-hook-fail-messages

Remove tx hook fail for messages

Author: rpanic

packages/protocol/src/prover/block/BlockProver.ts

@@ -171,7 +171,8 @@ export class BlockProverProgrammable extends ZkProgrammable<
     // Apply beforeTransaction hook state transitions
     const beforeBatch = await this.executeTransactionHooks(
       async (module, args) => await module.beforeTransaction(args),
-      beforeTxHookArguments
+      beforeTxHookArguments,
+      isMessage
     );
 
     state = this.addTransactionToBundle(
@@ -200,7 +201,8 @@ export class BlockProverProgrammable extends ZkProgrammable<
 
     const afterBatch = await this.executeTransactionHooks(
       async (module, args) => await module.afterTransaction(args),
-      afterTxHookArguments
+      afterTxHookArguments,
+      isMessage
     );
     state.pendingSTBatches.push(afterBatch);
 
@@ -266,20 +268,31 @@ export class BlockProverProgrammable extends ZkProgrammable<
     T extends BeforeTransactionHookArguments | AfterTransactionHookArguments,
   >(
     hook: (module: ProvableTransactionHook<unknown>, args: T) => Promise<void>,
-    hookArguments: T
+    hookArguments: T,
+    isMessage: Bool
   ) {
-    const { batch } = await this.executeHooks(hookArguments, async () => {
-      for (const module of this.transactionHooks) {
-        // eslint-disable-next-line no-await-in-loop
-        await hook(module, hookArguments);
-      }
-    });
+    const { batch, rawStatus } = await this.executeHooks(
+      hookArguments,
+      async () => {
+        for (const module of this.transactionHooks) {
+          // eslint-disable-next-line no-await-in-loop
+          await hook(module, hookArguments);
+        }
+      },
+      isMessage
+    );
+
+    // This is going to set applied to false in case the hook fails
+    // (that's only possible for messages though as others are hard-asserted)
+    batch.applied = rawStatus;
+
     return batch;
   }
 
   private async executeHooks<T>(
     contextArguments: RuntimeMethodExecutionData,
-    method: () => Promise<T>
+    method: () => Promise<T>,
+    isMessage: Bool | undefined = undefined
   ) {
     const executionContext = container.resolve(RuntimeMethodExecutionContext);
     executionContext.clear();
@@ -297,11 +310,23 @@ export class BlockProverProgrammable extends ZkProgrammable<
     const { stateTransitions, status, statusMessage } =
       executionContext.current().result;
 
-    status.assertTrue(`Transaction hook call failed: ${statusMessage ?? "-"}`);
+    // See https://github.com/proto-kit/framework/issues/321 for why we do this here
+    if (isMessage !== undefined) {
+      // isMessage is defined for all tx hooks
+      status
+        .or(isMessage)
+        .assertTrue(
+          `Transaction hook call failed for non-message tx: ${statusMessage ?? "-"}`
+        );
+    } else {
+      // isMessage is undefined for all block hooks
+      status.assertTrue(`Block hook call failed: ${statusMessage ?? "-"}`);
+    }
 
     return {
       batch: this.constructBatch(stateTransitions, Bool(true)),
       result,
+      rawStatus: status,
     };
   }
 

packages/protocol/src/state/assert/assert.ts

@@ -19,7 +19,7 @@ export function assert(condition: Bool, message?: string | (() => string)) {
   const status = condition.and(previousStatus);
 
   Provable.asProver(() => {
-    if (!condition.toBoolean()) {
+    if (!condition.toBoolean() && previousStatus.toBoolean()) {
       const messageString =
         message !== undefined && typeof message === "function"
           ? message()

packages/sdk/test/fees-failures.test.ts

@@ -60,6 +60,10 @@ describe("fee errors due to limited funds in sender accounts", () => {
     appChain.setSigner(senderKey);
   });
 
+  afterAll(async () => {
+    await appChain.close();
+  });
+
   it("should allow a free faucet transaction", async () => {
     expect.assertions(2);
 
@@ -105,8 +109,7 @@ describe("fee errors due to limited funds in sender accounts", () => {
     await appChain.produceBlock();
 
     expect(logSpy).toHaveBeenCalledWith(
-      "Error in inclusion of tx, skipping",
-      Error("Protocol hooks not executable: From balance is insufficient")
+      "Error in inclusion of tx, skipping: Protocol hooks not executable: From balance is insufficient"
     );
 
     const balance = await appChain.query.runtime.Balances.balances.get(

packages/sequencer/src/protocol/production/sequencing/BlockProductionService.ts

@@ -15,8 +15,13 @@ import {
 } from "@proto-kit/protocol";
 import { Field } from "o1js";
 import { log } from "@proto-kit/common";
+import { match } from "ts-pattern";
 
-import { Block, BlockWithResult } from "../../../storage/model/Block";
+import {
+  Block,
+  BlockWithResult,
+  TransactionExecutionResult,
+} from "../../../storage/model/Block";
 import { CachedStateService } from "../../../state/state/CachedStateService";
 import { PendingTransaction } from "../../../mempool/PendingTransaction";
 import { AsyncStateService } from "../../../state/async/AsyncStateService";
@@ -27,9 +32,16 @@ import { trace } from "../../../logging/trace";
 import {
   BlockTrackers,
   executeWithExecutionContext,
+  TransactionExecutionResultStatus,
   TransactionExecutionService,
 } from "./TransactionExecutionService";
 
+function isIncludedTxs(
+  x: TransactionExecutionResultStatus
+): x is { status: "included"; result: TransactionExecutionResult } {
+  return x.status === "included";
+}
+
 @injectable()
 @scoped(Lifecycle.ContainerScoped)
 export class BlockProductionService {
@@ -146,8 +158,12 @@ export class BlockProductionService {
       return undefined;
     }
 
+    const includedTransactions = executionResults
+      .filter(isIncludedTxs)
+      .map((x) => x.result);
+
     const block: Omit<Block, "hash"> = {
-      transactions: executionResults.map((x) => x.result),
+      transactions: includedTransactions,
       transactionsHash: newBlockState.transactionList.commitment,
       fromEternalTransactionsHash: lastBlock.toEternalTransactionsHash,
       toEternalTransactionsHash:
@@ -169,10 +185,17 @@ export class BlockProductionService {
 
     const hash = Block.hash(block);
 
-    const includedTxs = executionResults.map((x) => ({
-      hash: x.result.tx.hash().toString(),
-      type: x.status,
-    }));
+    const includedTxs = executionResults.map((x) => {
+      const txHash = match(x)
+        .with({ status: "included" }, ({ result }) => result.tx)
+        .otherwise(({ tx }) => tx)
+        .hash()
+        .toString();
+      return {
+        hash: txHash,
+        type: x.status,
+      };
+    });
 
     return {
       block: {

packages/sequencer/src/protocol/production/sequencing/TransactionExecutionService.ts

@@ -197,6 +197,14 @@ function traceLogSTs(msg: string, stateTransitions: StateTransition<any>[]) {
   );
 }
 
+export type TransactionExecutionResultStatus =
+  | {
+      result: TransactionExecutionResult;
+      status: "included";
+    }
+  | { tx: PendingTransaction; status: "skipped" }
+  | { tx: PendingTransaction; status: "shouldRemove" };
+
 @injectable()
 @scoped(Lifecycle.ContainerScoped)
 export class TransactionExecutionService {
@@ -276,15 +284,6 @@ export class TransactionExecutionService {
       runSimulated
     );
 
-    // if (!result.status.toBoolean()) {
-    //   const error = new Error(
-    //     `Protocol hooks not executable: ${result.statusMessage ?? "unknown"}`
-    //   );
-    //   log.debug("Protocol hook error stack trace:", result.stackTrace);
-    //   // Propagate stack trace from the assertion
-    //   throw error;
-    // }
-
     traceLogSTs(`${hookName} STs:`, result.stateTransitions);
 
     return result;
@@ -328,23 +327,18 @@ export class TransactionExecutionService {
     );
   }
 
+  // eslint-disable-next-line sonarjs/cognitive-complexity
   public async createExecutionTraces(
     asyncStateService: CachedStateService,
     transactions: PendingTransaction[],
     networkState: NetworkState,
     state: BlockTrackers
   ): Promise<{
     blockState: BlockTrackers;
-    executionResults: {
-      result: TransactionExecutionResult;
-      status: "included" | "skipped" | "shouldRemove";
-    }[];
+    executionResults: TransactionExecutionResultStatus[];
   }> {
     let blockState = state;
-    const executionResults: {
-      result: TransactionExecutionResult;
-      status: "included" | "skipped" | "shouldRemove";
-    }[] = [];
+    const executionResults: TransactionExecutionResultStatus[] = [];
 
     const networkStateHash = networkState.hash();
 
@@ -369,8 +363,14 @@ export class TransactionExecutionService {
           !executionTrace.hooksStatus.toBoolean() &&
           !executionTrace.tx.isMessage
         ) {
+          const actionMessage = shouldRemove
+            ? "removing as to removeWhen hooks"
+            : "skipping";
+          log.error(
+            `Error in inclusion of tx, ${actionMessage}: Protocol hooks not executable: ${executionTrace.statusMessage ?? "unknown reason"}`
+          );
           executionResults.push({
-            result: executionTrace,
+            tx,
             status: shouldRemove ? "shouldRemove" : "skipped",
           });
         } else {
@@ -381,7 +381,8 @@ export class TransactionExecutionService {
         }
       } catch (error) {
         if (error instanceof Error) {
-          log.error("Error in inclusion of tx, skipping", error);
+          log.error("Error in inclusion of tx, dropping", error);
+          executionResults.push({ tx, status: "shouldRemove" });
         }
       }
     }
@@ -545,7 +546,10 @@ export class TransactionExecutionService {
         tx,
         hooksStatus: Bool(txHooksValid),
         status: runtimeResult.status,
-        statusMessage: runtimeResult.statusMessage,
+        statusMessage:
+          beforeTxHookResult.statusMessage ??
+          afterTxHookResult.statusMessage ??
+          runtimeResult.statusMessage,
 
         stateTransitions,
         events: beforeHookEvents.concat(runtimeResultEvents, afterHookEvents),

packages/sequencer/test/integration/MempoolTxRemoved.test.ts

@@ -33,7 +33,7 @@ describe("block production", () => {
 
     // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
     mempool = sequencer.resolve("Mempool");
-  });
+  }, 60_000);
 
   it("check tx is removed", async () => {
     await mempool.add(