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

Add tests to ensure failure if an oracle maliciously returns note_header.nonce = 0 for a pre-existing note #1410

Closed
dbanks12 opened this issue Aug 3, 2023 · 2 comments

Comments

@dbanks12
Copy link
Collaborator

dbanks12 commented Aug 3, 2023

"We should add tests to ensure that an oracle which maliciously returns a note_header.nonce = 0 for a 'pre-existing note' results in a failing tx, because the resulting nullifier would be different from the 'correct' nullifier. Presumably the tx would correctly fail, either because the read request would result in a note which doesn't exist in the tree, or because the read request's note would look like a 'pending note', but there wouldn't be a corresponding earlier function which created such a pending note.
Not sure where in the stack such a test should be orchestrated?" - @iAmMichaelConnor

@github-project-automation github-project-automation bot moved this to Todo in A3 Aug 3, 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.
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.
@dbanks12
Copy link
Collaborator Author

dbanks12 commented Oct 1, 2024

@LeilaWang are we testing this? Would this still be a useful test?

@LeilaWang
Copy link
Collaborator

There are tests in noir to make sure that read requests can only be verified if it exists in the tree or in the pending set. I am not sure if we need a test that spans a bigger scope involving the oracle?

@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Oct 2, 2024
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

3 participants