Skip to content

Commit

Permalink
feat!: remove SharedMutablePrivateGetter (#8749)
Browse files Browse the repository at this point in the history 
Missed this deletion while removing the key registry. I slightly
adjusted one of the e2e tests that used to check certain private getter
properties so that it still performs the same checks on the regular
shared mutable, as that doesn't seem to be present in any other e2e
test.
  • Loading branch information
@nventuro @Rumata888
nventuro authored and Rumata888 committed Sep 27, 2024
1 parent fa17813 commit 216ef99
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 201 deletions.
4 changes: 4 additions & 0 deletions docs/docs/migration_notes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ Aztec is in full-speed development. Literally every version breaks compatibility

## TBD

### [Aztec.nr] Removed `SharedMutablePrivateGetter`

This state variable was deleted due to it being difficult to use safely.

### [Aztec.nr] Changes to `NullifiableNote`

The `compute_nullifier_without_context` function is now `unconstrained`. It had always been meant to be called in unconstrained contexts (which is why it did not receive the `context` object), but now that Noir supports trait functions being `unconstrained` this can be implemented properly. Users must add the `unconstrained` keyword to their implementations of the trait:
Expand Down
2 changes: 0 additions & 2 deletions noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
mod shared_mutable;
mod scheduled_delay_change;
mod scheduled_value_change;
mod shared_mutable_private_getter;

pub use shared_mutable::SharedMutable;
pub use shared_mutable_private_getter::SharedMutablePrivateGetter;
79 changes: 38 additions & 41 deletions noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,46 +81,6 @@ impl<T, let INITIAL_DELAY: u32, Context> SharedMutable<T, INITIAL_DELAY, Context
fn get_hash_storage_slot(self) -> Field {
pedersen_hash([self.storage_slot], HASH_SEPARATOR)
}

// It may seem odd that we take a header and address instead of reading from e.g. a PrivateContext, but this lets us
// reuse this function in SharedMutablePrivateGetter.
fn historical_read_from_public_storage(
self,
header: Header,
address: AztecAddress
) -> (ScheduledValueChange<T>, ScheduledDelayChange<INITIAL_DELAY>, u32) {
let historical_block_number = header.global_variables.block_number as u32;

// We could simply produce historical inclusion proofs for both the ScheduledValueChange and
// ScheduledDelayChange, but that'd require one full sibling path per storage slot (since due to kernel siloing
// the storage is not contiguous), and in the best case in which T is a single field that'd be 4 slots.
// Instead, we get an oracle to provide us the correct values for both the value and delay changes, and instead
// prove inclusion of their hash, which is both a much smaller proof (a single slot), and also independent of
// the size of T.
let (value_change_hint, delay_change_hint) = unsafe {
get_public_storage_hints(address, self.storage_slot, historical_block_number)
};

// Ideally the following would be simply public_storage::read_historical, but we can't implement that yet.
let hash = header.public_storage_historical_read(self.get_hash_storage_slot(), address);

if hash != 0 {
assert_eq(
hash, SharedMutable::hash_scheduled_data(value_change_hint, delay_change_hint), "Hint values do not match hash"
);
} else {
// The hash slot can only hold a zero if it is uninitialized, meaning no value or delay change was ever
// scheduled. Therefore, the hints must then correspond to uninitialized scheduled changes.
assert_eq(
value_change_hint, ScheduledValueChange::deserialize(zeroed()), "Non-zero value change for zero hash"
);
assert_eq(
delay_change_hint, ScheduledDelayChange::deserialize(zeroed()), "Non-zero delay change for zero hash"
);
};

(value_change_hint, delay_change_hint, historical_block_number)
}
}

impl<T, let INITIAL_DELAY: u32> SharedMutable<T, INITIAL_DELAY, &mut PublicContext> where T: ToField + FromField + Eq {
Expand Down Expand Up @@ -203,7 +163,7 @@ impl<T, let INITIAL_DELAY: u32> SharedMutable<T, INITIAL_DELAY, &mut PrivateCont
// will only be valid for however many blocks we can ensure the value will not change, which will depend on the
// current delay and any scheduled delay changes.

let (value_change, delay_change, historical_block_number) = self.historical_read_from_public_storage(self.context.get_header(), self.context.this_address());
let (value_change, delay_change, historical_block_number) = self.historical_read_from_public_storage();

// We use the effective minimum delay as opposed to the current delay at the historical block as this one also
// takes into consideration any scheduled delay changes.
Expand All @@ -221,6 +181,43 @@ impl<T, let INITIAL_DELAY: u32> SharedMutable<T, INITIAL_DELAY, &mut PrivateCont
value_change.get_current_at(historical_block_number)
}

