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

getCommitment should be able to work with pending commitments #1029

Closed
Tracked by #512
dbanks12 opened this issue Jul 11, 2023 · 1 comment
Closed
Tracked by #512

getCommitment should be able to work with pending commitments #1029

dbanks12 opened this issue Jul 11, 2023 · 1 comment
Assignees

Comments

@dbanks12
Copy link
Contributor

To support getCommitment on pending commitments, will likely require notifyCreatedNote to output a note's commitment alongside the note preimage. It may need to then be siloed by contract address in the simulator.

@github-project-automation github-project-automation bot moved this to Todo in A3 Jul 11, 2023
@dbanks12 dbanks12 self-assigned this Jul 18, 2023
@dbanks12 dbanks12 moved this from Todo to In Progress in A3 Jul 19, 2023
iAmMichaelConnor pushed a commit that referenced this issue Aug 3, 2023
# Description

The way nonces work now, there can be inconsistencies in nonce
assignment in the simulator vs the private kernel. Furthermore, you
cannot know during function execution what the full set of commitments
will be for the whole TX as some new commitments may be nullified and
squashed. But we still want the ability to determine nonces and
therefore uniqueNoteHashes from L1 calldata alone. I am sure I am not
explaining all of the issues well enough, but it was determined that the
current nonce paradigm will not work and therefore we must rework it.

Rework nonces so that siloing by contract address happens first and
uniqueness comes later. For now, nonces are injeced by the private
ordering circuit (vs suggestion which was base rollup circuit). Pending
notes and their reads have no nonces when processed in kernel. The
public kernel (and therefore all commitments created in public
functions) does not use nonces.

Here was Mike's proposal for the rework:

