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

Refactor APIs to receive keys instead of reading them #7954

Closed
nventuro opened this issue Aug 13, 2024 · 0 comments · Fixed by #8043
Closed

Refactor APIs to receive keys instead of reading them #7954

nventuro opened this issue Aug 13, 2024 · 0 comments · Fixed by #8043
Assignees

Comments

@nventuro
Copy link
Contributor

Once #7953 is implemented, we'll be reading all keys at once but discarding most results. We should change most APIs so that they can receive keys instead of reading them on demand, as that allows for reuse of the historical read proof, which is quite expensive. A simple example is BalancesMap::add, which fetches the npk instead of receiving it.

Perhaps in the future we might think of some global cache in which we can store expensive results to later reference them, but we'd need to at the very least settle on having key rotation before we explore such a thing.

nventuro added a commit that referenced this issue Aug 14, 2024
This is a tentative redesign of the key registry contract, which lets us
read all 4 keys in a single merkle inclusion proof, while also
performing fewer public storage writes. It relies on `PublicMutable`
with custom deadlines instead of `SharedMutable` as the key registry
must be lax during reads to prevent abuse from accounts performing
frequent rotation.

This is meant to be a proof of concept and reference for discussion, so
there are no tests yet. If we were to adopt this contract it should be
relatively straightforward to replace the current getters with
`get_current_public_keys`.

---

Update: we've now decided to move forward with this design, even if
whether we actually do have full key rotation is up for debate. This is
a good middle ground in terms of the code being performant and easy to
either add or remove rotation in the future.

What this PR does is introduce the new registry and switch all old
contracts to use it. They'll be using historical mode, i.e. reading keys
at a specific block in the past instead of reading the current keys,
resulting in no max block number constraints. This is a departure from
the current behavior, but is only temporary until we switch over from
the old API to the new one
(#7953) so that we
can then benefit from reduce gate counts
(#7954). I've also
left the original contract, libraries, etc., unmodified so as to later
delete them cleanly instead of making this PR too messy
(#7955).

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Aug 15, 2024
This is a tentative redesign of the key registry contract, which lets us
read all 4 keys in a single merkle inclusion proof, while also
performing fewer public storage writes. It relies on `PublicMutable`
with custom deadlines instead of `SharedMutable` as the key registry
must be lax during reads to prevent abuse from accounts performing
frequent rotation.

This is meant to be a proof of concept and reference for discussion, so
there are no tests yet. If we were to adopt this contract it should be
relatively straightforward to replace the current getters with
`get_current_public_keys`.

---

Update: we've now decided to move forward with this design, even if
whether we actually do have full key rotation is up for debate. This is
a good middle ground in terms of the code being performant and easy to
either add or remove rotation in the future.

What this PR does is introduce the new registry and switch all old
contracts to use it. They'll be using historical mode, i.e. reading keys
at a specific block in the past instead of reading the current keys,
resulting in no max block number constraints. This is a departure from
the current behavior, but is only temporary until we switch over from
the old API to the new one
(AztecProtocol/aztec-packages#7953) so that we
can then benefit from reduce gate counts
(AztecProtocol/aztec-packages#7954). I've also
left the original contract, libraries, etc., unmodified so as to later
delete them cleanly instead of making this PR too messy
(AztecProtocol/aztec-packages#7955).

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
@benesjan benesjan self-assigned this Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants