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

Switch old key registry API to the new one #7953

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

Switch old key registry API to the new one #7953

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

Comments

@nventuro
Copy link
Contributor

Keys are currently fetched using the KeyGetters trait, which is subpar because a) it muddles concerns by making fetching keys be tied to a header, and b) it only fetches one key at a time. #7523 introduces a new registry with the capacity to read all keys at once, as well as a new leaner API that can be used to leverage this, but maintains the old API and keeps callsites intact to reduce disruption.

We should remove KeyGetters and change usage for get_current_public_keys or get_historical_public_keys were applicable (in most cases it'll be current, except odd scenarios such as the voting contract).

@github-project-automation github-project-automation bot moved this to Todo in A3 Aug 13, 2024
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>
@github-project-automation github-project-automation bot moved this from Todo to Done in A3 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.

1 participant