-
Notifications
You must be signed in to change notification settings - Fork 234
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: fix contract issues with key rotation, introduce get npk_m_hash_at #6541
feat: fix contract issues with key rotation, introduce get npk_m_hash_at #6541
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
06517a5
to
3d81929
Compare
Benchmark resultsNo base data found for comparison. Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Proof generationEach column represents the number of threads used in proof generation.
L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every kernel circuit run across all benchmarks.
Stats on running time collected for app circuits
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method | Metric | | Transaction processing duration by data writes.
|
@@ -6,7 +6,7 @@ use dep::std::merkle::compute_merkle_root; | |||
|
|||
use crate::{context::PrivateContext, oracle::get_public_data_witness::get_public_data_witness}; | |||
|
|||
fn _public_storage_historical_read(storage_slot: Field, contract_address: AztecAddress, header: Header) -> Field { | |||
pub fn _public_storage_historical_read(storage_slot: Field, contract_address: AztecAddress, header: Header) -> Field { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've exposed this to access this by passing in a header directly. One can make an argument that to keep it consistent with all the other changes I've made, that actually the public_storage_historical_read_at
function below should change and take a header as well, and not only a block number; but I wanted to keep changes minimal. Open to opinions though !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think exposing this is totally fine but would rename it to something without the underscore prefix since that evokes private/internal function.
@@ -41,7 +61,23 @@ fn get_master_key( | |||
address: AztecAddress, | |||
key_index: Field | |||
) -> GrumpkinPoint { | |||
let key = fetch_key_from_registry(context, key_index, address); | |||
let (x_coordinate_registry, y_coordinate_registry) = get_registry_with_key_index(context, key_index, address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reorganized some things here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The split now made here such that you can use the _at
, right?
See comment above, I think we can make some simplifications to get rid of some of all the _at
case we have which make the code harder to read at the moment.
y_coordinate_registry: SharedMutablePrivateGetter<Field, DELAY>, | ||
header: Header | ||
) -> GrumpkinPoint { | ||
let key_x_coordinate = x_coordinate_registry.get_current_value_in_private_at(header.global_variables.block_number as u32, header); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using get_current_value_in_private_at
as opposed to get_value_in_private_at
just for consistency, also to disambiguate between scheduled / current.
) -> (ScheduledValueChange<T>, ScheduledDelayChange<INITIAL_DELAY>, u32) where T: FromField { | ||
let value_change_slot = self.get_value_change_storage_slot(); | ||
let mut raw_value_change_fields = [0; 3]; | ||
for i in 0..3 { | ||
raw_value_change_fields[i] = public_storage_historical_read( | ||
context, | ||
raw_value_change_fields[i] = _public_storage_historical_read( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this change because otherwise every time we wanted to read a historical value, we'd have to refetch the header :/. See this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not head over heals about the _at
we got, suggested an idea earlier in this and in the team channel, we might want to deal with that first to not mess with these more than necessary.
); | ||
} | ||
|
||
let delay_change_slot = self.get_delay_change_storage_slot(); | ||
let raw_delay_change_fields = [public_storage_historical_read(context, delay_change_slot, context.this_address())]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing context.this_address()
as it seems to be a mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird that this was not caught by a test. I see that it's used in the test contract to read from key registry so it's not the case that it was not caught because it was used to read from the same contract in a test.
Given that it's reading delay I would say we never test the case when a delay is non-default. Could you create a separate PR based on master in which you:
- Create a test which fails because of this issue and submit it in 1 commit (this way we verify that the test is good),
- fix the issue in another commit (this way we verify that it was addressed).
Thanks
@@ -59,18 +59,11 @@ contract InclusionProofs { | |||
block_number: u32, // The block at which we'll prove that the note exists | |||
nullified: bool | |||
) { | |||
let owner_npk_m_hash = get_npk_m_hash(&mut context, owner); | |||
|
|||
// TODO (#6312): This will break with key rotation. Fix this. Will not be able to find any notes after rotating key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed as private_values is scoped.
@@ -17,18 +21,30 @@ describe('e2e_voting_contract', () => { | |||
({ teardown, wallet, logger } = await setup(1)); | |||
owner = wallet.getAddress(); | |||
|
|||
testContract = await TestContract.deploy(wallet).send().deployed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this for access to cross delay. Not the best, but just reusing the pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably get the cheatcode going #3168
new_value | ||
} | ||
|
||
#[aztec(private)] | ||
fn private_get_value(amount: Field, owner: AztecAddress) -> Field { | ||
let owner_npk_m_hash = get_npk_m_hash(&mut context, owner); | ||
|
||
// TODO (#6312): This will break with key rotation. Fix this. Will not be able to find any notes after rotating key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer needed because the set is scoped to owner.
@@ -7,17 +7,12 @@ use dep::aztec::note::note_getter_options::{Sort, SortOrder}; | |||
// Shows how to use NoteGetterOptions and query for notes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in this file does not seem to be used anywhere and is for demonstration purposes only. Nothing actually was broken because account_npk_m_hash was passed in directly. I've made changes though because I think in general this pattern of filtering with npk_m_hash is not great.
@@ -27,7 +22,6 @@ pub fn create_exact_card_getter_options( | |||
secret: Field, | |||
account_npk_m_hash: Field | |||
) -> NoteGetterOptions<CardNote, CARD_NOTE_LEN, Field> { | |||
// TODO (#6312): This will break with key rotation. Fix this. Will not be able to find any notes after rotating key. | |||
let mut options = NoteGetterOptions::new(); | |||
options.select(CardNote::properties().points, points as Field, Option::none()).select(CardNote::properties().randomness, secret, Option::none()).select( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept this one as is to show that all fields can be simultaneously filtered on (name of test).
c90cba0
to
385ef25
Compare
@@ -6,7 +6,7 @@ use dep::std::merkle::compute_merkle_root; | |||
|
|||
use crate::{context::PrivateContext, oracle::get_public_data_witness::get_public_data_witness}; | |||
|
|||
fn _public_storage_historical_read(storage_slot: Field, contract_address: AztecAddress, header: Header) -> Field { | |||
pub fn _public_storage_historical_read(storage_slot: Field, contract_address: AztecAddress, header: Header) -> Field { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think exposing this is totally fine but would rename it to something without the underscore prefix since that evokes private/internal function.
@@ -19,8 +19,28 @@ pub fn get_npk_m(context: &mut PrivateContext, address: AztecAddress) -> Grumpki | |||
get_master_key(context, address, NULLIFIER_INDEX) | |||
} | |||
|
|||
// For historical access, we move the fetching of the header to the top level to minimize redundant fetching / proving validity of the header. | |||
pub fn get_npk_m_at( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is only called in get_npk_m_hash_at
so would merge it with that to not have a huge amount of variations of functions.
); | ||
} | ||
|
||
let delay_change_slot = self.get_delay_change_storage_slot(); | ||
let raw_delay_change_fields = [public_storage_historical_read(context, delay_change_slot, context.this_address())]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird that this was not caught by a test. I see that it's used in the test contract to read from key registry so it's not the case that it was not caught because it was used to read from the same contract in a test.
Given that it's reading delay I would say we never test the case when a delay is non-default. Could you create a separate PR based on master in which you:
- Create a test which fails because of this issue and submit it in 1 commit (this way we verify that the test is good),
- fix the issue in another commit (this way we verify that it was addressed).
Thanks
let mut decremented = 0; | ||
for i in 0..opt_notes.len() { | ||
if opt_notes[i].is_some() { | ||
let note = opt_notes[i].unwrap_unchecked(); | ||
|
||
// This is similar to destroy_note, except we only compute the owner_npk_m_hash once instead of doing it in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool!
// TODO (#6312): This will break with key rotation. Fix this. Will not be able to pass this after rotating keys. | ||
assert(note.npk_m_hash.eq(owner_npk_m_hash)); | ||
|
||
pub fn destroy_note(balance: PrivateSet<ValueNote, &mut PrivateContext>, note: ValueNote) -> Field { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cute
@@ -53,11 +53,6 @@ impl EasyPrivateUint { | |||
if maybe_notes[i].is_some() { | |||
let note = maybe_notes[i].unwrap_unchecked(); | |||
|
|||
// Ensure the notes are actually owned by the owner (to prevent user from generating a valid proof while |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we don't have a guarantee that EasyPrivateUInt will only ever be used in maps, no?
I feel like the correct justification for removing this in here is that the notes inside the EasyPrivateUInt is protected by nullifier key and hence can't be nullified by "non-owner".
let options = NoteGetterOptions::with_filter(filter_cards, cards); | ||
let maybe_notes = self.set.get_notes(options); | ||
let mut found_cards = [Option::none(); N]; | ||
for i in 0..maybe_notes.len() { | ||
if maybe_notes[i].is_some() { | ||
let card_note = CardNote::from_note(maybe_notes[i].unwrap_unchecked()); | ||
// Ensure the notes are actually owned by the owner (to prevent user from generating a valid proof while | ||
// spending someone else's notes). | ||
// TODO (#6312): This will break with key rotation. Fix this. Will not be able to pass this assert after rotating keys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again I feel like the main protection here is the nullifier key as relying on Maps for protection just seems scary to me. @LHerskind or WDYT about using Maps as a guardrail?
@@ -54,11 +90,11 @@ fn get_master_key( | |||
} | |||
} | |||
|
|||
fn fetch_key_from_registry( | |||
fn get_registry_with_key_index( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like much that we need to have this getter and then split the flows into fetch_key_from_registry
and fetch_key_from_registry_at
but I don't really now how to refactor it so will shut up for now.
But I think a better name would help here because when I saw the function I though it fetches some value from registry. I think something like instantiate_registry_getter
would be a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: Boss came up with how to refactor it here.
} | ||
// docs:end:constructor | ||
|
||
// docs:start:cast_vote | ||
#[aztec(private)] // annotation to mark function as private and expose private context | ||
fn cast_vote(candidate: Field) { | ||
let msg_sender_npk_m_hash = get_npk_m_hash(&mut context, context.msg_sender()); | ||
// TODO (#6312): This will break with key rotation. Fix this. Can vote multiple times by rotating keys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document how the protection against nullifier key rotation double spend works here. The contracts are used as an example so it's very valuable to share this info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I think the pr is fine, but realised that we can likely handle the _at
cases more cleanly than we are now which seems like something that could be beneficial to deal with before this one goes in since we don't need to add to then instantly remove it afterwards (you can add a new parent to this in the stack for example).
For that reason, I think we need to address #6589 and then merge this pr.
block_number: u32, | ||
header: Header | ||
) -> GrumpkinPoint { | ||
assert_eq(block_number, header.global_variables.block_number as u32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels strange, why do you want to pass the block_number
if it is the same value as we have in the header?
Just a small thing that popped up in my mind. As we seem to generally want to supply multiple ways to execute these functions on the header, should we take a look at having them as a trait on the header?
In that way, you would get the header that you are interested in for data, and then you can use the "getters".
let header = context.get_header_at(block_number);
let npk_m = header.get_npk_m(context, address);
// and for historical access to public state
let storage = header.public_storage_historical_read(address, storage_slot);
@nventuro what are you thoughts? Should allow us to get rid of some code and think it is similar or better ux, but your call 🫡
If to change like this, might be better to handle in separate PR since we have the flow a few places that was otherwise untouched here. Think this could essentially get rid of almost all of the _at
function we got for time, just keep the get_header_at
.
@@ -41,7 +61,23 @@ fn get_master_key( | |||
address: AztecAddress, | |||
key_index: Field | |||
) -> GrumpkinPoint { | |||
let key = fetch_key_from_registry(context, key_index, address); | |||
let (x_coordinate_registry, y_coordinate_registry) = get_registry_with_key_index(context, key_index, address); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The split now made here such that you can use the _at
, right?
See comment above, I think we can make some simplifications to get rid of some of all the _at
case we have which make the code harder to read at the moment.
|
||
GrumpkinPoint::new(x_coordinate, y_coordinate) | ||
(x_coordinate_registry, y_coordinate_registry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording here makes it sound like it is separate registries, but it is just different coordinates so think I would still keep the naming similar to not make misunderstandings that it is a registry 🤷
) -> (ScheduledValueChange<T>, ScheduledDelayChange<INITIAL_DELAY>, u32) where T: FromField { | ||
let value_change_slot = self.get_value_change_storage_slot(); | ||
let mut raw_value_change_fields = [0; 3]; | ||
for i in 0..3 { | ||
raw_value_change_fields[i] = public_storage_historical_read( | ||
context, | ||
raw_value_change_fields[i] = _public_storage_historical_read( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not head over heals about the _at
we got, suggested an idea earlier in this and in the team channel, we might want to deal with that first to not mess with these more than necessary.
let msg_sender_npk_m_hash = get_npk_m_hash(&mut context, context.msg_sender()); | ||
// TODO (#6312): This will break with key rotation. Fix this. Can vote multiple times by rotating keys. | ||
let active_at_block = storage.active_at_block.read_private(); | ||
assert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that you would could end up in this one failing? Namely, to call this function you have that the contract must be deployed and since it must be initialized before letting you call here think we cannot hit it? 🤔
Regardless, I think you will already have this handled from the get_header_at
? 🤔
@@ -291,15 +300,27 @@ describe('e2e_crowdfunding_and_claim', () => { | |||
// Set the value note in a format which can be passed to claim function | |||
const anotherDonationNote = await processExtendedNote(notes![0]); | |||
|
|||
// 3) We claim the reward token via the Claim contract | |||
// We create an unrelated pxe and wallet without access to the nsk_app that correlates to the npk_m specified in the proof note. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat, I'm gonna steal this. 👀
@@ -17,18 +21,30 @@ describe('e2e_voting_contract', () => { | |||
({ teardown, wallet, logger } = await setup(1)); | |||
owner = wallet.getAddress(); | |||
|
|||
testContract = await TestContract.deploy(wallet).send().deployed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably get the cheatcode going #3168
40053c2
to
276b08c
Compare
Have tried a different approach, and have addressed feedback in this stack |
Closed in favor of #6656 |
Resolves #6312