diff --git a/packages/protocol/src/prover/block/BlockProver.ts b/packages/protocol/src/prover/block/BlockProver.ts index 6a7e89cb3..0e7b2a362 100644 --- a/packages/protocol/src/prover/block/BlockProver.ts +++ b/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, args: T) => Promise, - 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( contextArguments: RuntimeMethodExecutionData, - method: () => Promise + method: () => Promise, + 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, }; } diff --git a/packages/protocol/src/state/assert/assert.ts b/packages/protocol/src/state/assert/assert.ts index f15b554d3..1af3e494f 100644 --- a/packages/protocol/src/state/assert/assert.ts +++ b/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() diff --git a/packages/sdk/test/fees-failures.test.ts b/packages/sdk/test/fees-failures.test.ts index 1e3d46b3d..a6df758d8 100644 --- a/packages/sdk/test/fees-failures.test.ts +++ b/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( diff --git a/packages/sequencer/src/protocol/production/sequencing/BlockProductionService.ts b/packages/sequencer/src/protocol/production/sequencing/BlockProductionService.ts index ddafd630b..18010fa28 100644 --- a/packages/sequencer/src/protocol/production/sequencing/BlockProductionService.ts +++ b/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 = { - 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: { diff --git a/packages/sequencer/src/protocol/production/sequencing/TransactionExecutionService.ts b/packages/sequencer/src/protocol/production/sequencing/TransactionExecutionService.ts index 792a6a6f7..753cfbb8f 100644 --- a/packages/sequencer/src/protocol/production/sequencing/TransactionExecutionService.ts +++ b/packages/sequencer/src/protocol/production/sequencing/TransactionExecutionService.ts @@ -197,6 +197,14 @@ function traceLogSTs(msg: string, stateTransitions: StateTransition[]) { ); } +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,6 +327,7 @@ export class TransactionExecutionService { ); } + // eslint-disable-next-line sonarjs/cognitive-complexity public async createExecutionTraces( asyncStateService: CachedStateService, transactions: PendingTransaction[], @@ -335,16 +335,10 @@ export class TransactionExecutionService { 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), diff --git a/packages/sequencer/test/integration/MempoolTxRemoved.test.ts b/packages/sequencer/test/integration/MempoolTxRemoved.test.ts index c13c423f9..14d14bec9 100644 --- a/packages/sequencer/test/integration/MempoolTxRemoved.test.ts +++ b/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(