fix: fail transaction if we revert in setup or teardown

[just-mitch]

Introduction

The work done in #4971 made it so that if a transaction reverts while executing the "app logic" phase of a transaction, we no longer drop the transaction and instead include it but drop all the revertible side effects.

This work ensures that we do in fact drop the transaction if it gives an error in any non-revertible phases, as this would render the transaction invalid.

For more context, see #4096.

Design

We need sequencer and public kernel circuit changes to support this.

Updates to the public processor and phase managers

At present, we publicStateDB.commit() at the end of each phase. This is a problem because if we revert in the teardown phase, we will have already committed the setup phase.

But we need to somehow commit the setup phase, because if we revert in the app logic phase, we need to keep the setup phase.

Broadly, we need to be able to "soft commit" to changes that occur during a phase.

I will call that "soft commit" a "checkpoint".

We then need the ability to rollback either to the last checkpoint, or to the last commit.

To do this, we can modify the PublicStateDB interface:

/**
 * Mark the uncommitted changes in this TX as a checkpoint.
 */
checkpoint(): Promise<void>

/**
 * Rollback to the last checkpoint.
 */
rollbackToCheckpoint(): Promise<void>

/**
 * Commit the changes in this TX. Includes all changes since the last commit,
 * even if they haven't been covered by a checkpoint.
 */
commit(): Promise<void>

/**
 * Rollback to the last commit.
 */
rollbackToCommit(): Promise<void>

Under the hood in WorldStatePublicDB, all updates will still initially go into the uncommitedWriteCache, but we'll just add a new checkpointedWriteCache. Only downside is that storageRead will need to check both caches, but they should stay small, so it should be fine.

In the setup_phase_manager and teardown_phase_manager we can do:

const [
  publicKernelOutput,
  publicKernelProof,
  newUnencryptedFunctionLogs,
  revertReason,
] = await this.processEnqueuedPublicCalls(
  tx,
  previousPublicKernelOutput,
  previousPublicKernelProof
).catch(
  // the abstract phase manager throws if simulation gives error in a non-revertible phase
  async (err) => {
    await this.publicStateDB.rollbackToCommit();
    throw err;
  }
);
tx.unencryptedLogs.addFunctionLogs(newUnencryptedFunctionLogs);
await this.publicStateDB.checkpoint();
return { publicKernelOutput, publicKernelProof, revertReason };

In app_logic_phase_manager, we can do:

const [
  publicKernelOutput,
  publicKernelProof,
  newUnencryptedFunctionLogs,
  revertReason,
] = await this.processEnqueuedPublicCalls(
  tx,
  previousPublicKernelOutput,
  previousPublicKernelProof
).catch(
  // if we throw for any reason other than simulation, we need to rollback and drop the TX
  async (err) => {
    await this.publicStateDB.rollbackToCommit();
    throw err;
  }
);

if (revertReason) {
  await this.publicContractsDB.removeNewContracts(tx);
  await this.publicStateDB.rollbackToCheckpoint();
} else {
  tx.unencryptedLogs.addFunctionLogs(newUnencryptedFunctionLogs);
  await this.publicStateDB.checkpoint();
}
return { publicKernelOutput, publicKernelProof, revertReason };

And finally in tail_phase_manager:

const [publicKernelOutput, publicKernelProof] = await this.runKernelCircuit(
  previousPublicKernelOutput,
  previousPublicKernelProof
).catch(
  // the abstract phase manager throws if simulation gives error in non-revertible phase
  async (err) => {
    await this.publicStateDB.rollbackToCommit();
    throw err;
  }
);

// commit the state updates from this transaction
await this.publicStateDB.commit();

return { publicKernelOutput, publicKernelProof, revertReason: undefined };

Circuit changes

As a sanity check, we need to verify in the public kernel setup and teardown that the public call did not revert.

Test Plan

Unit Tests

Add to public_processor.test.ts: show that if a transaction reverts in the setup or teardown phase, it is "Failed" and not "Reverted".

Add to world_state_public_db.test.ts: show that the rollbackToCheckpoint and rollbackToCommit methods work as expected.

Add to setup and teardown public kernel circuits to show that they fail assertions if the public call is reverted.

E2E Tests

In e2e_fees.test.ts, we can add bugged FeePaymentMethods that:

• take too much of a fee in setup
• distribute too much of a fee in teardown

Especially Relevant Parties

• @alexghr
• @PhilWindle
• @LeilaWang

Plan Approvals

Please add a +1 or comment to express your approval or disapproval of the plan above.

[AztecBot]

Benchmark results

