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

feat: inclusion and non-inclusion proofs experiment #3255

Merged
merged 71 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
71 commits
Select commit Hold shift + click to select a range
c37efa5
WIP
benesjan Nov 7, 2023
3da1c78
WIP
benesjan Nov 7, 2023
94f43be
WIP
benesjan Nov 7, 2023
32d2e39
minting
benesjan Nov 7, 2023
6343553
nullifying the note
benesjan Nov 7, 2023
ce3c4ab
fix after rebase
benesjan Nov 20, 2023
f159824
formatting
benesjan Nov 20, 2023
c98a5af
WIP
benesjan Nov 20, 2023
bbf4e66
fix: getPublicKeyAndPartialAddress in TypesOracle
benesjan Nov 20, 2023
e152b2c
WIP
benesjan Nov 20, 2023
af94bc9
WIP
benesjan Nov 20, 2023
e267f8d
WIP
benesjan Nov 20, 2023
7d2fab6
WIP
benesjan Nov 21, 2023
7cdd90e
WIP
benesjan Nov 21, 2023
3bf3036
fix
benesjan Nov 21, 2023
78eaadb
computing blocks hash
benesjan Nov 21, 2023
5528be5
WIP on verifying block hash
benesjan Nov 21, 2023
61a3682
fix
benesjan Nov 23, 2023
39b6011
feat: get_block_data.nr
benesjan Nov 23, 2023
227553f
finished get_block_data oracle
benesjan Nov 23, 2023
3635866
comment
benesjan Nov 24, 2023
71508b1
HistoricBlockData::block_hash()
benesjan Nov 24, 2023
eb5c260
renaming
benesjan Nov 24, 2023
508ab15
cleanup
benesjan Nov 24, 2023
cf14859
cleanup
benesjan Nov 24, 2023
b10f34c
running test in CI
benesjan Nov 24, 2023
38ba6b4
fix
benesjan Nov 24, 2023
ab2ba46
typo
benesjan Nov 24, 2023
1819c24
using blocks tree root from context
benesjan Nov 24, 2023
0016721
comment
benesjan Nov 24, 2023
61c6f85
fix
benesjan Nov 24, 2023
7360736
feat: note utils compute_siloed_nullifier
benesjan Nov 24, 2023
f407ce7
get_low_nullifier_membership_witness.nr
benesjan Nov 24, 2023
4b779af
updated get_low_nullifier_membership_witness.nr
benesjan Nov 24, 2023
1a5c4ab
WIP on proveNullifierNonInclusion
benesjan Nov 24, 2023
ec0d319
WIP
benesjan Nov 24, 2023
b9699d7
WIP
benesjan Nov 24, 2023
ff79a1f
naming fix
benesjan Nov 24, 2023
5533c1a
WIP
benesjan Nov 27, 2023
9e8890c
WIP
benesjan Nov 27, 2023
2329113
formatting
benesjan Nov 27, 2023
b298fc8
nullifier non-inclusion finished
benesjan Nov 27, 2023
4d4ecf1
updated comments
benesjan Nov 27, 2023
783b5ca
updated comments
benesjan Nov 27, 2023
5a1cbab
more consistent naming
benesjan Nov 27, 2023
9312223
overflow fix
benesjan Nov 27, 2023
c6881b0
WIP
benesjan Nov 27, 2023
976278c
fixes
benesjan Nov 27, 2023
4e29bb5
getPublicDataTreeSiblingPath
benesjan Nov 28, 2023
8dca2be
test fix
benesjan Nov 28, 2023
11bbc4d
refactor: constraining block data in a separate function
benesjan Nov 28, 2023
4b2f793
cleanup
benesjan Nov 28, 2023
17eb82d
docs fixes
benesjan Nov 28, 2023
5dd0a8b
Adding TODO
benesjan Nov 28, 2023
e10c3cb
clarified naming
benesjan Nov 28, 2023
fdca497
todo
benesjan Nov 28, 2023
acc1cb1
passing context instead of blocks tree root
benesjan Nov 28, 2023
e9769bb
forgotten TODO
benesjan Nov 28, 2023
a52985b
merkle tree id issue
benesjan Nov 28, 2023
cea9ae8
fixed TSDoc
benesjan Nov 28, 2023
c5a10e4
WIP on nullifier inclusion proof (currently broken)
benesjan Nov 28, 2023
3c89221
fix nullifier inclusion
benesjan Nov 28, 2023
df26f17
LowNullifierMembershipWitness > NullifierMembershipWitness
benesjan Nov 28, 2023
c90c3e2
test: nullifier non-inclusion proof failure case
benesjan Nov 29, 2023
1745153
feat: PrivateContext::get_block_data(...)
benesjan Nov 29, 2023
138c7ef
test: more failure cases
benesjan Nov 29, 2023
b8678ae
Merge branch 'master' into janb/liquidity-mining-experiment
benesjan Nov 29, 2023
b8a1e6c
TODOs
benesjan Nov 29, 2023
edf4628
clearer errors
benesjan Nov 29, 2023
da730d9
using Promise.all
benesjan Nov 29, 2023
52a9095
Merge branch 'master' into janb/liquidity-mining-experiment
benesjan Nov 29, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions yarn-project/aztec-nr/aztec/src/context.nr
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ use crate::oracle::{
public_call::call_public_function_internal,
enqueue_public_function_call::enqueue_public_function_call_internal,
context::get_portal_address,
get_block_data::get_block_data,
};

