-
Notifications
You must be signed in to change notification settings - Fork 310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: adding aztec node tx validation tests #8220
fix: adding aztec node tx validation tests #8220
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
3415327
to
2563301
Compare
Benchmark resultsNo metrics with a significant change found. Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 8 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
AVM SimulationTime to simulate various public functions in the AVM.
Public DB AccessTime to access various public DBs.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | |
2563301
to
da8276c
Compare
it('tests that the node correctly validates chain id', async () => { | ||
const tx = mockTxForRollup(0x10000); | ||
tx.data.constants.txContext.chainId = chainId; | ||
|
||
const mockedGlobalVariables = new GlobalVariables( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised by all of this, did we have no tests for the node validations at all?
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 <[email protected]>
ffb3fe1
to
30d2493
Compare
da8276c
to
501d079
Compare
501d079
into
ek/fix/4781/adding-double-spend-validator-to-node
This PR adds some tests on the aztec node for the validation of txs as requested by @nventuro on the previous PR in the stack.