-
Notifications
You must be signed in to change notification settings - Fork 312
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
feat!: constrain note_getter filter #6703
Conversation
Docs PreviewHey there! 👋 You can check your preview at https://66576dd0043c341571049363--aztec-docs-dev.netlify.app |
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. | Metric | | 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.
Think Jans comment on NoteHeader Eq is good and would make it less error prone 👍
@@ -6,6 +6,12 @@ keywords: [sandbox, cli, aztec, notes, migration, updating, upgrading] | |||
|
|||
Aztec is in full-speed development. Literally every version breaks compatibility with the previous ones. This page attempts to target errors and difficulties you might encounter when upgrading, and how to resolve them. | |||
|
|||
## TBD |
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.
This should be in 0.42 which is the next release
The application of the filter was not constrained when reading notes, this PR changes that. While filtering can be thought of as assisting PXE during note retrieval to avoid unnecessary extra read requests, I think most users will also assume that the function will also lay down constrains.
The biggest motivation for this change is that this is how
select()
work: instructions are sent to the PXE to only return certain notes, and we then verify that the oracle behaved as expected.filter()
is simply an extension of this for conditions that cannot be expressed in a simple select: it is extremely unexpected that filtering would not be constrained when selects are.Consider the following example:
An example is
filter_notes_min_sum
, which keeps notes until as long as the sum is below some minimum. Callers still have to process the sum again on their end however and compare it to the minimum, since the filter used to provide no guarantees. In some cases this was hidden behind multiple layers of indirection, and as a resultValueNote::decrement_by_at_most
does not really uphold that the balance is decremented by less thanmax_balance
- a very subtle bug.