Skip to content
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

940 - getNotes filter nullified notes #1186

Merged
merged 5 commits into from
Jul 27, 2023
Merged

Conversation

jeanmon
Copy link
Contributor

@jeanmon jeanmon commented Jul 25, 2023

Description

Resolves #940

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@netlify
Copy link

netlify bot commented Jul 25, 2023

Deploy Preview for preeminent-bienenstitch-606ad0 canceled.

Name Link
🔨 Latest commit 1f34198
🔍 Latest deploy log https://app.netlify.com/sites/preeminent-bienenstitch-606ad0/deploys/64c2857fddab6200072e4b04

@jeanmon jeanmon force-pushed the jm/940-filter-nullified-notes branch 2 times, most recently from 570a240 to cd761fe Compare July 25, 2023 16:43
@benesjan benesjan force-pushed the jm/940-filter-nullified-notes branch 2 times, most recently from b190be2 to 17bd3e6 Compare July 26, 2023 06:45
@jeanmon jeanmon force-pushed the jm/940-filter-nullified-notes branch 8 times, most recently from c5b19a7 to 635f2aa Compare July 27, 2023 14:46
@jeanmon jeanmon marked this pull request as ready for review July 27, 2023 14:47
@jeanmon jeanmon requested a review from dbanks12 July 27, 2023 14:51
@jeanmon jeanmon force-pushed the jm/940-filter-nullified-notes branch from 635f2aa to 1f34198 Compare July 27, 2023 14:55
Copy link
Contributor

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Not much feedback at all. Really great work!

Comment on lines +207 to +233
* Update the list of pending notes by chopping a note which is nullified.
* The identifier used to determine matching is the inner note hash value.
* However, we adopt a defensive approach and ensure that the contract address
* and storage slot do match.
* Note that this method might be called with an innerNoteHash referring to
* a note created in a previous transaction which will result in this array
* of pending notes left unchanged.
* @param innerNoteHash - The inner note hash value to which note will be chopped.
* @param contractAddress - The contract address
* @param storageSlot - The storage slot as a Field Fr element
*/
public nullifyPendingNotes(innerNoteHash: Fr, contractAddress: AztecAddress, storageSlot: Fr) {
// IMPORTANT: We do need an in-place array mutation of this.pendingNotes as this array is shared
// by reference among the nested calls. That is the main reason for 'splice' usage below.
this.pendingNotes.splice(
0,
this.pendingNotes.length,
...this.pendingNotes.filter(
n =>
!(
n.innerNoteHash.equals(innerNoteHash) &&
n.contractAddress.equals(contractAddress) &&
n.storageSlot.equals(storageSlot)
),
),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! As a followup task, we can probably replace the PrivateFunctionExecution's newNotes and then clean up this chunk of code in the kernel prover

@jeanmon jeanmon merged commit 12830b0 into master Jul 27, 2023
@jeanmon jeanmon deleted the jm/940-filter-nullified-notes branch July 27, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Noir contracts shouldn't get notes that were nullified earlier in TX
2 participants