Fix synthesis of circuits with Scalar outputs#3258
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses deployment verification failures caused by ejecting circuit Scalar outputs (notably when casting from Field/Group/Address to Scalar) during Synthesize and CheckDeployment flows. It does so by avoiding response ejection in those modes and adding circuit-native type checking for register values, with regenerated integration-test expectations to reflect RNG consumption changes.
Changes:
- Change
Stack::execute_functionto returnOption<Response>and skip ejecting aResponseinSynthesize/CheckDeployment, sampling dummy outputs downstream where only types are needed. - Replace console-ejection-based type checks in
load_circuit/store_circuitwith a newcircuit_matches_register_typecircuit-native type checker. - Add a regression test for scalar outputs and update
execute_and_finalizeexpectations; adjust the long integration test to avoid duplicate execution.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
vm/package/run.rs |
Adapts execute_function call site to new Option<Response> return type. |
synthesizer/tests/test_vm_execute_and_finalize.rs |
Removes duplicate #[test] attribute and adds per-test logging. |
synthesizer/tests/expectations/vm/execute_and_finalize/user_callable.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/public_wallet.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/program_callable.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/nested_external_struct.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/multi_external_struct.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/many_input_and_output.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/interleave_async_and_non_async.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/future_with_external_struct.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/future_with_external_struct_complex.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/future_out_of_order.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/external_struct.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/external_struct_in_record.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/external_struct_in_external_struct.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/external_struct_in_external_record.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/external_future_arg_access.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/count_usages.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/complex_finalization.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/child_and_parent.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/tests/expectations/vm/execute_and_finalize/branch_with_future.out |
Regenerated expectations due to RNG/outputs changes. |
synthesizer/src/vm/verify.rs |
Cleans up test-module imports. |
synthesizer/src/vm/tests/test_v15/scalar_outputs.rs |
Adds regression test covering scalar outputs from problematic casts. |
synthesizer/src/vm/tests/test_v15/mod.rs |
Registers the new scalar output test module. |
synthesizer/program/src/traits/stack_and_registers.rs |
Adds circuit_matches_register_type to the RegistersCircuit trait. |
synthesizer/process/src/tests/test_execute.rs |
Updates test to handle Option<Response> from execute_function. |
synthesizer/process/src/stack/registers/registers_circuit.rs |
Implements circuit-native type checking to avoid ejection in circuit paths. |
synthesizer/process/src/stack/registers/mod.rs |
Updates imports related to the registers module. |
synthesizer/process/src/stack/execute.rs |
Changes execute_function signature to Result<Option<Response<_>>> and gates response ejection by mode. |
synthesizer/process/src/stack/call/standard.rs |
Ensures Synthesize/CheckDeployment paths operate without a console response and sample dummy outputs when needed. |
synthesizer/process/src/stack/call/dynamic.rs |
Updates dynamic-call execution to unwrap Option<Response> appropriately by mode. |
synthesizer/process/src/execute.rs |
Updates process execution to require a populated response in Execute mode. |
circuit/program/src/data/future/mod.rs |
Adds Future::arguments() accessor used by circuit-side type checking. |
.circleci/config.yml |
Adds a feature branch to the merge-workflow branch filter. |
| use console::{ | ||
| network::prelude::*, | ||
| program::{Entry, Literal, Plaintext, Register, Request, Value}, | ||
| program::{Entry, Literal, Plaintext, PlaintextType, Register, RegisterType, Request, Value}, |
There was a problem hiding this comment.
They are used in the child module registers_circuit.rs, which imports super::*, which is why clippy was happy. I've now moved the two imports to the child module anyway.
Scalar outputs
vicsn
left a comment
There was a problem hiding this comment.
You may have forgotten to commit the new macro
| assert_eq!( | ||
| deployment_transaction_ids, | ||
| vec![deployment_1.id(), deployment_2.id(), deployment_4.id(), deployment_3.id()], | ||
| vec![deployment_4.id(), deployment_1.id(), deployment_3.id(), deployment_2.id()], |
There was a problem hiding this comment.
Can you remove this assert? I've thought for years its useless and creates busywork to just update the expectation
There was a problem hiding this comment.
@vicsn I think part of what that test wants to check is that VM::from imports programs (i.p. depending on one another) correctly regardless of the order vm.transaction_store() returns them in. So, in order for the check assert!(VM::from(vm.store.clone()).is_ok()); in line 1289 to be meaningful, one has to check vm.transaction_store() is not already returning them in the correct import order (1, 2, 3, 4) by chance.
Which of the two following options do you favour?
- Replacing the concrete hardcoded order in the check to the check "make sure
vm.transaction_store()returns them in an order different from 1, 2, 3, 4". This would maintain the meaningfulness of the test and make it so that, on average, if any PR changes things that affect that order (i.p. the random transaction IDs), this test will need to be updated with probability only 1 / 4! = 1/24 (= the chance that the random changes result in the order 1, 2, 3, 4). - Removing this check and living with the risk of the test not being meaningful during some periods in the future. This doesn't seem too bad in the sense that this has been tested over and over, but who knows if future changes could potentially break it (in which case it's likely other tests would break anyway).
Signed-off-by: Antonio Mejías Gil <anmegi.95@gmail.com>
Antonio95
left a comment
There was a problem hiding this comment.
Double checked everything.
|
(All flows pass with the exception of the exact same test cases which are already failing in |
2202627 to
f909e5f
Compare
|
If I understand correctly, this fixes a class of programs that didn't use to deploy and now does. Can you explain further why this doesn't need a consensus version check? Are we worried about a window where some nodes have upgraded and others haven't? |
|
|
||
| // Tests that a function which receives a Field/Group/Address input, casts it to | ||
| // a Scalar and outputs the latter, deploys correctly. | ||
| #[test] |
There was a problem hiding this comment.
A few extra test suggestions:
- Cross-function
callreturning a scalar: function A calls function B where B outputs a scalar (call.dynamictoo). - Nested scalar (struct/array/record entry).
- Negative test that the new
circuit_matches_value_typestill rejects a type-mismatched value. - Test execution here, not just deployment.
| register_type: &RegisterType<N>, | ||
| ) -> Result<()> { | ||
|
|
||
| let value_kind = match value { |
There was a problem hiding this comment.
value_kind is only used in the final bail! arm but it's built on every call. Maybe move its definition into the _ => arm?
| let transition = Transition::from(&console_request, &response, &output_types, &output_registers)?; | ||
| let transition = Transition::from( | ||
| &console_request, | ||
| console_response.as_ref().unwrap(), |
There was a problem hiding this comment.
Maybe justify why this unwrap is safe (an expect maybe)
| let transition = Transition::from(&console_request, &response, &output_types, &output_registers)?; | ||
| let transition = Transition::from( | ||
| &console_request, | ||
| console_response.as_ref().unwrap(), |
There was a problem hiding this comment.
Same here. expect instead of unwrap?
Closes #2801
Note: Before merging, confirm the
mergeCLI flow triggerssynthesizer/tests/test_vm_execute_and_finalize.rs(this PR deletes the#[test]annotation preceding the#[test_log::test], which was causing duplicate execution of that long test).The issue
Aleo programs which
casta value of type one of {Field,Group,Address} toScalarand output the latter often cause errors during deployment verification.The reason is that (unlike a
console::Scalar) the circuit representation of aScalaractually contains aFieldelement, which has ~2 more bits (i.e. takes values up to ~4 times larger) than aScalar. When a function’s circuit is constructed (e.g. during key synthesis and verifying-key verification), the resultingResponseis ejected, which in particular ejects the outputs therein. Since inputs to the function are sampled randomly during synthesis, an input of one of the three types mentioned at the start and cast to aScalarwhich is subsequently output often results in an underlyingFieldvalue which panics atejecttime.Only this case of
castis problematic. For instance, whencastingU16toU8, constraints are added to the circuit to ensure the most significant 8 bits are 0, and only the 8 least significant bits form the newU8value. Constraints are not enforced during key synthesis, and ejecting the 8 bits of theU8always results in validconsole::U8.There are two subtler places where problematic
Scalars were being ejected other than when ejecting the response: in theload_circuitandstore_circuitcalls in the the circuit-construction part of thecastinstruction. These two functions perform a type check of the circuit value received against the destination register’s type. This was done by first ejecting the circuit value into a console one and then callingStackTrait’smatches_value_type(which only accepts console types). This ejection also resulted in an error once the aforementioned one was fixed.The fix
Part of the design philosophy has been not to add any special-case treatment to
Scalartype for improved future maintainability.CheckDeploymentandSynthesizemode, we no longer eject theResponseat the point where we were. Instead of returning that, we now return anOptionwhich is only populated in modes other than the two above, returningNonein those two.CheckDeploymentandSynthesizemode, we sample random outputs. These are the only part of the (no longer present)Responseneeded at that stage, and only their type is relevant.eject+matches_value_typecalls inload_circuitandstore_circuithave been replaced by a newcircuit_matches_value_type. This function performs the same type exploration asmatches_value_typebut acts on circuit values and does not eject. Although this results in some code duplication, it does not result in a performance loss but the opposite: theeject+matches_value_typeflow effectively resulted in two separate explorations of the same type tree (plus any relevant value conversions ineject), as opposed to the single exploration in the newcircuit_matches_value_type(and no value conversions).The test file
synthesizer/src/vm/tests/test_v15/scalar_outputs.rshas been added, which tests key synthesis and verification in functions with previously failing casts. This has been checked on the three problematic source types, both for private and public inputs and outputs, as well as in closures.Note this PR does not require a new
ConsensusVersion. Expectations oftest_vm_execute_and_finalizehad to be regenerated since the sampling of random request outputs (where before aRequestwas being ejected instead) desynchronised the state of therngwith respect to pre-change executions and resulted in outputs different than expected. The expectation regeneration was inspected to be correct (and not, e.g., overwrite previously-expected-to-fail cases with passing ones or vice-versa).