From 427c879c697bdb3f984a79c91c40eadaf782e1fd Mon Sep 17 00:00:00 2001 From: sklppy88 Date: Tue, 13 Aug 2024 18:50:33 +0000 Subject: [PATCH] init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix: fixing private voting by correctly throwing an error if simulation fails (#7840) This PR makes a simulation of a tx fail, if the tx cannot be included in a block and added to the state. e.g. If a simulation produces duplicate nullifiers, or nullifiers that already exist in a state tree, the results of this simulation should not be returned, but should warn users that the transaction simulated is impossible to actually be added to a block due to being an invalid transaction. The method for achieving the above is that a new API on the node was created, used for validating the correctness of the metadata and side-effects produced by a transaction. A transaction is deemed valid if and only if the transaction can be added to a block that can be used to advance state. Note: this update does not make this validation necessary, and defaults to offline simulation. Offline simulation is previous non-validated behavior, and is potentially useful if we ever move to a model where a node is optional to a pxe. Another note just for reference: there is weirdness in e2e_prover, that fails the proof validation. Resolves #4781. Apply suggestions from code review Co-authored-by: Nicolás Venturo --- yarn-project/aztec-node/src/aztec-node/server.ts | 15 ++++++++++++--- .../src/contract/contract_function_interaction.ts | 4 +++- yarn-project/aztec.js/src/wallet/base_wallet.ts | 9 +++++++-- .../circuit-types/src/interfaces/aztec-node.ts | 8 ++++++++ yarn-project/circuit-types/src/interfaces/pxe.ts | 2 ++ yarn-project/pxe/src/pxe_service/pxe_service.ts | 7 +++++++ 6 files changed, 39 insertions(+), 6 deletions(-) diff --git a/yarn-project/aztec-node/src/aztec-node/server.ts b/yarn-project/aztec-node/src/aztec-node/server.ts index 1a4d9635e7b..1ce2b13d582 100644 --- a/yarn-project/aztec-node/src/aztec-node/server.ts +++ b/yarn-project/aztec-node/src/aztec-node/server.ts @@ -324,9 +324,7 @@ export class AztecNodeService implements AztecNode { const timer = new Timer(); this.log.info(`Received tx ${tx.getTxHash()}`); - const [_, invalidTxs] = await this.txValidator.validateTxs([tx]); - if (invalidTxs.length > 0) { - this.log.warn(`Rejecting tx ${tx.getTxHash()} because of validation errors`); + if (!(await this.isValidTx(tx))) { this.metrics.receivedTx(timer.ms(), false); return; } @@ -752,6 +750,17 @@ export class AztecNodeService implements AztecNode { ); } + public async isValidTx(tx: Tx): Promise { + const [_, invalidTxs] = await this.txValidator.validateTxs([tx]); + if (invalidTxs.length > 0) { + this.log.warn(`Rejecting tx ${tx.getTxHash()} because of validation errors`); + + return false; + } + + return true; + } + public async setConfig(config: Partial): Promise { const newConfig = { ...this.config, ...config }; this.sequencer?.updateSequencerConfig(config); diff --git a/yarn-project/aztec.js/src/contract/contract_function_interaction.ts b/yarn-project/aztec.js/src/contract/contract_function_interaction.ts index 7d31f4f74f4..6041664eca7 100644 --- a/yarn-project/aztec.js/src/contract/contract_function_interaction.ts +++ b/yarn-project/aztec.js/src/contract/contract_function_interaction.ts @@ -23,6 +23,8 @@ export type SimulateMethodOptions = { from?: AztecAddress; /** Gas settings for the simulation. */ gasSettings?: GasSettings; + /** Option to throw an error if simulation does not produce a valid transaction. */ + skipTxValidation?: boolean; }; /** @@ -93,7 +95,7 @@ export class ContractFunctionInteraction extends BaseContractInteraction { } const txRequest = await this.create(); - const simulatedTx = await this.wallet.simulateTx(txRequest, true, options?.from); + const simulatedTx = await this.wallet.simulateTx(txRequest, true, options?.from, options?.skipTxValidation); // As account entrypoints are private, for private functions we retrieve the return values from the first nested call // since we're interested in the first set of values AFTER the account entrypoint diff --git a/yarn-project/aztec.js/src/wallet/base_wallet.ts b/yarn-project/aztec.js/src/wallet/base_wallet.ts index b4c1ebec830..f170202c719 100644 --- a/yarn-project/aztec.js/src/wallet/base_wallet.ts +++ b/yarn-project/aztec.js/src/wallet/base_wallet.ts @@ -109,8 +109,13 @@ export abstract class BaseWallet implements Wallet { proveTx(txRequest: TxExecutionRequest, simulatePublic: boolean): Promise { return this.pxe.proveTx(txRequest, simulatePublic, this.scopes); } - simulateTx(txRequest: TxExecutionRequest, simulatePublic: boolean, msgSender?: AztecAddress): Promise { - return this.pxe.simulateTx(txRequest, simulatePublic, msgSender, this.scopes); + simulateTx( + txRequest: TxExecutionRequest, + simulatePublic: boolean, + msgSender?: AztecAddress, + skipTxValidation?: boolean, + ): Promise { + return this.pxe.simulateTx(txRequest, simulatePublic, msgSender, skipTxValidation, this.scopes); } sendTx(tx: Tx): Promise { return this.pxe.sendTx(tx); diff --git a/yarn-project/circuit-types/src/interfaces/aztec-node.ts b/yarn-project/circuit-types/src/interfaces/aztec-node.ts index 4973715724f..73fd5f61f8a 100644 --- a/yarn-project/circuit-types/src/interfaces/aztec-node.ts +++ b/yarn-project/circuit-types/src/interfaces/aztec-node.ts @@ -316,6 +316,14 @@ export interface AztecNode { **/ simulatePublicCalls(tx: Tx): Promise; + /** + * Returns true if the transaction is valid for inclusion at the current state. Valid transactions can be + * made invalid by *other* transactions if e.g. they emit the same nullifiers, or come become invalid + * due to e.g. the max_block_number property. + * @param tx - The transaction to validate for correctness. + */ + isValidTx(tx: Tx): Promise; + /** * Updates the configuration of this node. * @param config - Updated configuration to be merged with the current one. diff --git a/yarn-project/circuit-types/src/interfaces/pxe.ts b/yarn-project/circuit-types/src/interfaces/pxe.ts index 3fb1dad58e8..bcff339a02f 100644 --- a/yarn-project/circuit-types/src/interfaces/pxe.ts +++ b/yarn-project/circuit-types/src/interfaces/pxe.ts @@ -180,6 +180,7 @@ export interface PXE { * @param txRequest - An authenticated tx request ready for simulation * @param simulatePublic - Whether to simulate the public part of the transaction. * @param msgSender - (Optional) The message sender to use for the simulation. + * @param skipTxValidation - (Optional) If false, this function throws if the transaction is unable to be included in a block at the current state. * @param scopes - (Optional) The accounts whose notes we can access in this call. Currently optional and will default to all. * @returns A simulated transaction object that includes a transaction that is potentially ready * to be sent to the network for execution, along with public and private return values. @@ -190,6 +191,7 @@ export interface PXE { txRequest: TxExecutionRequest, simulatePublic: boolean, msgSender?: AztecAddress, + skipTxValidation?: boolean, scopes?: AztecAddress[], ): Promise; diff --git a/yarn-project/pxe/src/pxe_service/pxe_service.ts b/yarn-project/pxe/src/pxe_service/pxe_service.ts index ec4629d75da..79bca549717 100644 --- a/yarn-project/pxe/src/pxe_service/pxe_service.ts +++ b/yarn-project/pxe/src/pxe_service/pxe_service.ts @@ -513,6 +513,7 @@ export class PXEService implements PXE { txRequest: TxExecutionRequest, simulatePublic: boolean, msgSender: AztecAddress | undefined = undefined, + skipTxValidation: boolean = true, scopes?: AztecAddress[], ): Promise { return await this.jobQueue.put(async () => { @@ -521,6 +522,12 @@ export class PXEService implements PXE { simulatedTx.publicOutput = await this.#simulatePublicCalls(simulatedTx.tx); } + if (!skipTxValidation) { + if (!(await this.node.isValidTx(simulatedTx.tx))) { + throw new Error('The simulated transaction is unable to be added to state and is invalid.'); + } + } + // We log only if the msgSender is undefined, as simulating with a different msgSender // is unlikely to be a real transaction, and likely to be only used to read data. // Meaning that it will not necessarily have produced a nullifier (and thus have no TxHash)