From 1af1c4c820bde2fdfe32a2eea55cb215e5254c00 Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Tue, 18 Jun 2024 05:35:33 -0400 Subject: [PATCH 1/2] do not discard logs on revert since the kernel has pruned revertible logs. add test of private fee payment method with revert. --- .../circuit-types/src/logs/tx_l2_logs.ts | 21 ++-- .../circuit-types/src/tx/processed_tx.ts | 6 +- .../end-to-end/src/e2e_fees/failures.test.ts | 99 +++++++++++++++++++ 3 files changed, 114 insertions(+), 12 deletions(-) diff --git a/yarn-project/circuit-types/src/logs/tx_l2_logs.ts b/yarn-project/circuit-types/src/logs/tx_l2_logs.ts index f17004efa7c..eca90bfbdb2 100644 --- a/yarn-project/circuit-types/src/logs/tx_l2_logs.ts +++ b/yarn-project/circuit-types/src/logs/tx_l2_logs.ts @@ -157,16 +157,17 @@ export class UnencryptedTxL2Logs extends TxL2Logs { * for more details. */ public hash(): Buffer { - if (this.unrollLogs().length == 0) { + const unrolledLogs = this.unrollLogs(); + if (unrolledLogs.length == 0) { return Buffer.alloc(32); } let flattenedLogs = Buffer.alloc(0); - for (const logsFromSingleFunctionCall of this.unrollLogs()) { + for (const logsFromSingleFunctionCall of unrolledLogs) { flattenedLogs = Buffer.concat([flattenedLogs, logsFromSingleFunctionCall.getSiloedHash()]); } // pad the end of logs with 0s - for (let i = 0; i < MAX_UNENCRYPTED_LOGS_PER_TX - this.unrollLogs().length; i++) { + for (let i = 0; i < MAX_UNENCRYPTED_LOGS_PER_TX - unrolledLogs.length; i++) { flattenedLogs = Buffer.concat([flattenedLogs, Buffer.alloc(32)]); } @@ -235,16 +236,17 @@ export class EncryptedNoteTxL2Logs extends TxL2Logs { * for more details. */ public hash(): Buffer { - if (this.unrollLogs().length == 0) { + const unrolledLogs = this.unrollLogs(); + if (unrolledLogs.length == 0) { return Buffer.alloc(32); } let flattenedLogs = Buffer.alloc(0); - for (const logsFromSingleFunctionCall of this.unrollLogs()) { + for (const logsFromSingleFunctionCall of unrolledLogs) { flattenedLogs = Buffer.concat([flattenedLogs, logsFromSingleFunctionCall.hash()]); } // pad the end of logs with 0s - for (let i = 0; i < MAX_NOTE_ENCRYPTED_LOGS_PER_TX - this.unrollLogs().length; i++) { + for (let i = 0; i < MAX_NOTE_ENCRYPTED_LOGS_PER_TX - unrolledLogs.length; i++) { flattenedLogs = Buffer.concat([flattenedLogs, Buffer.alloc(32)]); } @@ -313,16 +315,17 @@ export class EncryptedTxL2Logs extends TxL2Logs { * for more details. */ public hash(): Buffer { - if (this.unrollLogs().length == 0) { + const unrolledLogs = this.unrollLogs(); + if (unrolledLogs.length == 0) { return Buffer.alloc(32); } let flattenedLogs = Buffer.alloc(0); - for (const logsFromSingleFunctionCall of this.unrollLogs()) { + for (const logsFromSingleFunctionCall of unrolledLogs) { flattenedLogs = Buffer.concat([flattenedLogs, logsFromSingleFunctionCall.getSiloedHash()]); } // pad the end of logs with 0s - for (let i = 0; i < MAX_ENCRYPTED_LOGS_PER_TX - this.unrollLogs().length; i++) { + for (let i = 0; i < MAX_ENCRYPTED_LOGS_PER_TX - unrolledLogs.length; i++) { flattenedLogs = Buffer.concat([flattenedLogs, Buffer.alloc(32)]); } diff --git a/yarn-project/circuit-types/src/tx/processed_tx.ts b/yarn-project/circuit-types/src/tx/processed_tx.ts index cf6bb977423..5147b71c2ce 100644 --- a/yarn-project/circuit-types/src/tx/processed_tx.ts +++ b/yarn-project/circuit-types/src/tx/processed_tx.ts @@ -160,9 +160,9 @@ export function makeProcessedTx( data: kernelOutput, proof, // TODO(4712): deal with non-revertible logs here - noteEncryptedLogs: revertReason ? EncryptedNoteTxL2Logs.empty() : tx.noteEncryptedLogs, - encryptedLogs: revertReason ? EncryptedTxL2Logs.empty() : tx.encryptedLogs, - unencryptedLogs: revertReason ? UnencryptedTxL2Logs.empty() : tx.unencryptedLogs, + noteEncryptedLogs: tx.noteEncryptedLogs, + encryptedLogs: tx.encryptedLogs, + unencryptedLogs: tx.unencryptedLogs, isEmpty: false, revertReason, publicProvingRequests, diff --git a/yarn-project/end-to-end/src/e2e_fees/failures.test.ts b/yarn-project/end-to-end/src/e2e_fees/failures.test.ts index ceeee8d7655..733a8577425 100644 --- a/yarn-project/end-to-end/src/e2e_fees/failures.test.ts +++ b/yarn-project/end-to-end/src/e2e_fees/failures.test.ts @@ -4,8 +4,10 @@ import { Fr, type FunctionCall, FunctionSelector, + PrivateFeePaymentMethod, PublicFeePaymentMethod, TxStatus, + computeSecretHash, } from '@aztec/aztec.js'; import { Gas, GasSettings } from '@aztec/circuits.js'; import { FunctionType } from '@aztec/foundation/abi'; @@ -119,6 +121,103 @@ describe('e2e_fees failures', () => { // Can't do presently because all logs are "revertible" so we lose notes that get broadcasted during unshielding. }); + it('reverts transactions but still pays fees using PrivateFeePaymentMethod', async () => { + const OutrageousPublicAmountAliceDoesNotHave = BigInt(1e8); + const PrivateMintedAlicePrivateBananas = BigInt(1e15); + + const [initialAlicePrivateBananas, initialFPCPrivateBananas] = await t.bananaPrivateBalances( + aliceAddress, + bananaFPC.address, + ); + const [initialAlicePublicBananas, initialFPCPublicBananas] = await t.bananaPublicBalances( + aliceAddress, + bananaFPC.address, + ); + const [initialAliceGas, initialFPCGas, initialSequencerGas] = await t.gasBalances( + aliceAddress, + bananaFPC.address, + sequencerAddress, + ); + + await t.mintPrivateBananas(PrivateMintedAlicePrivateBananas, aliceAddress); + + // if we simulate locally, it throws an error + await expect( + bananaCoin.methods + // still use a public transfer so as to fail in the public app logic phase + .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0) + .send({ + fee: { + gasSettings, + paymentMethod: new PrivateFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet), + }, + }) + .wait(), + ).rejects.toThrow(/attempt to subtract with underflow 'hi == high'/); + + // we did not pay the fee, because we did not submit the TX + await expectMapping( + t.bananaPrivateBalances, + [aliceAddress, bananaFPC.address], + [initialAlicePrivateBananas + PrivateMintedAlicePrivateBananas, initialFPCPrivateBananas], + ); + await expectMapping( + t.bananaPublicBalances, + [aliceAddress, bananaFPC.address], + [initialAlicePublicBananas, initialFPCPublicBananas], + ); + await expectMapping(t.gasBalances, [aliceAddress, bananaFPC.address], [initialAliceGas, initialFPCGas]); + + // if we skip simulation, it includes the failed TX + const rebateSecret = Fr.random(); + const currentSequencerL1Gas = await t.getCoinbaseBalance(); + const txReceipt = await bananaCoin.methods + .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0) + .send({ + skipPublicSimulation: true, + fee: { + gasSettings, + paymentMethod: new PrivateFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet, rebateSecret), + }, + }) + .wait({ dontThrowOnRevert: true }); + + expect(txReceipt.status).toBe(TxStatus.APP_LOGIC_REVERTED); + const feeAmount = txReceipt.transactionFee!; + const newSequencerL1Gas = await t.getCoinbaseBalance(); + expect(newSequencerL1Gas).toEqual(currentSequencerL1Gas + feeAmount); + + // and thus we paid the fee + await expectMapping( + t.bananaPrivateBalances, + [aliceAddress, bananaFPC.address], + [ + // alice paid the maximum amount in private bananas + initialAlicePrivateBananas + PrivateMintedAlicePrivateBananas - gasSettings.getFeeLimit().toBigInt(), + initialFPCPrivateBananas, + ], + ); + await expectMapping( + t.bananaPublicBalances, + [aliceAddress, bananaFPC.address], + [initialAlicePublicBananas, initialFPCPublicBananas + feeAmount], + ); + await expectMapping(t.gasBalances, [aliceAddress, bananaFPC.address], [initialAliceGas, initialFPCGas - feeAmount]); + + // Alice can redeem her shield to get the rebate + const refund = gasSettings.getFeeLimit().toBigInt() - feeAmount; + expect(refund).toBeGreaterThan(0n); + const secretHashForRebate = computeSecretHash(rebateSecret); + await t.addPendingShieldNoteToPXE(t.aliceWallet, refund, secretHashForRebate, txReceipt.txHash); + await bananaCoin.methods.redeem_shield(aliceAddress, refund, rebateSecret).send().wait(); + + await expectMapping( + t.bananaPrivateBalances, + [aliceAddress, bananaFPC.address], + [initialAlicePrivateBananas + PrivateMintedAlicePrivateBananas - feeAmount, initialFPCPrivateBananas], + ); + }); + it('fails transaction that error in setup', async () => { const OutrageousPublicAmountAliceDoesNotHave = BigInt(100e12); From 3be3879190ecf66b09623202f29e47f67ad31578 Mon Sep 17 00:00:00 2001 From: Mitchell Tracy Date: Thu, 20 Jun 2024 15:59:37 -0400 Subject: [PATCH 2/2] filter out reverted logs from tx. fix mock tx setup. --- .../src/logs/encrypted_l2_note_log.ts | 4 + .../src/logs/function_l2_logs.ts | 3 +- .../circuit-types/src/logs/tx_l2_logs.ts | 32 ++++- yarn-project/circuit-types/src/mocks.ts | 60 ++++++--- yarn-project/circuit-types/src/tx/tx.ts | 38 +++++- .../end-to-end/src/e2e_fees/failures.test.ts | 127 +++++++++--------- .../src/public/app_logic_phase_manager.ts | 1 + .../src/public/public_processor.test.ts | 28 +++- .../src/public/teardown_phase_manager.ts | 1 + 9 files changed, 195 insertions(+), 99 deletions(-) diff --git a/yarn-project/circuit-types/src/logs/encrypted_l2_note_log.ts b/yarn-project/circuit-types/src/logs/encrypted_l2_note_log.ts index fea25ec838d..a84039e3829 100644 --- a/yarn-project/circuit-types/src/logs/encrypted_l2_note_log.ts +++ b/yarn-project/circuit-types/src/logs/encrypted_l2_note_log.ts @@ -52,6 +52,10 @@ export class EncryptedL2NoteLog { return sha256Trunc(preimage); } + public getSiloedHash(): Buffer { + return this.hash(); + } + /** * Crates a random log. * @returns A random log. diff --git a/yarn-project/circuit-types/src/logs/function_l2_logs.ts b/yarn-project/circuit-types/src/logs/function_l2_logs.ts index a176af8bf00..e45fcbef4e9 100644 --- a/yarn-project/circuit-types/src/logs/function_l2_logs.ts +++ b/yarn-project/circuit-types/src/logs/function_l2_logs.ts @@ -37,9 +37,8 @@ export abstract class FunctionL2Logs acc + log.length + 4, 0) + 4; + return this.getKernelLength() + 4; } /** diff --git a/yarn-project/circuit-types/src/logs/tx_l2_logs.ts b/yarn-project/circuit-types/src/logs/tx_l2_logs.ts index eca90bfbdb2..90610fa5331 100644 --- a/yarn-project/circuit-types/src/logs/tx_l2_logs.ts +++ b/yarn-project/circuit-types/src/logs/tx_l2_logs.ts @@ -1,4 +1,6 @@ import { + Fr, + type LogHash, MAX_ENCRYPTED_LOGS_PER_TX, MAX_NOTE_ENCRYPTED_LOGS_PER_TX, MAX_UNENCRYPTED_LOGS_PER_TX, @@ -22,6 +24,8 @@ import { type UnencryptedL2Log } from './unencrypted_l2_log.js'; * Data container of logs emitted in 1 tx. */ export abstract class TxL2Logs { + abstract hash(): Buffer; + constructor( /** * An array containing logs emitted in individual function invocations in this tx. */ public readonly functionLogs: FunctionL2Logs[], @@ -94,6 +98,28 @@ export abstract class TxL2Logs): boolean { return isEqual(this, other); } + + /** + * Filter the logs from functions from this TxL2Logs that + * appear in the provided logHashes + * @param logHashes hashes we want to keep + * @param output our aggregation + * @returns our aggregation + */ + public filter(logHashes: LogHash[], output: TxL2Logs): TxL2Logs { + for (const fnLogs of this.functionLogs) { + let include = false; + for (const log of fnLogs.logs) { + if (logHashes.findIndex(lh => lh.value.equals(Fr.fromBuffer(log.getSiloedHash()))) !== -1) { + include = true; + } + } + if (include) { + output.addFunctionLogs([fnLogs]); + } + } + return output; + } } export class UnencryptedTxL2Logs extends TxL2Logs { @@ -156,7 +182,7 @@ export class UnencryptedTxL2Logs extends TxL2Logs { * Note: This is a TS implementation of `computeKernelUnencryptedLogsHash` function in Decoder.sol. See that function documentation * for more details. */ - public hash(): Buffer { + public override hash(): Buffer { const unrolledLogs = this.unrollLogs(); if (unrolledLogs.length == 0) { return Buffer.alloc(32); @@ -235,7 +261,7 @@ export class EncryptedNoteTxL2Logs extends TxL2Logs { * Note: This is a TS implementation of `computeKernelNoteEncryptedLogsHash` function in Decoder.sol. See that function documentation * for more details. */ - public hash(): Buffer { + public override hash(): Buffer { const unrolledLogs = this.unrollLogs(); if (unrolledLogs.length == 0) { return Buffer.alloc(32); @@ -314,7 +340,7 @@ export class EncryptedTxL2Logs extends TxL2Logs { * Note: This is a TS implementation of `computeKernelEncryptedLogsHash` function in Decoder.sol. See that function documentation * for more details. */ - public hash(): Buffer { + public override hash(): Buffer { const unrolledLogs = this.unrollLogs(); if (unrolledLogs.length == 0) { return Buffer.alloc(32); diff --git a/yarn-project/circuit-types/src/mocks.ts b/yarn-project/circuit-types/src/mocks.ts index 1c4989c97b0..2ffb92850c3 100644 --- a/yarn-project/circuit-types/src/mocks.ts +++ b/yarn-project/circuit-types/src/mocks.ts @@ -107,24 +107,50 @@ export const mockTx = ( if (hasLogs) { let i = 1; // 0 used in first nullifier - encryptedLogs.functionLogs.forEach((log, j) => { - // ts complains if we dont check .forPublic here, even though it is defined ^ - if (data.forPublic) { - data.forPublic.end.encryptedLogsHashes[j] = new LogHash( - Fr.fromBuffer(log.hash()), - i++, - new Fr(log.toBuffer().length), - ); - } + let nonRevertibleIndex = 0; + let revertibleIndex = 0; + let functionCount = 0; + encryptedLogs.functionLogs.forEach(functionLog => { + functionLog.logs.forEach(log => { + // ts complains if we dont check .forPublic here, even though it is defined ^ + if (data.forPublic) { + const hash = new LogHash( + Fr.fromBuffer(log.getSiloedHash()), + i++, + // +4 for encoding the length of the buffer + new Fr(log.length + 4), + ); + // make the first log non-revertible + if (functionCount === 0) { + data.forPublic.endNonRevertibleData.encryptedLogsHashes[nonRevertibleIndex++] = hash; + } else { + data.forPublic.end.encryptedLogsHashes[revertibleIndex++] = hash; + } + } + }); + functionCount++; }); - unencryptedLogs.functionLogs.forEach((log, j) => { - if (data.forPublic) { - data.forPublic.end.unencryptedLogsHashes[j] = new LogHash( - Fr.fromBuffer(log.hash()), - i++, - new Fr(log.toBuffer().length), - ); - } + nonRevertibleIndex = 0; + revertibleIndex = 0; + functionCount = 0; + unencryptedLogs.functionLogs.forEach(functionLog => { + functionLog.logs.forEach(log => { + if (data.forPublic) { + const hash = new LogHash( + Fr.fromBuffer(log.getSiloedHash()), + i++, + // +4 for encoding the length of the buffer + new Fr(log.length + 4), + ); + // make the first log non-revertible + if (functionCount === 0) { + data.forPublic.endNonRevertibleData.unencryptedLogsHashes[nonRevertibleIndex++] = hash; + } else { + data.forPublic.end.unencryptedLogsHashes[revertibleIndex++] = hash; + } + } + }); + functionCount++; }); } } else { diff --git a/yarn-project/circuit-types/src/tx/tx.ts b/yarn-project/circuit-types/src/tx/tx.ts index d10e8fdb5f6..8cb40f57d62 100644 --- a/yarn-project/circuit-types/src/tx/tx.ts +++ b/yarn-project/circuit-types/src/tx/tx.ts @@ -3,6 +3,7 @@ import { PrivateKernelTailCircuitPublicInputs, Proof, PublicCallRequest, + type PublicKernelCircuitPublicInputs, } from '@aztec/circuits.js'; import { arraySerializedSizeOfNonEmpty } from '@aztec/foundation/collection'; import { BufferReader, serializeToBuffer } from '@aztec/foundation/serialize'; @@ -29,15 +30,15 @@ export class Tx { /** * Encrypted note logs generated by the tx. */ - public readonly noteEncryptedLogs: EncryptedNoteTxL2Logs, + public noteEncryptedLogs: EncryptedNoteTxL2Logs, /** * Encrypted logs generated by the tx. */ - public readonly encryptedLogs: EncryptedTxL2Logs, + public encryptedLogs: EncryptedTxL2Logs, /** * Unencrypted logs generated by the tx. */ - public readonly unencryptedLogs: UnencryptedTxL2Logs, + public unencryptedLogs: UnencryptedTxL2Logs, /** * Enqueued public functions from the private circuit to be run by the sequencer. * Preimages of the public call stack entries from the private kernel circuit output. @@ -249,6 +250,37 @@ export class Tx { publicTeardownFunctionCall, ); } + + /** + * Filters out logs from functions that are not present in the provided kernel output. + * + * The purpose of this is to remove logs that got dropped due to a revert, + * in which case, we only have the kernel's hashes to go on, as opposed to + * this grouping by function maintained in this class. + * + * The logic therefore is to drop all FunctionLogs if any constituent hash + * does not appear in the provided hashes: it is impossible for part of a + * function to revert. + * + * @param logHashes the individual log hashes we want to keep + * @param out the output to put passing logs in, to keep this function abstract + */ + public filterRevertedLogs(kernelOutput: PublicKernelCircuitPublicInputs) { + this.encryptedLogs = this.encryptedLogs.filter( + kernelOutput.endNonRevertibleData.encryptedLogsHashes, + EncryptedTxL2Logs.empty(), + ); + + this.unencryptedLogs = this.unencryptedLogs.filter( + kernelOutput.endNonRevertibleData.unencryptedLogsHashes, + UnencryptedTxL2Logs.empty(), + ); + + this.noteEncryptedLogs = this.noteEncryptedLogs.filter( + kernelOutput.endNonRevertibleData.noteEncryptedLogsHashes, + EncryptedNoteTxL2Logs.empty(), + ); + } } /** Utility type for an entity that has a hash property for a txhash */ diff --git a/yarn-project/end-to-end/src/e2e_fees/failures.test.ts b/yarn-project/end-to-end/src/e2e_fees/failures.test.ts index 733a8577425..dd0bb68635c 100644 --- a/yarn-project/end-to-end/src/e2e_fees/failures.test.ts +++ b/yarn-project/end-to-end/src/e2e_fees/failures.test.ts @@ -36,9 +36,9 @@ describe('e2e_fees failures', () => { await t.teardown(); }); - it('reverts transactions but still pays fees using PublicFeePaymentMethod', async () => { - const OutrageousPublicAmountAliceDoesNotHave = BigInt(1e15); - const PublicMintedAlicePublicBananas = BigInt(1e12); + it('reverts transactions but still pays fees using PrivateFeePaymentMethod', async () => { + const OutrageousPublicAmountAliceDoesNotHave = BigInt(1e8); + const PrivateMintedAlicePrivateBananas = BigInt(1e15); const [initialAlicePrivateBananas, initialFPCPrivateBananas] = await t.bananaPrivateBalances( aliceAddress, @@ -48,21 +48,19 @@ describe('e2e_fees failures', () => { aliceAddress, bananaFPC.address, ); - const [initialAliceGas, initialFPCGas, initialSequencerGas] = await t.gasBalances( - aliceAddress, - bananaFPC.address, - sequencerAddress, - ); + const [initialAliceGas, initialFPCGas] = await t.gasBalances(aliceAddress, bananaFPC.address); + + await t.mintPrivateBananas(PrivateMintedAlicePrivateBananas, aliceAddress); - await bananaCoin.methods.mint_public(aliceAddress, PublicMintedAlicePublicBananas).send().wait(); // if we simulate locally, it throws an error await expect( bananaCoin.methods + // still use a public transfer so as to fail in the public app logic phase .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0) .send({ fee: { gasSettings, - paymentMethod: new PublicFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet), + paymentMethod: new PrivateFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet), }, }) .wait(), @@ -71,59 +69,69 @@ describe('e2e_fees failures', () => { // we did not pay the fee, because we did not submit the TX await expectMapping( t.bananaPrivateBalances, - [aliceAddress, bananaFPC.address, sequencerAddress], - [initialAlicePrivateBananas, initialFPCPrivateBananas, 0n], + [aliceAddress, bananaFPC.address], + [initialAlicePrivateBananas + PrivateMintedAlicePrivateBananas, initialFPCPrivateBananas], ); await expectMapping( t.bananaPublicBalances, - [aliceAddress, bananaFPC.address, sequencerAddress], - [initialAlicePublicBananas + PublicMintedAlicePublicBananas, initialFPCPublicBananas, 0n], - ); - await expectMapping( - t.gasBalances, - [aliceAddress, bananaFPC.address, sequencerAddress], - [initialAliceGas, initialFPCGas, initialSequencerGas], + [aliceAddress, bananaFPC.address], + [initialAlicePublicBananas, initialFPCPublicBananas], ); + await expectMapping(t.gasBalances, [aliceAddress, bananaFPC.address], [initialAliceGas, initialFPCGas]); // if we skip simulation, it includes the failed TX + const rebateSecret = Fr.random(); + const currentSequencerL1Gas = await t.getCoinbaseBalance(); const txReceipt = await bananaCoin.methods .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0) .send({ skipPublicSimulation: true, fee: { gasSettings, - paymentMethod: new PublicFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet), + paymentMethod: new PrivateFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet, rebateSecret), }, }) .wait({ dontThrowOnRevert: true }); expect(txReceipt.status).toBe(TxStatus.APP_LOGIC_REVERTED); const feeAmount = txReceipt.transactionFee!; + const newSequencerL1Gas = await t.getCoinbaseBalance(); + expect(newSequencerL1Gas).toEqual(currentSequencerL1Gas + feeAmount); // and thus we paid the fee await expectMapping( t.bananaPrivateBalances, - [aliceAddress, bananaFPC.address, sequencerAddress], - [initialAlicePrivateBananas, initialFPCPrivateBananas, 0n], + [aliceAddress, bananaFPC.address], + [ + // alice paid the maximum amount in private bananas + initialAlicePrivateBananas + PrivateMintedAlicePrivateBananas - gasSettings.getFeeLimit().toBigInt(), + initialFPCPrivateBananas, + ], ); await expectMapping( t.bananaPublicBalances, - [aliceAddress, bananaFPC.address, sequencerAddress], - [initialAlicePublicBananas + PublicMintedAlicePublicBananas - feeAmount, initialFPCPublicBananas + feeAmount, 0n], + [aliceAddress, bananaFPC.address], + [initialAlicePublicBananas, initialFPCPublicBananas + feeAmount], ); + await expectMapping(t.gasBalances, [aliceAddress, bananaFPC.address], [initialAliceGas, initialFPCGas - feeAmount]); + + // Alice can redeem her shield to get the rebate + const refund = gasSettings.getFeeLimit().toBigInt() - feeAmount; + expect(refund).toBeGreaterThan(0n); + const secretHashForRebate = computeSecretHash(rebateSecret); + await t.addPendingShieldNoteToPXE(t.aliceWallet, refund, secretHashForRebate, txReceipt.txHash); + await bananaCoin.methods.redeem_shield(aliceAddress, refund, rebateSecret).send().wait(); + await expectMapping( - t.gasBalances, - [aliceAddress, bananaFPC.address, sequencerAddress], - [initialAliceGas, initialFPCGas - feeAmount, initialSequencerGas], + t.bananaPrivateBalances, + [aliceAddress, bananaFPC.address], + [initialAlicePrivateBananas + PrivateMintedAlicePrivateBananas - feeAmount, initialFPCPrivateBananas], ); - - // TODO(#4712) - demonstrate reverts with the PrivateFeePaymentMethod. - // Can't do presently because all logs are "revertible" so we lose notes that get broadcasted during unshielding. }); - it('reverts transactions but still pays fees using PrivateFeePaymentMethod', async () => { - const OutrageousPublicAmountAliceDoesNotHave = BigInt(1e8); - const PrivateMintedAlicePrivateBananas = BigInt(1e15); + it('reverts transactions but still pays fees using PublicFeePaymentMethod', async () => { + const OutrageousPublicAmountAliceDoesNotHave = BigInt(1e15); + const PublicMintedAlicePublicBananas = BigInt(1e12); const [initialAlicePrivateBananas, initialFPCPrivateBananas] = await t.bananaPrivateBalances( aliceAddress, @@ -139,17 +147,15 @@ describe('e2e_fees failures', () => { sequencerAddress, ); - await t.mintPrivateBananas(PrivateMintedAlicePrivateBananas, aliceAddress); - + await bananaCoin.methods.mint_public(aliceAddress, PublicMintedAlicePublicBananas).send().wait(); // if we simulate locally, it throws an error await expect( bananaCoin.methods - // still use a public transfer so as to fail in the public app logic phase .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0) .send({ fee: { gasSettings, - paymentMethod: new PrivateFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet), + paymentMethod: new PublicFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet), }, }) .wait(), @@ -158,63 +164,50 @@ describe('e2e_fees failures', () => { // we did not pay the fee, because we did not submit the TX await expectMapping( t.bananaPrivateBalances, - [aliceAddress, bananaFPC.address], - [initialAlicePrivateBananas + PrivateMintedAlicePrivateBananas, initialFPCPrivateBananas], + [aliceAddress, bananaFPC.address, sequencerAddress], + [initialAlicePrivateBananas, initialFPCPrivateBananas, 0n], ); await expectMapping( t.bananaPublicBalances, - [aliceAddress, bananaFPC.address], - [initialAlicePublicBananas, initialFPCPublicBananas], + [aliceAddress, bananaFPC.address, sequencerAddress], + [initialAlicePublicBananas + PublicMintedAlicePublicBananas, initialFPCPublicBananas, 0n], + ); + await expectMapping( + t.gasBalances, + [aliceAddress, bananaFPC.address, sequencerAddress], + [initialAliceGas, initialFPCGas, initialSequencerGas], ); - await expectMapping(t.gasBalances, [aliceAddress, bananaFPC.address], [initialAliceGas, initialFPCGas]); // if we skip simulation, it includes the failed TX - const rebateSecret = Fr.random(); - const currentSequencerL1Gas = await t.getCoinbaseBalance(); const txReceipt = await bananaCoin.methods .transfer_public(aliceAddress, sequencerAddress, OutrageousPublicAmountAliceDoesNotHave, 0) .send({ skipPublicSimulation: true, fee: { gasSettings, - paymentMethod: new PrivateFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet, rebateSecret), + paymentMethod: new PublicFeePaymentMethod(bananaCoin.address, bananaFPC.address, aliceWallet), }, }) .wait({ dontThrowOnRevert: true }); expect(txReceipt.status).toBe(TxStatus.APP_LOGIC_REVERTED); const feeAmount = txReceipt.transactionFee!; - const newSequencerL1Gas = await t.getCoinbaseBalance(); - expect(newSequencerL1Gas).toEqual(currentSequencerL1Gas + feeAmount); // and thus we paid the fee await expectMapping( t.bananaPrivateBalances, - [aliceAddress, bananaFPC.address], - [ - // alice paid the maximum amount in private bananas - initialAlicePrivateBananas + PrivateMintedAlicePrivateBananas - gasSettings.getFeeLimit().toBigInt(), - initialFPCPrivateBananas, - ], + [aliceAddress, bananaFPC.address, sequencerAddress], + [initialAlicePrivateBananas, initialFPCPrivateBananas, 0n], ); await expectMapping( t.bananaPublicBalances, - [aliceAddress, bananaFPC.address], - [initialAlicePublicBananas, initialFPCPublicBananas + feeAmount], + [aliceAddress, bananaFPC.address, sequencerAddress], + [initialAlicePublicBananas + PublicMintedAlicePublicBananas - feeAmount, initialFPCPublicBananas + feeAmount, 0n], ); - await expectMapping(t.gasBalances, [aliceAddress, bananaFPC.address], [initialAliceGas, initialFPCGas - feeAmount]); - - // Alice can redeem her shield to get the rebate - const refund = gasSettings.getFeeLimit().toBigInt() - feeAmount; - expect(refund).toBeGreaterThan(0n); - const secretHashForRebate = computeSecretHash(rebateSecret); - await t.addPendingShieldNoteToPXE(t.aliceWallet, refund, secretHashForRebate, txReceipt.txHash); - await bananaCoin.methods.redeem_shield(aliceAddress, refund, rebateSecret).send().wait(); - await expectMapping( - t.bananaPrivateBalances, - [aliceAddress, bananaFPC.address], - [initialAlicePrivateBananas + PrivateMintedAlicePrivateBananas - feeAmount, initialFPCPrivateBananas], + t.gasBalances, + [aliceAddress, bananaFPC.address, sequencerAddress], + [initialAliceGas, initialFPCGas - feeAmount, initialSequencerGas], ); }); diff --git a/yarn-project/simulator/src/public/app_logic_phase_manager.ts b/yarn-project/simulator/src/public/app_logic_phase_manager.ts index bf25c580cdb..de2628b6bea 100644 --- a/yarn-project/simulator/src/public/app_logic_phase_manager.ts +++ b/yarn-project/simulator/src/public/app_logic_phase_manager.ts @@ -47,6 +47,7 @@ export class AppLogicPhaseManager extends AbstractPhaseManager { // if so, this is removing contracts deployed in private setup await this.publicContractsDB.removeNewContracts(tx); await this.publicStateDB.rollbackToCheckpoint(); + tx.filterRevertedLogs(kernelOutput); } else { tx.unencryptedLogs.addFunctionLogs(newUnencryptedLogs); // TODO(#6470): we should be adding contracts deployed in those logs to the publicContractsDB diff --git a/yarn-project/simulator/src/public/public_processor.test.ts b/yarn-project/simulator/src/public/public_processor.test.ts index 5040854a83a..4c2e84eeed1 100644 --- a/yarn-project/simulator/src/public/public_processor.test.ts +++ b/yarn-project/simulator/src/public/public_processor.test.ts @@ -230,6 +230,7 @@ describe('public_processor', () => { it('runs a tx with enqueued public calls', async function () { const tx = mockTxWithPartialState({ + hasLogs: true, numberOfRevertiblePublicCallRequests: 2, publicTeardownCallRequest: PublicCallRequest.empty(), }); @@ -253,6 +254,10 @@ describe('public_processor', () => { expect(publicWorldStateDB.commit).toHaveBeenCalledTimes(1); expect(publicWorldStateDB.rollbackToCommit).toHaveBeenCalledTimes(0); + // we keep the logs + expect(processed[0].encryptedLogs.getTotalLogCount()).toBe(6); + expect(processed[0].unencryptedLogs.getTotalLogCount()).toBe(2); + expect(prover.addNewTx).toHaveBeenCalledWith(processed[0]); }); @@ -346,7 +351,7 @@ describe('public_processor', () => { expect(prover.addNewTx).toHaveBeenCalledTimes(0); }); - it('rolls back app logic db updates on failed public execution, but persists setup/teardown', async function () { + it('rolls back app logic db updates on failed public execution, but persists setup', async function () { const baseContractAddressSeed = 0x200; const baseContractAddress = makeAztecAddress(baseContractAddressSeed); const publicCallRequests: PublicCallRequest[] = [ @@ -360,6 +365,7 @@ describe('public_processor', () => { const teardown = publicCallRequests.pop()!; // Remove the last call request to test that the processor can handle this const tx = mockTxWithPartialState({ + hasLogs: true, numberOfNonRevertiblePublicCallRequests: 1, numberOfRevertiblePublicCallRequests: 1, publicCallRequests, @@ -469,8 +475,10 @@ describe('public_processor', () => { expect(txEffect.publicDataWrites[4]).toEqual( new PublicDataWrite(computePublicDataTreeLeafSlot(baseContractAddress, contractSlotC), fr(0x201)), ); - expect(txEffect.encryptedLogs.getTotalLogCount()).toBe(0); - expect(txEffect.unencryptedLogs.getTotalLogCount()).toBe(0); + + // we keep the non-revertible logs + expect(txEffect.encryptedLogs.getTotalLogCount()).toBe(3); + expect(txEffect.unencryptedLogs.getTotalLogCount()).toBe(1); expect(prover.addNewTx).toHaveBeenCalledWith(processed[0]); }); @@ -589,6 +597,7 @@ describe('public_processor', () => { const teardown = publicCallRequests.pop()!; const tx = mockTxWithPartialState({ + hasLogs: true, numberOfNonRevertiblePublicCallRequests: 1, numberOfRevertiblePublicCallRequests: 1, publicCallRequests, @@ -689,8 +698,10 @@ describe('public_processor', () => { expect(txEffect.publicDataWrites[1]).toEqual( new PublicDataWrite(computePublicDataTreeLeafSlot(baseContractAddress, contractSlotA), fr(0x102)), ); - expect(txEffect.encryptedLogs.getTotalLogCount()).toBe(0); - expect(txEffect.unencryptedLogs.getTotalLogCount()).toBe(0); + + // we keep the non-revertible logs + expect(txEffect.encryptedLogs.getTotalLogCount()).toBe(3); + expect(txEffect.unencryptedLogs.getTotalLogCount()).toBe(1); expect(processed[0].data.revertCode).toEqual(RevertCode.TEARDOWN_REVERTED); @@ -711,6 +722,7 @@ describe('public_processor', () => { const teardown = publicCallRequests.pop()!; const tx = mockTxWithPartialState({ + hasLogs: true, numberOfNonRevertiblePublicCallRequests: 1, numberOfRevertiblePublicCallRequests: 1, publicCallRequests, @@ -812,8 +824,10 @@ describe('public_processor', () => { expect(txEffect.publicDataWrites[1]).toEqual( new PublicDataWrite(computePublicDataTreeLeafSlot(baseContractAddress, contractSlotA), fr(0x102)), ); - expect(txEffect.encryptedLogs.getTotalLogCount()).toBe(0); - expect(txEffect.unencryptedLogs.getTotalLogCount()).toBe(0); + + // we keep the non-revertible logs + expect(txEffect.encryptedLogs.getTotalLogCount()).toBe(3); + expect(txEffect.unencryptedLogs.getTotalLogCount()).toBe(1); expect(processed[0].data.revertCode).toEqual(RevertCode.BOTH_REVERTED); diff --git a/yarn-project/simulator/src/public/teardown_phase_manager.ts b/yarn-project/simulator/src/public/teardown_phase_manager.ts index bd1eafbcba9..14eb475746a 100644 --- a/yarn-project/simulator/src/public/teardown_phase_manager.ts +++ b/yarn-project/simulator/src/public/teardown_phase_manager.ts @@ -44,6 +44,7 @@ export class TeardownPhaseManager extends AbstractPhaseManager { ); if (revertReason) { await this.publicStateDB.rollbackToCheckpoint(); + tx.filterRevertedLogs(kernelOutput); } else { // TODO(#6464): Should we allow emitting contracts in the public teardown phase? // if so, we should insert them here