fn historical_read_from_public_storage(self) -> (ScheduledValueChange<T>, ScheduledDelayChange<INITIAL_DELAY>, u32) {
let header = self.context.get_header();
let address = self.context.this_address();

let historical_block_number = header.global_variables.block_number as u32;

// We could simply produce historical inclusion proofs for both the ScheduledValueChange and
// ScheduledDelayChange, but that'd require one full sibling path per storage slot (since due to kernel siloing
// the storage is not contiguous), and in the best case in which T is a single field that'd be 4 slots.
// Instead, we get an oracle to provide us the correct values for both the value and delay changes, and instead
// prove inclusion of their hash, which is both a much smaller proof (a single slot), and also independent of
// the size of T.
let (value_change_hint, delay_change_hint) = unsafe {
get_public_storage_hints(address, self.storage_slot, historical_block_number)
};

// Ideally the following would be simply public_storage::read_historical, but we can't implement that yet.
let hash = header.public_storage_historical_read(self.get_hash_storage_slot(), address);

if hash != 0 {
assert_eq(
hash, SharedMutable::hash_scheduled_data(value_change_hint, delay_change_hint), "Hint values do not match hash"
);
} else {
// The hash slot can only hold a zero if it is uninitialized, meaning no value or delay change was ever
// scheduled. Therefore, the hints must then correspond to uninitialized scheduled changes.
assert_eq(
value_change_hint, ScheduledValueChange::deserialize(zeroed()), "Non-zero value change for zero hash"
);
assert_eq(
delay_change_hint, ScheduledDelayChange::deserialize(zeroed()), "Non-zero delay change for zero hash"
);
};

(value_change_hint, delay_change_hint, historical_block_number)
}

