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(StateVariables): Extend SharedMutable to work with multi-field values #5491

Open
Tracked by #4761 ...
nventuro opened this issue Mar 27, 2024 · 4 comments
Open
Tracked by #4761 ...
Labels
S-blocked Status: Blocked

Comments

@nventuro
Copy link
Contributor

#5490 introduced a version of SharedMutable<T> where T is required to implement FromField, meaning it must fit inside a Field wrapper value. While this is fine for values such as AztecAddress, we'll eventually want to store multi-field structs, and so this constraint will need to be relaxed.

We may implement this by relying solely on Serialize and Deserialize, but are likely going to run into trouble until noir-lang/noir#4633 is fixed, since creating an array out of multiple single-field elements is the core issue there.

@github-project-automation github-project-automation bot moved this to Todo in A3 Mar 27, 2024
@LHerskind LHerskind changed the title Extend SharedMutable to work with multi-field values feat(StateVariables): Extend SharedMutable to work with multi-field values Apr 2, 2024
nventuro added a commit that referenced this issue Apr 10, 2024
(Large) part of
#4761.

This is an initial implementation of `SharedMutableStorage`, with some
limitations. I think those are best worked on in follow-up PRs, once we
have the bones working.

The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a
pure Noir struct that has all of the block number related logic.
`SharedMutable` then makes a state variable out of that struct, adding
public storage access both in public and private (via historical reads -
see #5379), and using the new `request_max_block_number` function (from
#5251).

I made an effort to test as much as I could of these in Noir, with
partial success in the case of `SharedMutable` due to lack of certain
features, notably noir-lang/noir#4652. There
is also an end-to-end test that goes through two scheuled value changes,
showing that scheduled values do not affect the current one.

I added some inline docs but didn't include proper docsite pages yet so
that we can discuss the implementation, API, etc., and make e.g.
renamings less troublesome.

### Notable implementation details

I chose to make the delay a type parameter instead of a value mostly
because of two reasons:
- it lets us nicely serialize and deserialize `ScheduledValueChange`
without including this field (which we are not currently interested in
storing)
- it lets us declare a state variable of type `SharedMutable<T, DELAY>`
without having to change the signature of the `new` function, which is
automatically injected by the macro.

Overall I think this is fine, especially since we may later make the
delay mutable (see below), but still worth noting.

Additionally, I created a simple `public_storage` module to get slightly
nicer API and encapsulation. This highlighted a Noir issue
(noir-lang/noir#4633), which currently only
affects public historical reads but will also affect current reads once
we migrate to using the AVM opcodes.

### Future work

- #5491
- #5492 (this
takes care of padding during storage slot allocation)
- #5501
- #5493

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Apr 11, 2024
(Large) part of
AztecProtocol/aztec-packages#4761.

This is an initial implementation of `SharedMutableStorage`, with some
limitations. I think those are best worked on in follow-up PRs, once we
have the bones working.

The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a
pure Noir struct that has all of the block number related logic.
`SharedMutable` then makes a state variable out of that struct, adding
public storage access both in public and private (via historical reads -
see #5379), and using the new `request_max_block_number` function (from
#5251).

I made an effort to test as much as I could of these in Noir, with
partial success in the case of `SharedMutable` due to lack of certain
features, notably noir-lang/noir#4652. There
is also an end-to-end test that goes through two scheuled value changes,
showing that scheduled values do not affect the current one.

I added some inline docs but didn't include proper docsite pages yet so
that we can discuss the implementation, API, etc., and make e.g.
renamings less troublesome.

### Notable implementation details

I chose to make the delay a type parameter instead of a value mostly
because of two reasons:
- it lets us nicely serialize and deserialize `ScheduledValueChange`
without including this field (which we are not currently interested in
storing)
- it lets us declare a state variable of type `SharedMutable<T, DELAY>`
without having to change the signature of the `new` function, which is
automatically injected by the macro.

Overall I think this is fine, especially since we may later make the
delay mutable (see below), but still worth noting.

Additionally, I created a simple `public_storage` module to get slightly
nicer API and encapsulation. This highlighted a Noir issue
(noir-lang/noir#4633), which currently only
affects public historical reads but will also affect current reads once
we migrate to using the AVM opcodes.

### Future work

- AztecProtocol/aztec-packages#5491
- AztecProtocol/aztec-packages#5492 (this
takes care of padding during storage slot allocation)
- AztecProtocol/aztec-packages#5501
- AztecProtocol/aztec-packages#5493

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
@nventuro nventuro added the S-blocked Status: Blocked label Apr 12, 2024
@nventuro
Copy link
Contributor Author

This will likely result in ScheduledValueChange requiring that T implements the serde traits (instead of to/from field).

LHerskind added a commit that referenced this issue Jul 1, 2024
This PR changes public write functions so that they not only store the
new delay and/or value, but also a hash of the combined serialization of
them. This lets us produce smaller proofs in private, since we only need
to prove inclusion of the hash and not each of the individual 4 values.
This will result in even larger savings once we do #5491.

The only annoying bit is that producing the unconstrained hint so that
we can contrain the hash and prove inclusion involves performing reads,
and hence computation of storage slots. But because we cannot pass
mutable references to unconstrained functions, we cannot have methods
for the `&mut PrivateContext` impl, resulting in a bit of a hack to
create a dummy state variable that I can call methods on. This is all
going to go away regardless once #5492 is done.

I also restored the tests deleted in #6985, updating the existing ones
and adding new cases.

---------

Co-authored-by: LHerskind <lasse.herskind@gmail.com>
@LHerskind
Copy link
Contributor

Napkin math of saving on the Token::transfer function with this.

We need to lookup 2 users keys.

One lookup should cost ~17K
Currently a lookup of a key is 27K, but we are really doing two lookups of 10k + 6.5K for the case where it is in the address).

The cost should therefore be ~34K.

What is our current cost:

  • We lookup npk, ovpk, ivpk for the sender
  • We lookup npk, ivpk for the recipient

Meaning that we have 5 lookups of 27K -> ~135K spent on key rotation.

We should save 101K constraints by having this in.

@iAmMichaelConnor, not sure where you would ideally have some of these numbers 🤷

@iAmMichaelConnor
Copy link
Contributor

Can a sharedMutable be hashed into a single field?

I.e. store the following in a single storage slot:

  • Current values of:
    • Ivpk.x
    • Ovpk.x
    • Npk.x
    • Encoding of their y-coord signs
  • Scheduled changed values of them
  • Time of change

(I forget the naming and mechanics of how sharedMutable has been implemented, but hopefully you get the gist).

If that's all stored in a single slot, for a particular user address, then to read that slot from the archive tree, it's ~80 constraints for poseidon2 of 2 fields * 40 height = 3200 constraints.
To unpack all that data listed above, something like a poseidon2 of 10 fields - maybe ~400 constraints.
To unpack the signs, a few constraints.
To recompute the y-coords: 4 or 5 constraints per point. Although there might need to be a range check, to check the 'negative-ness', in which case all this messing around with signs might be unnecessary and similar in cost to poseidon-hashing the y-coords directly into the slot's hash value. Meh, for an estimate, let's say the coordinates aren't compressed, then instead of ~400 constraints, it's more like a poseidon2 of 16 fields - maybe ~640 constraints.
So 3200 + 640 = 4000 constraints to read the keys from the registry for 1 address.
8000 for two addresses.
And if we're in the case where the address isn't-yet registered in the registry, we should be re-purposing the poseidon hashes that were used for unpacking above. I.e. we read the registry, find that the value is 0 (or maybe some known hash of 0's if we're reading a hash of some empty shared mutable slot), and then pass the address's preimage data to the existing poseidon hashes. So this case should add approx 0 extra gates.

So I reckon 8000 to read all keys for sender & recipient.

@LHerskind
Copy link
Contributor

LHerskind commented Jul 2, 2024

Can a sharedMutable be hashed into a single field?

Yes, that is done as part of #7169. So we have the logic. But you still need the values to be there such that you can actually read them and open (using oracles). This issue is just to fix the latter, but is blocked by noir atm.

superstar0402 added a commit to superstar0402/aztec-nr that referenced this issue Aug 16, 2024
(Large) part of
AztecProtocol/aztec-packages#4761.

This is an initial implementation of `SharedMutableStorage`, with some
limitations. I think those are best worked on in follow-up PRs, once we
have the bones working.

The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a
pure Noir struct that has all of the block number related logic.
`SharedMutable` then makes a state variable out of that struct, adding
public storage access both in public and private (via historical reads -
see #5379), and using the new `request_max_block_number` function (from
#5251).

I made an effort to test as much as I could of these in Noir, with
partial success in the case of `SharedMutable` due to lack of certain
features, notably noir-lang/noir#4652. There
is also an end-to-end test that goes through two scheuled value changes,
showing that scheduled values do not affect the current one.

I added some inline docs but didn't include proper docsite pages yet so
that we can discuss the implementation, API, etc., and make e.g.
renamings less troublesome.

### Notable implementation details

I chose to make the delay a type parameter instead of a value mostly
because of two reasons:
- it lets us nicely serialize and deserialize `ScheduledValueChange`
without including this field (which we are not currently interested in
storing)
- it lets us declare a state variable of type `SharedMutable<T, DELAY>`
without having to change the signature of the `new` function, which is
automatically injected by the macro.

Overall I think this is fine, especially since we may later make the
delay mutable (see below), but still worth noting.

Additionally, I created a simple `public_storage` module to get slightly
nicer API and encapsulation. This highlighted a Noir issue
(noir-lang/noir#4633), which currently only
affects public historical reads but will also affect current reads once
we migrate to using the AVM opcodes.

### Future work

- AztecProtocol/aztec-packages#5491
- AztecProtocol/aztec-packages#5492 (this
takes care of padding during storage slot allocation)
- AztecProtocol/aztec-packages#5501
- AztecProtocol/aztec-packages#5493

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked
Projects
Status: Todo
Development

No branches or pull requests

3 participants