feat: TXE fixes to avm opcodes and missing oracles, forced ci failure #7252
Conversation
Benchmark resultsMetrics with a significant change:
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 | | |
avm-transpiler/src/transpile.rs
Outdated
| @@ -992,7 +992,7 @@ fn handle_storage_read( | |||
| inputs: &Vec<ValueOrArray>, | |||
| ) { | |||
| // For the foreign calls we want to handle, we do not want inputs, as they are getters | |||
| assert!(inputs.len() == 1); // storage_slot | |||
| assert!(inputs.len() == 2); // output, len - but we dont use this len - its for the oracle | |||
There was a problem hiding this comment.
We need a bit more detail here. In particular explain that the destination already has a length, and that is the one that will be used. (and why do we need this extra length? what does "the oracle" mean)
However... ideally we'd have an assert here that length==destination.length, if you can force the length to be known at compile time.
If you can't I guess that's life.
There was a problem hiding this comment.
Clarified a bit in the comment (first just reverted the change), but I don't see an easy way to make the second assertion, since the destination size is known at this point, but the second argument is just a memory address. Is it possible to "unwrap" it somehow?
There was a problem hiding this comment.
Length should be known at compile time btw, since the output of the opcode is an array explicitly (not a slice)
There was a problem hiding this comment.
This is a question for @sirasistant
There was a problem hiding this comment.
The only way for the transpiler to do it is to emit a const opcode with destination.length and an opcode that checks equality between that one and the second argument. But without scratch space it's a pain. (should I reserve scratch space at this point? it has come up several times)
There was a problem hiding this comment.
If this is part of a larger discussion, I would leave as it is for the time being and try to get this in, since it's blocking TXE usage in public ATM (and this was already there, just removed recently)
There was a problem hiding this comment.
Yeah, I'm ok with it as it is. The question for Alvaro was more on the Noir/Brillig side. If there was any way to mark length as comptime/etc so that it gets to the transpiler as a resolved number.
There was a problem hiding this comment.
That'd be possible but I think there is no way to have a literal in foreign call params with the current brillig spec
…ocol/aztec-packages into gj/fail_ci_on_txe_test_failure
… gj/fail_ci_on_txe_test_failure
… gj/fail_ci_on_txe_test_failure
…ocol/aztec-packages into gj/fail_ci_on_txe_test_failure
… gj/fail_ci_on_txe_test_failure
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
Makes CI fail in case TXE test fail. Noticed they were broken, but master was still green and removed some unnecessary commands in the docker build.
Also reverted some of the changes from #7237 since we need the length of the data we're about to read in
avmOpcodeStorageRead(the oracle cannot obtain this information from the assigned output variable.