fn hash_scheduled_data(
value_change: ScheduledValueChange<T>,
delay_change: ScheduledDelayChange<INITIAL_DELAY>
Expand Down
44 changes: 0 additions & 44 deletions noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable_private_getter.nr
View file

This file was deleted.

20 changes: 0 additions & 20 deletions noir-projects/noir-contracts/contracts/test_contract/src/main.nr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ contract Test {
use dep::aztec::note::constants::MAX_NOTES_PER_PAGE;
use dep::aztec::keys::getters::get_public_keys;

use dep::aztec::state_vars::{shared_mutable::SharedMutablePrivateGetter};

use dep::aztec::{
context::inputs::private_context_inputs::PrivateContextInputs,
hash::{pedersen_hash, compute_secret_hash, ArgsHasher}, keys::public_keys::IvpkM,
Expand Down Expand Up @@ -485,24 +483,6 @@ contract Test {
constant.value
}

// This function is used in the e2e_state_vars to test the SharedMutablePrivateGetter in isolation
#[private]
fn test_shared_mutable_private_getter(
contract_address_to_read: AztecAddress,
storage_slot_of_shared_mutable: Field
) -> Field {
// It's a bit wonky because we need to know the delay for get_current_value_in_private to work correctly
let test: SharedMutablePrivateGetter<AztecAddress, 5> = SharedMutablePrivateGetter::new(
&mut context,
contract_address_to_read,
storage_slot_of_shared_mutable
);

let ret = test.get_value_in_private(context.get_header());

ret.to_field()
}

#[private]
fn test_nullifier_key_freshness(address: AztecAddress, public_nullifying_key: Point) {
assert_eq(get_public_keys(address).npk_m.inner, public_nullifying_key);
Expand Down
105 changes: 11 additions & 94 deletions yarn-project/end-to-end/src/e2e_state_vars.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { AztecAddress, BatchCall, Fr, type PXE, type Wallet } from '@aztec/aztec.js';
import { AuthContract, DocsExampleContract, TestContract } from '@aztec/noir-contracts.js';
import { BatchCall, Fr, type PXE, type Wallet } from '@aztec/aztec.js';
import { AuthContract, DocsExampleContract } from '@aztec/noir-contracts.js';

import { jest } from '@jest/globals';

Expand Down Expand Up @@ -236,9 +236,8 @@ describe('e2e_state_vars', () => {
});
});

describe('SharedMutablePrivateGetter', () => {
describe('SharedMutable', () => {
let authContract: AuthContract;
let testContract: TestContract;

const delay = async (blocks: number) => {
for (let i = 0; i < blocks; i++) {
Expand All @@ -247,65 +246,14 @@ describe('e2e_state_vars', () => {
};

beforeAll(async () => {
testContract = await TestContract.deploy(wallet).send().deployed();
// We use the auth contract here because has a nice, clear, simple implementation of the Shared Mutable,
// and we will need to read from it to test our private getter.
// We use the auth contract here because has a nice, clear, simple implementation of Shared Mutable
authContract = await AuthContract.deploy(wallet, wallet.getAddress()).send().deployed();
});

it('checks authorized in AuthContract from TestContract with our SharedMutablePrivateGetter before and after a value change', async () => {
// We set the authorized value here, knowing there will be some delay before the value change takes place
await authContract
.withWallet(wallet)
.methods.set_authorized(AztecAddress.fromField(new Fr(6969696969)))
.send()
.wait();

const authorized = await testContract.methods
.test_shared_mutable_private_getter(authContract.address, 2)
.simulate();

// We expect the value to not have been applied yet
expect(AztecAddress.fromBigInt(authorized)).toEqual(AztecAddress.ZERO);

// We wait for the SharedMutable delay
await delay(5);

// We check after the delay, expecting to find the value we set.
const newAuthorized = await testContract.methods
.test_shared_mutable_private_getter(authContract.address, 2)
.simulate();

expect(AztecAddress.fromBigInt(newAuthorized)).toEqual(AztecAddress.fromBigInt(6969696969n));
});

it('checks authorized in AuthContract from TestContract and finds the correctly set max block number', async () => {
const lastBlockNumber = await pxe.getBlockNumber();

const tx = await testContract.methods.test_shared_mutable_private_getter(authContract.address, 2).prove();

const expectedMaxBlockNumber = lastBlockNumber + 5;

expect(tx.data.forRollup!.rollupValidationRequests.maxBlockNumber.isSome).toEqual(true);
expect(tx.data.forRollup!.rollupValidationRequests.maxBlockNumber.value).toEqual(new Fr(expectedMaxBlockNumber));
});

it('checks propagation of max block number in private', async () => {
// Our initial max block number will be 5 because that is the value of INITIAL_DELAY
const expectedInitialMaxBlockNumber = (await pxe.getBlockNumber()) + 5;

// Our SharedMutablePrivateGetter here reads from the SharedMutable authorized storage property in AuthContract
const tx = await testContract.methods.test_shared_mutable_private_getter(authContract.address, 2).prove();

// The validity of our SharedMutable read request is limited to 5 blocks, which is our initial delay.
expect(tx.data.forRollup!.rollupValidationRequests.maxBlockNumber.isSome).toEqual(true);
expect(tx.data.forRollup!.rollupValidationRequests.maxBlockNumber.value).toEqual(
new Fr(expectedInitialMaxBlockNumber),
);

it('sets the max block number property', async () => {
// We change the SharedMutable authorized delay here to 2, this means that a change to the "authorized" value can
// only be applied 2 blocks after it is initiated, and thus read requests on a historical state without an initiated change is
// valid for at least 2 blocks.
// only be applied 2 blocks after it is initiated, and thus read requests on a historical state without an
// initiated change is valid for at least 2 blocks.
await authContract.methods.set_authorized_delay(2).send().wait();

// Note: Because we are decreasing the delay, we must first wait for the full previous delay - 1 (5 -1).
Expand All @@ -314,44 +262,13 @@ describe('e2e_state_vars', () => {
const expectedModifiedMaxBlockNumber = (await pxe.getBlockNumber()) + 2;

// We now call our AuthContract to see if the change in max block number has reflected our delay change
const tx2 = await authContract.methods.get_authorized_in_private().prove();

// The validity of our SharedMutable read request should now be limited to 2 blocks, instead of 5
expect(tx2.data.forRollup!.rollupValidationRequests.maxBlockNumber.isSome).toEqual(true);
expect(tx2.data.forRollup!.rollupValidationRequests.maxBlockNumber.value).toEqual(
new Fr(expectedModifiedMaxBlockNumber),
);
const tx = await authContract.methods.get_authorized_in_private().prove();

// We check for parity between accessing our SharedMutable directly and from our SharedMutablePrivateGetter. The validity assumptions should remain the same.
const tx3 = await testContract.methods.test_shared_mutable_private_getter(authContract.address, 2).prove();

expect(tx3.data.forRollup!.rollupValidationRequests.maxBlockNumber.isSome).toEqual(true);
expect(tx3.data.forRollup!.rollupValidationRequests.maxBlockNumber.value).toEqual(
// The validity of our SharedMutable read request should be limited to 2 blocks
expect(tx.data.forRollup!.rollupValidationRequests.maxBlockNumber.isSome).toEqual(true);
expect(tx.data.forRollup!.rollupValidationRequests.maxBlockNumber.value).toEqual(
new Fr(expectedModifiedMaxBlockNumber),
);

// We now change the SharedMutable authorized delay here to 100, the same assumptions apply here
await authContract.methods.set_authorized_delay(100).send().wait();

// Note: Because we are increasing the delay, we do not have to wait for this change to apply
const expectedLengthenedModifiedMaxBlockNumber = (await pxe.getBlockNumber()) + 100;

// We now call our AuthContract to see if the change in max block number has reflected our delay change
const tx4 = await authContract.methods.get_authorized_in_private().prove();

// The validity of our SharedMutable read request should now be limited to 100 blocks, instead of 2
expect(tx4.data.forRollup!.rollupValidationRequests.maxBlockNumber.isSome).toEqual(true);
expect(tx4.data.forRollup!.rollupValidationRequests.maxBlockNumber.value).toEqual(
new Fr(expectedLengthenedModifiedMaxBlockNumber),
);

// We check for parity between accessing our SharedMutable directly and from our SharedMutablePrivateGetter. The validity assumptions should remain the same.
const tx5 = await testContract.methods.test_shared_mutable_private_getter(authContract.address, 2).prove();

expect(tx5.data.forRollup!.rollupValidationRequests.maxBlockNumber.isSome).toEqual(true);
expect(tx5.data.forRollup!.rollupValidationRequests.maxBlockNumber.value).toEqual(
new Fr(expectedLengthenedModifiedMaxBlockNumber),
);
});
});
});

0 comments on commit 216ef99

Please sign in to comment.