feat: Pass through unencrypted logs to base rollup #7457
Conversation
Changes to circuit sizes
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
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 | | |
| hints.siloed_unencrypted_log_hashes, | ||
| hints.sorted_siloed_unencrypted_log_hashes, | ||
| self.previous_kernel.end.unencrypted_logs_hashes, | ||
| hints.sorted_unencrypted_log_hashes, |
There was a problem hiding this comment.
This can be self.output.end.unencrypted_logs! And the assert_eq below won't be needed.
Actually, maybe there can be a variation of assert_sorted_array that takes the OrderHint. Something like:
assert_sorted_array(
self.previous_kernel.end.unencrypted_logs_hashes,
self.output.end.unencrypted_logs,
hints.sorted_unencrypted_log_hash_hints
);
There was a problem hiding this comment.
Hmmm but I think we cannot get rid of the assert_eq due to the self.output.end.unencrypted_logs being self.previous_kernel.end.unencrypted_logs_hashes reordered and exposed to public (so counters are zeroed out)
There was a problem hiding this comment.
I will write a specialized function to avoid using the transformed version when there was no transformation though!
| UnencryptedTxL2Logs.hashSiloedLogs( | ||
| tx.data.end.unencryptedLogsHashes.filter(hash => !hash.isEmpty()).map(h => h.getSiloedHash()), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
I find it weird that this validateProcessedTxLogs is called in the public processor, after it has processed a tx with or without public calls.
Shouldn't the check for log hashes vs log data be done in the tx validator? A tx shouldn't be considered valid if the user submits wrong log data for it.
The public processor doesn't have to check the logs after simulation if the check is already done before the tx is given to it. If there's any unencrypted log emitted from the public functions, the data will be there. Unless it's using a faulty simulator.
Maybe it's a question for @just-mitch? Wonder if it makes sense or am I missing something 😅
There was a problem hiding this comment.
That makes sense to me! It's even listed as a validity condition, but it is not done in yarn-project/sequencer-client/src/tx_validator/tx_validator_factory.ts
There was a problem hiding this comment.
Yes, I think it would need to be to be a tx validator since with the current approach the sequencer will select the TX for inclusion (and simulate its public functions, losing valuable time!) even if the preimages of the logs don't match the hashes in the kernels
Instead of siloing and crunching together all the unencrypted logs in the private kernels and the public kernels, we pass them through to the base rollup who will do that work.