No metrics with a significant change found.

Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit cfd673d6 and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric	8 txs	32 txs	64 txs
l1_rollup_calldata_size_in_bytes	5,668	18,820	36,356
l1_rollup_calldata_gas	66,376	239,104	468,992
l1_rollup_execution_gas	667,057	954,014	1,336,349
l2_block_processing_time_in_ms	1,264 (-3%)	4,409 (-6%)	9,262 (+4%)
note_successful_decrypting_time_in_ms	198	504 (+5%)	977 (+4%)
note_trial_decrypting_time_in_ms	104 (-1%)	123 (+23%)	106 (+44%)
l2_block_building_time_in_ms	18,095 (-2%)	68,318 (-2%)	136,401 (-2%)
l2_block_rollup_simulation_time_in_ms	8,121 (-2%)	28,660 (-2%)	56,898
l2_block_public_tx_process_time_in_ms	9,951 (-3%)	39,602 (-3%)	79,404 (-2%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric	5 blocks	10 blocks
node_history_sync_time_in_ms	14,185 (+2%)	26,591 (-11%)
note_history_successful_decrypting_time_in_ms	1,305 (+6%)	2,388 (-6%)
note_history_trial_decrypting_time_in_ms	104 (+12%)	116 (-43%)
node_database_size_in_bytes	19,075,152	35,717,200
pxe_database_size_in_bytes	29,859	59,414

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit	circuit_simulation_time_in_ms	circuit_input_size_in_bytes	circuit_output_size_in_bytes
private-kernel-init	283 (-2%)	44,370	27,700
private-kernel-ordering	217 (-1%)	52,324	14,326
base-parity	1,840 (-4%)	128	311
base-rollup	722 (-1%)	177,083	925
root-parity	1,485 (+2%)	1,244	311
root-rollup	68.4	4,487	789
private-kernel-inner	643 (-1%)	73,229	27,700
public-kernel-app-logic	451 (-2%)	35,262	28,215
public-kernel-tail	177 (-3%)	40,926	28,215
merge-rollup	8.85 (+6%)	2,696	925

Tree insertion stats

The duration to insert a fixed batch of leaves into each tree type.

Metric	1 leaves	16 leaves	64 leaves	128 leaves	512 leaves	1024 leaves	2048 leaves	4096 leaves	32 leaves
batch_insert_into_append_only_tree_16_depth_ms	9.86 (-1%)	15.8 (-2%)	N/A	N/A	N/A	N/A	N/A	N/A	N/A
batch_insert_into_append_only_tree_16_depth_hash_count	16.8	31.6	N/A	N/A	N/A	N/A	N/A	N/A	N/A
batch_insert_into_append_only_tree_16_depth_hash_ms	0.577 (-1%)	0.490 (-1%)	N/A	N/A	N/A	N/A	N/A	N/A	N/A
batch_insert_into_append_only_tree_32_depth_ms	N/A	N/A	45.5	71.3 (-1%)	228	444	853 (-1%)	1,707 (-1%)	N/A
batch_insert_into_append_only_tree_32_depth_hash_count	N/A	N/A	96.0	159	543	1,055	2,079	4,127	N/A
batch_insert_into_append_only_tree_32_depth_hash_ms	N/A	N/A	0.468	0.441 (-1%)	0.417	0.416	0.407 (-1%)	0.409 (-1%)	N/A
batch_insert_into_indexed_tree_20_depth_ms	N/A	N/A	53.1 (-1%)	105 (-1%)	327 (-2%)	656 (-1%)	1,286 (-2%)	2,600	N/A
batch_insert_into_indexed_tree_20_depth_hash_count	N/A	N/A	104	207	691	1,363	2,707	5,395	N/A
batch_insert_into_indexed_tree_20_depth_hash_ms	N/A	N/A	0.474 (-1%)	0.476 (-1%)	0.446 (-2%)	0.453	0.447 (-1%)	0.452	N/A
batch_insert_into_indexed_tree_40_depth_ms	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	61.2
batch_insert_into_indexed_tree_40_depth_hash_count	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	109
batch_insert_into_indexed_tree_40_depth_hash_ms	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	0.537 (+1%)

Miscellaneous

Transaction sizes based on how many contract classes are registered in the tx.

Metric	0 registered classes
tx_size_in_bytes	22,014

Transaction processing duration by data writes.

Metric	0 new note hashes	1 new note hashes
tx_pxe_processing_time_ms	4,015 (-1%)	1,743 (-1%)

Metric	0 public data writes	1 public data writes
tx_sequencer_processing_time_ms	12.9 (-14%)	1,257 (-2%)

[github-actions]

Changes to circuit sizes

Generated at commit: 6f073d8eec72a0c76110843d1e2aa48933421856, compared to commit: 19eb7f9bd48f1f5fb8d9e9a2e172c8f0c2c9445b

🧾 Summary (100% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
public_kernel_teardown	-2 ✅	-0.01%	-2 ✅	-0.00%
public_kernel_setup	-2 ✅	-0.01%	-2 ✅	-0.00%

---

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
public_kernel_teardown	29,355 (-2)	-0.01%	182,046 (-2)	-0.00%
public_kernel_setup	27,609 (-2)	-0.01%	180,299 (-2)	-0.00%

[just-mitch]

Built to spec. Only discrepancy is that in the e2e test for reverts in teardown, instead of distributing too much, I instead just stub a failed transfer (due to non-zero nonce) as the distribution function.

[PhilWindle]

Is this comment correct? Aren't we using a bad nonce instead?

[just-mitch]

It is correct: we're using a bad nonce in teardown! In BuggedTeardownFeePaymentMethod at the bottom of this test, it specifies a transfer as the teardown function, and it is in that transfer that we use the bad nonce.

[alexghr]

It looks good but I just had one question related to the checkpoint/commit/rollback mechanic. Might be worth adding a unit test for a tx with two functions enqueued for app logic.

[alexghr]

Could call simulate directly to make it match the comment above.

Suggested change

	.send({
	.simulate({

[just-mitch]

My thinking was that it was simulating the public part since skipPublicSimulation is not set.

[alexghr]

I think a different failure scenario that doesn't involve frontrunning the tx with a nother tx would be for the bugged payment method to transfer MaxFee/2 tokens from Alice causing the FPC to run out of public funds and not be able to process the refund (similar to the bugged setup above that tries to take MaxFee*2)

[LeilaWang]

I didn't check the e2e test thoroughly. But other part of the changes look good to me!

Concerns were resolved, confirmed via slack.