![image](https://github.com/AztecProtocol/aztec-packages/assets/47112877/7b20c886-1e92-452c-a886-c3da5ed64e17)

Why not just use leaf index as nonce?

![image](https://github.com/AztecProtocol/aztec-packages/assets/47112877/e6337107-ac93-4a3b-b83c-27213cb5133d)

## Followup tasks
* #1029
* #1194
* #1329
* #1407
* #1408
* #1409
* #1410
* Future enhancement: The root rollup circuit could insert all messages
at the very beginning of the root rollup circuit, so that txs within the
rollup can refer to that state root and read L1>L2 messages immediately.
* #1383
* #1386
* We should implement subscription / polling methods for Aztec logs
* We should maybe write rpc functions which allow calldata to be
subscribed-to, keyed by tx_hash.
* If a dapp wants to write a note from a public function, a lot of honus
will be on a dapp developer to retain preimage information, query the
blockchain, and derive the nonce. We should provide some examples to
demonstrate this pattern.
AztecBot pushed a commit to AztecProtocol/docs that referenced this issue Aug 3, 2023
# Description

The way nonces work now, there can be inconsistencies in nonce
assignment in the simulator vs the private kernel. Furthermore, you
cannot know during function execution what the full set of commitments
will be for the whole TX as some new commitments may be nullified and
squashed. But we still want the ability to determine nonces and
therefore uniqueNoteHashes from L1 calldata alone. I am sure I am not
explaining all of the issues well enough, but it was determined that the
current nonce paradigm will not work and therefore we must rework it.

Rework nonces so that siloing by contract address happens first and
uniqueness comes later. For now, nonces are injeced by the private
ordering circuit (vs suggestion which was base rollup circuit). Pending
notes and their reads have no nonces when processed in kernel. The
public kernel (and therefore all commitments created in public
functions) does not use nonces.

Here was Mike's proposal for the rework:

![image](https://github.com/AztecProtocol/aztec-packages/assets/47112877/7b20c886-1e92-452c-a886-c3da5ed64e17)

Why not just use leaf index as nonce?

![image](https://github.com/AztecProtocol/aztec-packages/assets/47112877/e6337107-ac93-4a3b-b83c-27213cb5133d)

## Followup tasks
* AztecProtocol/aztec-packages#1029
* AztecProtocol/aztec-packages#1194
* AztecProtocol/aztec-packages#1329
* AztecProtocol/aztec-packages#1407
* AztecProtocol/aztec-packages#1408
* AztecProtocol/aztec-packages#1409
* AztecProtocol/aztec-packages#1410
* Future enhancement: The root rollup circuit could insert all messages
at the very beginning of the root rollup circuit, so that txs within the
rollup can refer to that state root and read L1>L2 messages immediately.
* AztecProtocol/aztec-packages#1383
* AztecProtocol/aztec-packages#1386
* We should implement subscription / polling methods for Aztec logs
* We should maybe write rpc functions which allow calldata to be
subscribed-to, keyed by tx_hash.
* If a dapp wants to write a note from a public function, a lot of honus
will be on a dapp developer to retain preimage information, query the
blockchain, and derive the nonce. We should provide some examples to
demonstrate this pattern.
@dbanks12 dbanks12 changed the title [TS, Noir] getCommitment should be able to work with pending commitments getCommitment should be able to work with pending commitments Aug 7, 2023
@dbanks12 dbanks12 removed this from the 🚀 Initial Aztec Sandbox Release milestone Aug 7, 2023
@dbanks12 dbanks12 assigned LeilaWang and unassigned dbanks12 Sep 12, 2023
LeilaWang added a commit that referenced this issue Sep 13, 2023
Closes #1142 #1029 

- Rename `oracle.getCommitment` to `oracle.checkNoteHashExists`.
- Change `Set.assert_contains_and_remove(note, nonce)` to take a nonce
in addition to a note. We calculate the inner note hash from the
provided note. And although the nonce can be set to the header of the
note, by making it a required parameter of this method makes it clearer
that nonce is needed to check the existence of a note hash. (An example
will be created in a later PR to show how a recipient can learn about
the nonce if they don't have the encrypted data.)
- Change the api on CommitmentDb to only return an index of a note hash:
the index is all we need to know if a note hash exists, and to use it to
get `readRequestMembershipWitnesses` later in the kernel prover.
- (A tiny change that is not really related to this PR): We don't have
to emit storage slot when notifying the simulator about a new nullifier.

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).

---------

Co-authored-by: David Banks <47112877+dbanks12@users.noreply.github.com>
@LeilaWang
Copy link
Contributor

Closed by #2256

@github-project-automation github-project-automation bot moved this from In Progress to Done in A3 Sep 13, 2023
superstar0402 added a commit to superstar0402/aztec-nr that referenced this issue Aug 16, 2024
# Description

The way nonces work now, there can be inconsistencies in nonce
assignment in the simulator vs the private kernel. Furthermore, you
cannot know during function execution what the full set of commitments
will be for the whole TX as some new commitments may be nullified and
squashed. But we still want the ability to determine nonces and
therefore uniqueNoteHashes from L1 calldata alone. I am sure I am not
explaining all of the issues well enough, but it was determined that the
current nonce paradigm will not work and therefore we must rework it.

Rework nonces so that siloing by contract address happens first and
uniqueness comes later. For now, nonces are injeced by the private
ordering circuit (vs suggestion which was base rollup circuit). Pending
notes and their reads have no nonces when processed in kernel. The
public kernel (and therefore all commitments created in public
functions) does not use nonces.

Here was Mike's proposal for the rework:

![image](https://github.com/AztecProtocol/aztec-packages/assets/47112877/7b20c886-1e92-452c-a886-c3da5ed64e17)

Why not just use leaf index as nonce?

![image](https://github.com/AztecProtocol/aztec-packages/assets/47112877/e6337107-ac93-4a3b-b83c-27213cb5133d)

## Followup tasks
* AztecProtocol/aztec-packages#1029
* AztecProtocol/aztec-packages#1194
* AztecProtocol/aztec-packages#1329
* AztecProtocol/aztec-packages#1407
* AztecProtocol/aztec-packages#1408
* AztecProtocol/aztec-packages#1409
* AztecProtocol/aztec-packages#1410
* Future enhancement: The root rollup circuit could insert all messages
at the very beginning of the root rollup circuit, so that txs within the
rollup can refer to that state root and read L1>L2 messages immediately.
* AztecProtocol/aztec-packages#1383
* AztecProtocol/aztec-packages#1386
* We should implement subscription / polling methods for Aztec logs
* We should maybe write rpc functions which allow calldata to be
subscribed-to, keyed by tx_hash.
* If a dapp wants to write a note from a public function, a lot of honus
will be on a dapp developer to retain preimage information, query the
blockchain, and derive the nonce. We should provide some examples to
demonstrate this pattern.
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

No branches or pull requests

2 participants