use dep::std::option::Option;
Expand Down Expand Up @@ -128,6 +129,10 @@ impl PrivateContext {
self.inputs.call_context.function_selector
}

pub fn get_block_data(self, block_number: Field) -> HistoricBlockData {
get_block_data(block_number, self)
}

pub fn finish(self) -> abi::PrivateCircuitPublicInputs {
// TODO(https://github.com/AztecProtocol/aztec-packages/issues/1165)
let encrypted_logs_hash = [0; NUM_FIELDS_PER_SHA256];
Expand Down
38 changes: 34 additions & 4 deletions yarn-project/end-to-end/src/e2e_inclusion_proofs_contract.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { AccountWallet, CompleteAddress, PXE } from '@aztec/aztec.js';
import { AccountWallet, AztecAddress, CompleteAddress, Fr, PXE } from '@aztec/aztec.js';
import { InclusionProofsContract } from '@aztec/noir-contracts/types';

import { jest } from '@jest/globals';
Expand Down Expand Up @@ -29,7 +29,7 @@ describe('e2e_inclusion_proofs_contract', () => {

afterAll(() => teardown());

it('creates a note and proves its existence', async () => {
it('proves note existence and its nullifier non-existence and nullifier non-existence failure case', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this failure case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the test I nullify the note and I try to prove the nullifier non-inclusion again.

// Owner of a note
const owner = accounts[0].address;
{
Expand All @@ -48,14 +48,16 @@ describe('e2e_inclusion_proofs_contract', () => {
// Prove note inclusion in a given block.
// We prove the note existence at current block number because we don't currently have historical data
const blockNumber = await pxe.getBlockNumber();
await contract.methods.proveNoteInclusion(owner, blockNumber).send().wait();
const ignoredCommitment = 0; // Not ignored only when the note doesn't exist
await contract.methods.proveNoteInclusion(owner, blockNumber, ignoredCommitment).send().wait();
}

{
// Prove that the note has not been nullified
// We prove the note existence at current block number because we don't currently have historical data
const blockNumber = await pxe.getBlockNumber();
await contract.methods.proveNullifierNonInclusion(owner, blockNumber, 0).send().wait();
const ignoredNullifier = 0; // Not ignored only when the note doesn't exist
await contract.methods.proveNullifierNonInclusion(owner, blockNumber, ignoredNullifier).send().wait();
}

{
Expand All @@ -75,16 +77,44 @@ describe('e2e_inclusion_proofs_contract', () => {
}
});

it('note existence failure case', async () => {
// Owner of a note
const owner = AztecAddress.random();

const blockNumber = await pxe.getBlockNumber();
const randomNoteCommitment = Fr.random();
await expect(
contract.methods.proveNoteInclusion(owner, blockNumber, randomNoteCommitment).send().wait(),
).rejects.toThrow(/Leaf value: 0x[0-9a-fA-F]+ not found in tree/);
Copy link
Contributor

Choose a reason for hiding this comment

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

UX thing - why are the error messages so different from each other? If it isn't tooo much work, Can all of them be consistent:

For note failure - Can't prove note inclusion in note hash tree for leaf value: 0xabc. Are you sure note exists?
For public data - Can't prove value exists in public data tree for value: 0xabc. Are you sure it exists?
For nullifier inclusion failure - Can't prove nullifer inclusion in nullifier tree for leaf value: 0xabc. Are you sure nullifier exists?

Actually Jan for nullifier tree inclusion, do we check against the low nullifier witness?

For nullifier noninclusion failure - `nullifier exists in the nullifier tree. Low nullifier [value].next_value is [nullifier]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the errors clearer in edf4628

For the note inclusion failure case it's having the same error is a bit tricky because there it fails in the oracle call and not when doing assertions and that's because in sparse tree and indexed tree you never fail to get a membership witness.

Actually Jan for nullifier tree inclusion, do we check against the low nullifier witness?

No, for nullifier inclusion I just do standard membership proof.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the standard to do it the low nullifier way in the rest of the codebase? I have not worked with indexed tree much before

});

it('proves an existence of a public value in private context', async () => {
const blockNumber = await pxe.getBlockNumber();
await contract.methods.provePublicValueInclusion(publicValue, blockNumber).send().wait();
});

it('public value existence failure case', async () => {
const blockNumber = await pxe.getBlockNumber();
const randomPublicValue = Fr.random();
await expect(
contract.methods.provePublicValueInclusion(randomPublicValue, blockNumber).send().wait(),
).rejects.toThrow(/Proving membership of a value in public data tree failed/);
});

it('proves existence of a nullifier in private context', async () => {
benesjan marked this conversation as resolved.
Show resolved Hide resolved
const blockNumber = await pxe.getBlockNumber();
const block = await pxe.getBlock(blockNumber);
const nullifier = block?.newNullifiers[0];

await contract.methods.proveNullifierInclusion(nullifier!, blockNumber).send().wait();
});

it('nullifier existence failure case', async () => {
const blockNumber = await pxe.getBlockNumber();
const randomNullifier = Fr.random();

await expect(contract.methods.proveNullifierInclusion(randomNullifier, blockNumber).send().wait()).rejects.toThrow(
/Low nullifier witness not found for nullifier 0x[0-9a-fA-F]+ at block/,
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -101,23 +101,29 @@ contract InclusionProofs {
fn proveNoteInclusion(
benesjan marked this conversation as resolved.
Show resolved Hide resolved
owner: AztecAddress,
block_number: Field, // The block at which we'll prove that the note exists
spare_commitment: Field, // This is only used when the note is not found --> used to test the failure case
) {
// TODO: assert that block number is less than the block number of context.block_data
// --> This will either require a new oracle method that returns block_data.global_variables_hash preimage
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer the oracle for one main reason:
If we expose it in the context, I believe people will think that it is the current one because that is kinda how you would use it in public etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But having the oracle would result in more hashing because I would have to hash there the preimage to check that it matches the globals hash. Don't you think having a function PrivateContext::block_data_block_number or something like HistoricBlockData::data_block_number would be enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the HistoricBlockData one, but generally don't want to make it too easy for people to blow off their own heads by being stupid.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about PrivateContext::historic_block_data_block_number

// or modifying the private context so that we somehow expose it.

// 1) Get historic block data from oracle and ensure that the block hash is included in the current blocks tree
// root.
let block_data = get_block_data(block_number, context);
let block_data = context.get_block_data(block_number);

// 2) Get the note from PXE.
Copy link
Contributor

Choose a reason for hiding this comment

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

The amount of boiler plate for this is quite wild.

Copy link
Member

Choose a reason for hiding this comment

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

I think the only thing that isnt boilerplate is the actual commitment itself, this could probably be moved into a method to be reused? Potentially bolted onto the context

Copy link
Contributor Author

@benesjan benesjan Nov 29, 2023

Choose a reason for hiding this comment

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

I think that manually calling the compute_unique_siloed_note_hash function is not common enough to justify bolting it onto context. But getting a first note from a set and unwrapping it seems very common.

Should we expand Set to have get_first function? Then this code:

let private_values = storage.private_values.at(owner.address);
let options = NoteGetterOptions::new().select(1, owner.address).set_limit(1);
let notes = private_values.get_notes(options);
let note = notes[0].unwrap();

would look like this:

let note = storage.private_values.at(owner.address).get_first();

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you would need to have some filter or sort in there for the get_first to be nice. I is only often used because we mostly don't have a lot of notes in the tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

A Set is by definition an unordered collection. There's no such thing as "first".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iAmMichaelConnor get_one()?

I am not really rooting for doing this now as it's not clear yet if this is a common usecase.

let private_values = storage.private_values.at(owner.address);
let options = NoteGetterOptions::new().select(1, owner.address).set_limit(1);
let notes = private_values.get_notes(options);
let note = notes[0].unwrap();
let maybe_note = notes[0];

// 3) Compute the commitment from the note
let note_commitment = note_utils::compute_unique_siloed_note_hash(ValueNoteMethods, note);
let note_commitment = if maybe_note.is_some() {
note_utils::compute_unique_siloed_note_hash(ValueNoteMethods, maybe_note.unwrap_unchecked())
} else {
// Note was not found so we will use the spare commitment
spare_commitment
};

// 4) Get the membership witness of the note in the note hash tree
let note_hash_tree_id = 2; // TODO(#3443)
Expand Down Expand Up @@ -146,7 +152,7 @@ contract InclusionProofs {

// 1) Get historic block data from oracle and ensure that the block hash is included in the current blocks tree
// root.
let block_data = get_block_data(block_number, context);
let block_data = context.get_block_data(block_number);

// 2) Get the note from PXE
let private_values = storage.private_values.at(owner.address);
Expand Down Expand Up @@ -217,7 +223,7 @@ contract InclusionProofs {

// 1) Get historic block data from oracle and ensure that the block hash is included in the current blocks tree
// root.
let block_data = get_block_data(block_number, context);
let block_data = context.get_block_data(block_number);

// 2) Get the membership witness of the nullifier
let witness = get_nullifier_membership_witness(block_number, nullifier);
Expand Down Expand Up @@ -249,7 +255,7 @@ contract InclusionProofs {

// 1) Get historic block data from oracle and ensure that the block hash is included in the current blocks tree
// root.
let block_data = get_block_data(block_number, context);
let block_data = context.get_block_data(block_number);

// 2) Compute the public value leaf index.
// We have to compute the leaf index here because unlike in the case of note commitments, public values are
Expand Down