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

refactor: remove addNullifiedNote from pxe #11822

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

sklppy88
Copy link
Contributor

@sklppy88 sklppy88 commented Feb 7, 2025

This PR removes addNullifiedNote from the PXE as it is unused.

It was also suggested to rework getNoteHashAndOptionallyANullifier into getNoteHashAndNullifier, in TS as now the second case where we don't get a nullifier is almost trivially used.

I have kept get_note_hash_and_optionally_a_nullifier in nr intact as I wasn't sure this desired changing. If so, I will do that in another PR to keep things relatively isolated.

Resolves #11821

@sklppy88 sklppy88 changed the title init refactor: remove addNullifiedNote from pxe Feb 7, 2025
Copy link
Contributor Author

sklppy88 commented Feb 7, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@sklppy88 sklppy88 added the e2e-all CI: Deprecated, use ci-full. Enable all master checks. label Feb 7, 2025
@sklppy88 sklppy88 force-pushed the ek/refactor/11821/remove-add-nullified-note-from-pxe branch 3 times, most recently from 5c19733 to 69bec66 Compare February 7, 2025 17:17
@sklppy88 sklppy88 marked this pull request as ready for review February 7, 2025 17:25
@sklppy88 sklppy88 force-pushed the ek/refactor/11821/remove-add-nullified-note-from-pxe branch 3 times, most recently from 3370666 to e3b3a76 Compare February 10, 2025 15:18
@sklppy88 sklppy88 force-pushed the ek/refactor/11821/remove-add-nullified-note-from-pxe branch 2 times, most recently from cceedb2 to c97f0ac Compare February 18, 2025 08:39
@@ -162,21 +162,19 @@ export class AcirSimulator {
}

/**
* Computes the inner nullifier of a note.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like this was stale from before

@sklppy88 sklppy88 requested a review from nventuro February 18, 2025 08:41
@sklppy88 sklppy88 force-pushed the ek/refactor/11821/remove-add-nullified-note-from-pxe branch 3 times, most recently from a80a79e to ba81b83 Compare February 18, 2025 10:21
await acirSimulator.computeNoteHash(contractAddress, newNote.storageSlot, newNote.noteTypeId, newNote.note),
await acirSimulator.computeNoteHashAndNullifier(
contractAddress,
Fr.ZERO,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously computeNoteHash, which we removed from the simulator, was just passing Fr.ZERO here anyways.

@sklppy88 sklppy88 force-pushed the ek/refactor/11821/remove-add-nullified-note-from-pxe branch from ba81b83 to 5109c01 Compare February 18, 2025 10:56
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

Lovely thanks!

@nventuro nventuro enabled auto-merge (squash) February 18, 2025 19:13
@nventuro nventuro merged commit 022f2d6 into master Feb 18, 2025
12 checks passed
@nventuro nventuro deleted the ek/refactor/11821/remove-add-nullified-note-from-pxe branch February 18, 2025 19:39
TomAFrench added a commit that referenced this pull request Feb 19, 2025
* master: (245 commits)
  chore: Fix unbound CI variable on release image bootstrap (#12095)
  fix: dry run on grind (#12088)
  fix(spartan): eth-execution logging (#12094)
  fix: aws_handle_evict recovery & termination (#12086)
  chore: Use native acvm when available on orchestrator tests (#11560)
  refactor: function macros cleanup (#12066)
  refactor: remove `addNullifiedNote` from pxe (#11822)
  fix: `#[aztec]` macro warnings (#12038)
  refactor!: Notes implementing `Packable<N>` (#12004)
  chore(ops): add gcloud cli into devbox base image (#12082)
  fix: hotfix grinding
  fix: L1 deployment on reth (#12060)
  fix: Add missing bootstrap fast aliases (#12078)
  fix: hash_str caching (#12074)
  fix: Naive attempt to fix nightly deployments (#12079)
  fix: kind smoke (#12084)
  refactor!: nuking `NoteHeader` (#11942)
  fix: inject dockerhub creds (#12072)
  feat(docs): Note discovery concepts page (#11760)
  chore: cleanup libp2p logger (#12058)
  ...
nventuro added a commit that referenced this pull request Feb 21, 2025
This removes the `addNote` function from PXE (`addNullifiedNote` had
been removed in #11822). With this change, PXE no longer needs to
understand how to compute note hashes and perform nonce discovery, which
means we can also get rid of all of that code, _plus_ we can delete the
mandatory `compute_note_hash_and_optionally_a_nullifier` contract
function, _plus_ all of the auxiliary code used to call those.

Instead, contracts that wish to deliver notes to their recipients via
offchain mechanisms (i.e. not the protocol logs) must create custom
unconstrained functions that know how to construct said notes and add
them to PXE. For cases such as `TransparentNote`, where all of the note
contents are public already, this is quite simple:
`aztec::discovery::process_private_log` can be leveraged to a great
degree by mimicking the log encoding aztec-nr uses - see the
TokenBlacklist and Test contracts for examples of this. More fine
grained control could be achieved by calling
`aztec::discovery::attempt_note_nonce_discovery` and then the
`deliver_note` oracle (which is essentially what `process_private_log`
does, sans the decoding).

The removal of `compute_note_hash_and_optionally_a_nullifier` freed us
from some legacy burdens in having to produce the 4 field array, dealing
with optional nullifier computation, etc., which in turn allowed for the
contract library method `_compute_note_hash_and_optionally_a_nullifier`
to be streamlined and converted into the new
`compute_note_hash_and_nullifier`, which matches
`aztec::discovery::ComputeNoteHashAndNullifier` and hence results in
much easier use of the discovery functions.

Tagging @critesjosh since `addNote` was quite a basic and old primitive.

Closes #11638.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Deprecated, use ci-full. Enable all master checks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remote addNullifiedNote from PXE
2 participants