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.

Sign up for GitHub

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

Jump to bottom

[Optimization] Inefficient fetching of credits.aleo PKs #2416

Open
raychu86 opened this issue Apr 1, 2024 · 2 comments · May be fixed by #2510
Open

[Optimization] Inefficient fetching of credits.aleo PKs #2416

raychu86 opened this issue Apr 1, 2024 · 2 comments · May be fixed by #2510
Assignees
@ljedrz
Labels
does not block mainnet For when we make decisions that this will not block mainnet.

Comments

@raychu86
Copy link
Contributor

raychu86 commented Apr 1, 2024

It looks like our VM does not fetch the credits.aleo proving keys from the resources folder and simply resynthesizes them from scratch at time of use. This is completely safe, however will incur a one-time synthesis cost.

When we fetch the proving keys functions via Stack::get_proving_key, we check if the function is a credits.aleo function and read the proving key directly from the bytes in .resources if it is. This is done in Stack::execute - https://github.com/AleoHQ/snarkVM/blob/2cbf34a1010bf781277cdc6ff1ae966230cf97c1/synthesizer/process/src/stack/execute.rs#L462

However a few lines above, we are always re-synthesizing the key at first time of use: https://github.com/AleoHQ/snarkVM/blob/2cbf34a1010bf781277cdc6ff1ae966230cf97c1/synthesizer/process/src/stack/execute.rs#L425-L429 This ignores the optimization we have for the PKs where we can simply read from bytes instead of resynthesizing.

Note: The current behavior does NOT impact security, but can be optimized.
@ljedrz
Copy link
Collaborator

ljedrz commented Apr 3, 2024

Would the following tweak suffice?

diff --git a/synthesizer/process/src/stack/execute.rs b/synthesizer/process/src/stack/execute.rs
index 4a2936d91..3c4331c79 100644
--- a/synthesizer/process/src/stack/execute.rs
+++ b/synthesizer/process/src/stack/execute.rs
@@ -421,8 +421,9 @@ impl<N: Network> StackExecute<N> for Stack<N> {
         if matches!(registers.call_stack(), CallStack::Synthesize(..))
             || matches!(registers.call_stack(), CallStack::Execute(..))
         {
-            // If the proving key does not exist, then synthesize it.
-            if !self.contains_proving_key(function.name()) {
+            // If the proving key does not exist, then synthesize it. This is not needed for `credits.aleo`.
+            if self.program_id() != &ProgramID::from_str("credits.aleo")? && !self.contains_proving_key(function.name())
+            {
                 // Add the circuit key to the mapping.
                 self.synthesize_from_assignment(function.name(), &assignment)?;
                 lap!(timer, "Synthesize the {} circuit key", function.name());

@damons damons added the does not block mainnet For when we make decisions that this will not block mainnet. label Jun 11, 2024
@raychu86
Copy link
Contributor Author

raychu86 commented Jul 1, 2024

@ljedrz That should be sufficient. Could you open up a PR with this change so we can test it?

@ljedrz ljedrz linked a pull request Jul 1, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ljedrz ljedrz

Labels
does not block mainnet For when we make decisions that this will not block mainnet.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Perf] Don't re-synthesize credits.aleo ljedrz/snarkVM
3 participants
@ljedrz @damons @raychu86