Skip to content

Commit

Permalink
fix: fail transaction if we revert in setup or teardown (#5093)
Browse files Browse the repository at this point in the history
public-processor: Drop transactions that revert in non-revertible phases

This change ensures that transactions are dropped if they revert in the
setup or teardown phases, while still including transactions that revert
only in the app logic phase.

Changes:
- Add `checkpoint`, `rollbackToCheckpoint`, `rollbackToCommit` methods to `PublicStateDB`
- Update phase managers to use new `PublicStateDB` methods for checkpointing and rollbacks
- Add assertions to setup and teardown public kernel circuits to fail if the public call is reverted
- Add unit tests for `PublicProcessor` and `WorldStatePublicDB`
- Add E2E tests with bugged `FeePaymentMethod`s to verify behavior

Resolves: #4096
  • Loading branch information
just-mitch authored Mar 14, 2024
1 parent 6977e81 commit db9a960
Show file tree
Hide file tree
Showing 16 changed files with 608 additions and 111 deletions.
1 change: 1 addition & 0 deletions cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"chainsafe",
"cheatcode",
"cheatcodes",
"checkpointed",
"checksummed",
"cimg",
"ciphertext",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ pub fn validate_inputs(public_call: PublicCallData) {
assert(public_call.bytecode_hash != 0, "Bytecode hash cannot be zero");
}

pub fn validate_public_call_non_revert(public_call: PublicCallData) {
assert(public_call.call_stack_item.public_inputs.reverted == false, "Public call cannot be reverted");
}

pub fn initialize_reverted_flag(
previous_kernel: PublicKernelData,
public_call: PublicCallData,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ impl PublicKernelSetupCircuitPrivateInputs {
fn public_kernel_setup(self) -> PublicKernelCircuitPublicInputs {
// construct the circuit outputs
let mut public_inputs: PublicKernelCircuitPublicInputsBuilder = unsafe::zeroed();
// since this phase is non-revertible, we must assert the public call did not revert
common::validate_public_call_non_revert(self.public_call);
common::initialize_reverted_flag(self.previous_kernel, self.public_call, &mut public_inputs);

// initialise the end state with our provided previous kernel state
Expand Down Expand Up @@ -523,4 +525,12 @@ mod tests {
assert_eq(request_context.counter, request_1.counter);
assert_eq(request_context.contract_address, storage_contract_address);
}

#[test(should_fail_with="Public call cannot be reverted")]
fn fails_if_public_call_reverted() {
let mut builder = PublicKernelSetupCircuitPrivateInputsBuilder::new();
builder.public_call.public_inputs.reverted = true;

builder.failed();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ impl PublicKernelTeardownCircuitPrivateInputs {
fn public_kernel_teardown(self) -> PublicKernelCircuitPublicInputs {
// construct the circuit outputs
let mut public_inputs: PublicKernelCircuitPublicInputsBuilder = unsafe::zeroed();
// since this phase is non-revertible, we must assert the public call did not revert
common::validate_public_call_non_revert(self.public_call);
common::initialize_reverted_flag(self.previous_kernel, self.public_call, &mut public_inputs);

// initialise the end state with our provided previous kernel state
Expand Down Expand Up @@ -390,4 +392,12 @@ mod tests {
let expected_unencrypted_logs_hash = compute_logs_hash(prev_unencrypted_logs_hash, unencrypted_logs_hash);
assert_eq(public_inputs.end.unencrypted_logs_hash, expected_unencrypted_logs_hash);
}

#[test(should_fail_with="Public call cannot be reverted")]
fn fails_if_public_call_reverted() {
let mut builder = PublicKernelTeardownCircuitPrivateInputsBuilder::new();
builder.public_call.public_inputs.reverted = true;

builder.failed();
}
}
4 changes: 3 additions & 1 deletion yarn-project/aztec.js/src/contract/sent_tx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ export class SentTx {
}
const receipt = await this.waitForReceipt(opts);
if (receipt.status !== TxStatus.MINED) {
throw new Error(`Transaction ${await this.getTxHash()} was ${receipt.status}`);
throw new Error(
`Transaction ${await this.getTxHash()} was ${receipt.status}. Reason: ${receipt.error ?? 'unknown'}`,
);
}
if (opts?.debug) {
const txHash = await this.getTxHash();
Expand Down
6 changes: 3 additions & 3 deletions yarn-project/aztec.js/src/fee/public_fee_payment_method.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,16 @@ export class PublicFeePaymentMethod implements FeePaymentMethod {
/**
* The asset used to pay the fee.
*/
private asset: AztecAddress,
protected asset: AztecAddress,
/**
* Address which will hold the fee payment.
*/
private paymentContract: AztecAddress,
protected paymentContract: AztecAddress,

/**
* An auth witness provider to authorize fee payments
*/
private wallet: AccountWallet,
protected wallet: AccountWallet,
) {}

/**
Expand Down
189 changes: 189 additions & 0 deletions yarn-project/end-to-end/src/e2e_fees.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,17 @@ import {
DebugLogger,
ExtendedNote,
Fr,
FunctionCall,
FunctionSelector,
Note,
PrivateFeePaymentMethod,
PublicFeePaymentMethod,
TxHash,
Wallet,
computeAuthWitMessageHash,
computeMessageSecretHash,
} from '@aztec/aztec.js';
import { FunctionData } from '@aztec/circuits.js';
import { ContractArtifact, decodeFunctionSignature } from '@aztec/foundation/abi';
import {
TokenContract as BananaCoin,
Expand Down Expand Up @@ -506,6 +509,114 @@ describe('e2e_fees', () => {
});
});

it('fails transaction that error in setup', async () => {
const OutrageousPublicAmountAliceDoesNotHave = 10000n;
// const PublicMintedAlicePublicBananas = 1000n;
const FeeAmount = 1n;
const RefundAmount = 2n;
const MaxFee = FeeAmount + RefundAmount;
const { wallets } = e2eContext;

// simulation throws an error when setup fails
await expect(
bananaCoin.methods
.transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0)
.send({
fee: {
maxFee: MaxFee,
paymentMethod: new BuggedSetupFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]),
},
})
.wait(),
).rejects.toThrow(/Message not authorized by account 'is_valid == true'/);

// so does the sequencer
await expect(
bananaCoin.methods
.transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0)
.send({
skipPublicSimulation: true,
fee: {
maxFee: MaxFee,
paymentMethod: new BuggedSetupFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]),
},
})
.wait(),
).rejects.toThrow(/Transaction [0-9a-f]{64} was dropped\. Reason: Tx dropped by P2P node\./);
});

it('fails transaction that error in teardown', async () => {
/**
* We trigger an error in teardown by having the FPC authorize a transfer of its entire balance to Alice
* as part of app logic. This will cause the FPC to not have enough funds to pay the refund back to Alice.
*/

const PublicMintedAlicePublicBananas = 1000n;
const FeeAmount = 1n;
const RefundAmount = 2n;
const MaxFee = FeeAmount + RefundAmount;
const { wallets } = e2eContext;

const [initialAlicePrivateBananas, initialFPCPrivateBananas] = await bananaPrivateBalances(
aliceAddress,
bananaFPC.address,
);
const [initialAlicePublicBananas, initialFPCPublicBananas] = await bananaPublicBalances(
aliceAddress,
bananaFPC.address,
);
const [initialAliceGas, initialFPCGas, initialSequencerGas] = await gasBalances(
aliceAddress,
bananaFPC.address,
sequencerAddress,
);

await bananaCoin.methods.mint_public(aliceAddress, PublicMintedAlicePublicBananas).send().wait();

await expect(
bananaCoin.methods
.mint_public(aliceAddress, 1n) // random operation
.send({
fee: {
maxFee: MaxFee,
paymentMethod: new BuggedTeardownFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]),
},
})
.wait(),
).rejects.toThrow(/invalid nonce/);

// node also drops
await expect(
bananaCoin.methods
.mint_public(aliceAddress, 1n) // random operation
.send({
skipPublicSimulation: true,
fee: {
maxFee: MaxFee,
paymentMethod: new BuggedTeardownFeePaymentMethod(bananaCoin.address, bananaFPC.address, wallets[0]),
},
})
.wait(),
).rejects.toThrow(/Transaction [0-9a-f]{64} was dropped\. Reason: Tx dropped by P2P node\./);

// nothing happened
await expectMapping(
bananaPrivateBalances,
[aliceAddress, bananaFPC.address, sequencerAddress],
[initialAlicePrivateBananas, initialFPCPrivateBananas, 0n],
);
await expectMapping(
bananaPublicBalances,
[aliceAddress, bananaFPC.address, sequencerAddress],
[initialAlicePublicBananas + PublicMintedAlicePublicBananas, initialFPCPublicBananas, 0n],
);
await expectMapping(
gasBalances,
[aliceAddress, bananaFPC.address, sequencerAddress],
[initialAliceGas, initialFPCGas, initialSequencerGas],
);
});

function logFunctionSignatures(artifact: ContractArtifact, logger: DebugLogger) {
artifact.functions.forEach(fn => {
const sig = decodeFunctionSignature(fn.name, fn.parameters);
Expand Down Expand Up @@ -543,3 +654,81 @@ describe('e2e_fees', () => {
await e2eContext.wallets[accountIndex].addNote(extendedNote);
};
});

class BuggedSetupFeePaymentMethod extends PublicFeePaymentMethod {
getFunctionCalls(maxFee: Fr): Promise<FunctionCall[]> {
const nonce = Fr.random();
const messageHash = computeAuthWitMessageHash(this.paymentContract, {
args: [this.wallet.getAddress(), this.paymentContract, maxFee, nonce],
functionData: new FunctionData(
FunctionSelector.fromSignature('transfer_public((Field),(Field),Field,Field)'),
false,
false,
false,
),
to: this.asset,
});

const tooMuchFee = new Fr(maxFee.toBigInt() * 2n);

return Promise.resolve([
this.wallet.setPublicAuth(messageHash, true).request(),
{
to: this.getPaymentContract(),
functionData: new FunctionData(
FunctionSelector.fromSignature('fee_entrypoint_public(Field,(Field),Field)'),
false,
true,
false,
),
args: [tooMuchFee, this.asset, nonce],
},
]);
}
}

class BuggedTeardownFeePaymentMethod extends PublicFeePaymentMethod {
async getFunctionCalls(maxFee: Fr): Promise<FunctionCall[]> {
// authorize the FPC to take the max fee from Alice
const nonce = Fr.random();
const messageHash1 = computeAuthWitMessageHash(this.paymentContract, {
args: [this.wallet.getAddress(), this.paymentContract, maxFee, nonce],
functionData: new FunctionData(
FunctionSelector.fromSignature('transfer_public((Field),(Field),Field,Field)'),
false,
false,
false,
),
to: this.asset,
});

// authorize the FPC to take the maxFee
// do this first because we only get 2 feepayload calls
await this.wallet.setPublicAuth(messageHash1, true).send().wait();

return Promise.resolve([
// in this, we're actually paying the fee in setup
{
to: this.getPaymentContract(),
functionData: new FunctionData(
FunctionSelector.fromSignature('fee_entrypoint_public(Field,(Field),Field)'),
false,
true,
false,
),
args: [maxFee, this.asset, nonce],
},
// and trying to take a little extra in teardown, but specify a bad nonce
{
to: this.asset,
functionData: new FunctionData(
FunctionSelector.fromSignature('transfer_public((Field),(Field),Field,Field)'),
false,
false,
false,
),
args: [this.wallet.getAddress(), this.paymentContract, new Fr(1), Fr.random()],
},
]);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ import { getVerificationKeys } from '../mocks/verification_keys.js';
import { PublicProver } from '../prover/index.js';
import { PublicKernelCircuitSimulator } from '../simulator/index.js';
import { HintsBuilder } from './hints_builder.js';
import { FailedTx } from './processed_tx.js';
import { lastSideEffectCounter } from './utils.js';

export enum PublicKernelPhase {
Expand Down Expand Up @@ -115,7 +114,6 @@ export abstract class AbstractPhaseManager {
*/
revertReason: SimulationError | undefined;
}>;
abstract rollback(tx: Tx, err: unknown): Promise<FailedTx>;

public static extractEnqueuedPublicCallsByPhase(
publicInputs: PrivateKernelTailCircuitPublicInputs,
Expand Down Expand Up @@ -220,8 +218,18 @@ export abstract class AbstractPhaseManager {

const result = isExecutionRequest ? await simulator(current, this.globalVariables) : current;

newUnencryptedFunctionLogs.push(result.unencryptedLogs);
const functionSelector = result.execution.functionData.selector.toString();
if (result.reverted && !PhaseIsRevertible[this.phase]) {
this.log.debug(
`Simulation error on ${result.execution.contractAddress.toString()}:${functionSelector} with reason: ${
result.revertReason
}`,
);
throw result.revertReason;
}

newUnencryptedFunctionLogs.push(result.unencryptedLogs);

this.log.debug(
`Running public kernel circuit for ${result.execution.contractAddress.toString()}:${functionSelector}`,
);
Expand All @@ -230,14 +238,23 @@ export abstract class AbstractPhaseManager {

[kernelOutput, kernelProof] = await this.runKernelCircuit(kernelOutput, kernelProof, callData);

if (kernelOutput.reverted && this.phase === PublicKernelPhase.APP_LOGIC) {
// sanity check. Note we can't expect them to just be equal, because e.g.
// if the simulator reverts in app logic, it "resets" and result.reverted will be false when we run teardown,
// but the kernel carries the reverted flag forward. But if the simulator reverts, so should the kernel.
if (result.reverted && !kernelOutput.reverted) {
throw new Error(
`Public kernel circuit did not revert on ${result.execution.contractAddress.toString()}:${functionSelector}, but simulator did.`,
);
}

// We know the phase is revertible due to the above check.
// So safely return the revert reason and the kernel output (which has had its revertible side effects dropped)
if (result.reverted) {
this.log.debug(
`Reverting on ${result.execution.contractAddress.toString()}:${functionSelector} with reason: ${
result.revertReason
}`,
);
// halt immediately if the public kernel circuit has reverted.
// return no logs, as they should not go on-chain.
return [kernelOutput, kernelProof, [], result.revertReason];
}

Expand Down
Loading

0 comments on commit db9a960

Please sign in to comment.