-
Notifications
You must be signed in to change notification settings - Fork 184
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
chore: purge unconstrained + batch simulate improvements #6639
Conversation
…tocol/aztec-packages into gj_lh/purge_unconstrained_batch_simulate
… gj_lh/purge_unconstrained_batch_simulate
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 16 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
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 | | |
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.
Mainly that we could use static for the getters 👀
noir-projects/noir-contracts/contracts/stateful_test_contract/src/main.nr
Show resolved
Hide resolved
expect(await this.token.methods.total_supply().simulate()).toEqual(this.totalSupply); | ||
|
||
// Check that all our public matches | ||
public async check(wallet: Wallet) { |
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.
Don't love this, works when we have just one wallet, but with multiple as we will need in escrow it gets funky, think we could set a default when we are constructing or something 🤷, But I can deal with that at the escrow instead.
Think we might also run into some issues if we are moving towards more calls than what a single batch can handle, or will it already be handled in the batch 🤔
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.
Haven't seen the escrow yet, was planning to check it out in a different PR to avoid merge hell, but yeah, it won't work with different wallets interacting (this was mainly to get it working and see if the time was somewhat reasonable)
The batching doesn't handle +4 fns per tx, we have to do it externally (we have just been lucky for the time being, but creating a private getter insteat of the current unconstrained will make it blow up). I can do it now if we anticipate the check function to be called with +4 constrained calls.
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.
Fixed the batching for mixing public/private and tested it by jerry-rigging the token test to do a bunch of calls that I had to split into multiple transactions.
I'll leave the escrowing shenanigans to you ❤️
Recovers work done by @LHerskind in #5819 combining it with batched simulations to avoid 💀 ⏳ in tests