From c8b8cf8de19beee88be4be17ec688e84fa5d7ece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Tue, 24 Sep 2024 19:53:22 +0000 Subject: [PATCH 1/2] Remove SharedMutablePrivateGetter --- docs/docs/migration_notes.md | 7 +- .../aztec/src/state_vars/shared_mutable.nr | 2 - .../shared_mutable/shared_mutable.nr | 79 +++++++------- .../shared_mutable_private_getter.nr | 44 -------- .../contracts/test_contract/src/main.nr | 20 ---- .../end-to-end/src/e2e_state_vars.test.ts | 101 ++---------------- 6 files changed, 53 insertions(+), 200 deletions(-) delete mode 100644 noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable_private_getter.nr diff --git a/docs/docs/migration_notes.md b/docs/docs/migration_notes.md index 3a691c9b33c..dcd57a1c033 100644 --- a/docs/docs/migration_notes.md +++ b/docs/docs/migration_notes.md @@ -6,8 +6,13 @@ keywords: [sandbox, aztec, notes, migration, updating, upgrading] Aztec is in full-speed development. Literally every version breaks compatibility with the previous ones. This page attempts to target errors and difficulties you might encounter when upgrading, and how to resolve them. -## 0.56.0 +## TBD + +### [Aztec.nr] Removed `SharedMutablePrivateGetter` +This state variable was deleted due to it being difficult to use safely. + +## 0.56.0 ### [Aztec.nr] Changes to contract definition diff --git a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr index 618b06a675c..afcfc4795ac 100644 --- a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr +++ b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable.nr @@ -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; diff --git a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr index dfd458c3915..fab02d322ec 100644 --- a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr +++ b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable.nr @@ -81,46 +81,6 @@ impl SharedMutable 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, ScheduledDelayChange, 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 SharedMutable where T: ToField + FromField + Eq { @@ -203,7 +163,7 @@ impl SharedMutable SharedMutable (ScheduledValueChange, ScheduledDelayChange, 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, delay_change: ScheduledDelayChange diff --git a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable_private_getter.nr b/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable_private_getter.nr deleted file mode 100644 index 443fdf10973..00000000000 --- a/noir-projects/aztec-nr/aztec/src/state_vars/shared_mutable/shared_mutable_private_getter.nr +++ /dev/null @@ -1,44 +0,0 @@ -use dep::protocol_types::{traits::{FromField, ToField}, address::AztecAddress, header::Header}; - -use crate::{context::PrivateContext, state_vars::shared_mutable::shared_mutable::SharedMutable}; - -struct SharedMutablePrivateGetter { - context: &mut PrivateContext, - // The contract address of the contract we want to read from - other_contract_address: AztecAddress, - // The storage slot where the SharedMutable is stored on the other contract - storage_slot: Field, -} - -// We have this as a view-only interface to reading Shared Mutables in other contracts. -// Currently the Shared Mutable does not support this. We can adapt SharedMutable at a later date -impl SharedMutablePrivateGetter where T: FromField + ToField + Eq { - pub fn new( - context: &mut PrivateContext, - other_contract_address: AztecAddress, - storage_slot: Field - ) -> Self { - assert(storage_slot != 0, "Storage slot 0 not allowed. Storage slots must start from 1."); - assert(other_contract_address.to_field() != 0, "Other contract address cannot be 0"); - Self { context, other_contract_address, storage_slot } - } - - pub fn get_value_in_private(self, header: Header) -> T { - // We create a dummy SharedMutable state variable so that we can reuse its historical_read_from_public_storage - // method, greatly reducing code duplication. - let dummy: SharedMutable = SharedMutable::new((), self.storage_slot); - let (value_change, delay_change, historical_block_number) = dummy.historical_read_from_public_storage(header, self.other_contract_address); - - let effective_minimum_delay = delay_change.get_effective_minimum_delay_at(historical_block_number); - let block_horizon = value_change.get_block_horizon(historical_block_number, effective_minimum_delay); - - // If our context has the same header as the one we pass in via the parameter, we are trying to read the "current" value - // and thus need to set the tx max block number below. If the context header is not the same as the one we pass in, this means - // we are trying to read a historical value and thus have no constraint on the max block number that this transaction can be included in. - if (self.context.historical_header.global_variables.block_number.eq(header.global_variables.block_number)) { - self.context.set_tx_max_block_number(block_horizon); - } - - value_change.get_current_at(historical_block_number) - } -} diff --git a/noir-projects/noir-contracts/contracts/test_contract/src/main.nr b/noir-projects/noir-contracts/contracts/test_contract/src/main.nr index c5afcfbfaec..23e7a4b8e78 100644 --- a/noir-projects/noir-contracts/contracts/test_contract/src/main.nr +++ b/noir-projects/noir-contracts/contracts/test_contract/src/main.nr @@ -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, @@ -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 = 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); diff --git a/yarn-project/end-to-end/src/e2e_state_vars.test.ts b/yarn-project/end-to-end/src/e2e_state_vars.test.ts index 400a25e9f36..c960683c854 100644 --- a/yarn-project/end-to-end/src/e2e_state_vars.test.ts +++ b/yarn-project/end-to-end/src/e2e_state_vars.test.ts @@ -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++) { @@ -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). @@ -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), - ); }); }); }); From e7fd5b3dda017c819c040fe03e58ec6cff6e040b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Wed, 25 Sep 2024 16:43:40 +0000 Subject: [PATCH 2/2] Remove unused import --- yarn-project/end-to-end/src/e2e_state_vars.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/yarn-project/end-to-end/src/e2e_state_vars.test.ts b/yarn-project/end-to-end/src/e2e_state_vars.test.ts index c960683c854..90d89404194 100644 --- a/yarn-project/end-to-end/src/e2e_state_vars.test.ts +++ b/yarn-project/end-to-end/src/e2e_state_vars.test.ts @@ -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';