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 CallStack::Synthesize to produce consistent dummy outputs for faster nested import deployments #2598

Open
wants to merge 8 commits into
base: staging
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 99 additions & 6 deletions synthesizer/process/src/stack/call/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::{CallStack, Registers, RegistersCall, StackEvaluate, StackExecute, stack::Address};
use crate::{CallStack, Registers, RegistersCall, Stack, StackEvaluate, StackExecute, stack::Address};
use aleo_std::prelude::{finish, lap, timer};
use console::{
account::Field,
Expand Down Expand Up @@ -45,7 +45,7 @@ pub trait CallTrait<N: Network> {
/// Executes the instruction.
fn execute<A: circuit::Aleo<Network = N>, R: CryptoRng + Rng>(
&self,
stack: &(impl StackEvaluate<N> + StackExecute<N> + StackMatches<N> + StackProgram<N>),
stack: &Stack<N>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the interface? Can we keep it the way it was? Same for the other function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed here: 774efae

registers: &mut (
impl RegistersCall<N>
+ RegistersSigner<N>
Expand Down Expand Up @@ -138,7 +138,7 @@ impl<N: Network> CallTrait<N> for Call<N> {
#[inline]
fn execute<A: circuit::Aleo<Network = N>, R: Rng + CryptoRng>(
&self,
stack: &(impl StackEvaluate<N> + StackExecute<N> + StackMatches<N> + StackProgram<N>),
stack: &Stack<N>,
registers: &mut (
impl RegistersCall<N>
+ RegistersSigner<N>
Expand Down Expand Up @@ -225,11 +225,12 @@ impl<N: Network> CallTrait<N> for Call<N> {

// Set the (console) caller.
let console_caller = Some(*stack.program_id());
// Check if the substack has a proving key or not.
let pk_missing = !substack.has_proving_key(function.name());

match registers.call_stack() {
// If the circuit is in authorize or synthesize mode, then add any external calls to the stack.
CallStack::Authorize(_, private_key, authorization)
| CallStack::Synthesize(_, private_key, authorization) => {
// If the circuit is in authorize mode, then add any external calls to the stack.
CallStack::Authorize(_, private_key, authorization) => {
// Compute the request.
let request = Request::sign(
&private_key,
Expand All @@ -256,6 +257,98 @@ impl<N: Network> CallTrait<N> for Call<N> {
// Return the request and response.
(request, response)
}
// If the proving key is missing, build real sub-circuit.
CallStack::Synthesize(_, private_key, ..) if pk_missing => {
// Compute the request.
let request = Request::sign(
&private_key,
*substack.program_id(),
*function.name(),
inputs.iter(),
&function.input_types(),
root_tvk,
false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use false?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed here: d7070d9

rng,
)?;

// Retrieve the call stack.
let mut call_stack = registers.call_stack();

// Push the request onto the call stack.
call_stack.push(request.clone())?;

// Execute the request.
let response = substack.execute_function::<A, R>(call_stack, console_caller, root_tvk, rng)?;

// Return the request and response.
(request, response)
}
CallStack::Synthesize(_, private_key, ..) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing comment like you added to the other synthesize and authorize cases

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any difference to the contents of CallStack::Synthesize and CallStack::CheckDeployment? Perhaps they can be merged into the same case.

Copy link
Author

@kpandl kpandl Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed both here: 815a4df
Synthesize and CheckDeployment are indeed the same!

// Compute the request.
let request = Request::sign(
&private_key,
*substack.program_id(),
*function.name(),
inputs.iter(),
&function.input_types(),
root_tvk,
false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use false?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed here: d7070d9

rng,
)?;

// Compute the address.
let address = Address::try_from(&private_key)?;

// For each output, if it's a record, compute the randomizer and nonce.
let outputs = function
.outputs()
.iter()
.map(|output| match output.value_type() {
ValueType::Record(record_name) => {
let index = match output.operand() {
Operand::Register(Register::Locator(index)) => Field::from_u64(*index),
_ => bail!("Expected a `Register::Locator` operand for a record output."),
};
// Compute the randomizer.
let randomizer = N::hash_to_scalar_psd2(&[*request.tvk(), index])?;
// Construct the record nonce from that randomizer.
let record_nonce = N::g_scalar_multiply(&randomizer);
// Sample the record with that nonce.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be made a part of sample_record, so that callers of sample_record don't have to think about it anymore and all are guaranteed correct usage?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a new function sample_record_using_tvk here: 0503e5c

Ok(Value::Record(substack.sample_record(
&address,
record_name,
record_nonce,
rng,
)?))
}
_ => substack.sample_value(&address, output.value_type(), rng),
})
.collect::<Result<Vec<_>>>()?;

// Construct the dummy response from these outputs.
let output_registers = function
.outputs()
.iter()
.map(|output| match output.operand() {
Operand::Register(register) => Some(register.clone()),
_ => None,
})
.collect::<Vec<_>>();

let response = crate::Response::new(
request.network_id(),
substack.program().id(),
function.name(),
request.inputs().len(),
request.tvk(),
request.tcm(),
outputs,
&function.output_types(),
&output_registers,
)?;

(request, response)
}
CallStack::PackageRun(_, private_key, ..) => {
// Compute the request.
let request = Request::sign(
Expand Down
6 changes: 6 additions & 0 deletions synthesizer/process/src/stack/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,12 @@ impl<N: Network> StackProgram<N> for Stack<N> {
.ok_or_else(|| anyhow!("Function '{function_name}' does not exist"))
}

/// Returns true if the proving key for the given function name already exists in this stack.
#[inline]
fn has_proving_key(&self, function_name: &Identifier<N>) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this extra wrapper?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we don't want to have this wrapper, I need to revert 774efae.
I guess the cleanest solution is if I go ahead and to that, so we can directly call contains_proving_key?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. Hmm I don't know the reasoning behind the choice of StackProgram<N> trait existing. But let's try to keep that unchanged.

One approach which could be somewhat consistent, is to create a new trait StackKeys. And these particular functions (contains/get/insert proving/verifying keys) could impl<N> StackKeys<N>.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed here: 6ebe642

self.contains_proving_key(function_name)
}

/// Returns a value for the given value type.
fn sample_value<R: Rng + CryptoRng>(
&self,
Expand Down
3 changes: 3 additions & 0 deletions synthesizer/program/src/traits/stack_and_registers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ pub trait StackProgram<N: Network> {
/// Returns the expected number of calls for the given function name.
fn get_number_of_calls(&self, function_name: &Identifier<N>) -> Result<usize>;

/// Returns true if the proving key for the given function exists.
fn has_proving_key(&self, function_name: &Identifier<N>) -> bool;

/// Samples a value for the given value_type.
fn sample_value<R: Rng + CryptoRng>(
&self,
Expand Down