Fix check_deployment disagreement due to non-deterministic signer - #3346
Fix check_deployment disagreement due to non-deterministic signer#3346Antonio95 wants to merge 11 commits into
check_deployment disagreement due to non-deterministic signer#3346Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a consensus-risky nondeterminism in deployment checking by making the “dummy” signer used during CheckDeployment circuit synthesis deterministic (derived from a seeded RNG), preventing validator disagreements when call.dynamic targets depend on self.signer.
Changes:
- Make the burner private key (and thus
self.signer) deterministic during deployment verification by sampling it from the deployment-ID-seeded RNG. - Update
resolve_dynamic_targetdocumentation to reflect current behavior inSynthesize/CheckDeploymentmodes. - Add a V19 regression test covering dynamic target resolution involving closures, and update several related comments for clarity.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| synthesizer/process/src/stack/deploy.rs | Samples burner private key from a deployment-ID-seeded RNG to make CheckDeployment deterministic. |
| synthesizer/process/src/stack/call/dynamic.rs | Updates documentation for resolve_dynamic_target return behavior (esp. closure cases). |
| synthesizer/src/vm/tests/test_v19/closure_dynamic_targets.rs | Adds regression test for deterministic verify_deployment behavior with closure-vs-function dynamic targets. |
| synthesizer/src/vm/tests/test_v19/mod.rs | Registers the new V19 test module. |
| synthesizer/src/vm/tests/test_v18/mod.rs | Comment wording/line-wrapping cleanup. |
| synthesizer/process/src/stack/helpers/synthesize.rs | Comment clarifying is_root = true semantics during synthesis. |
| synthesizer/process/benches/check_deployment.rs | Comment clarifying is_root = true semantics in benchmark setup. |
| synthesizer/process/src/tests/test_credits.rs | Comment clarifying is_root is set (not sampled). |
| circuit/program/src/request/verify.rs | Comment clarifying is_root = true semantics in circuit-side request verification tests. |
| // Deploys a program whose function points the target of a call.dynamic instruction to a closure or a | ||
| // function depending on the last bit of self.signer. The test checks several runs of test- |
| for (function, (_, (verifying_key, _))) in | ||
| deployment.program().functions().values().zip_eq(deployment.function_verifying_keys()) | ||
| { | ||
| // Initialize a burner private key. | ||
| let burner_private_key = PrivateKey::new(rng)?; | ||
| let burner_private_key = PrivateKey::new(&mut seeded_rng)?; |
| for i in 0..N_EXPERIMENTS { | ||
| let deployment_attempt = process.deploy::<CurrentAleo, _>(&parse_program(i), rng); | ||
|
|
||
| match deployment_attempt { | ||
| Ok(deployment) => { | ||
| println!(" - Deployment computation {i} succeeded with ID {}", deployment.to_deployment_id().unwrap()); | ||
|
|
||
| // Ensure that different runs of verify_deployment agree on the result - even if it is a rejection. | ||
| let verification_successful = | ||
| process.verify_deployment::<CurrentAleo, _>(ConsensusVersion::V19, &deployment, rng).is_ok(); | ||
| for _ in 1..N_EXPERIMENTS { | ||
| assert_eq!( | ||
| verification_successful, | ||
| process.verify_deployment::<CurrentAleo, _>(ConsensusVersion::V19, &deployment, rng).is_ok(), | ||
| "Verifier disagreement during deployment verification", | ||
| ); | ||
| } | ||
| println!(" All verifiers {}", if verification_successful { "accepted" } else { "rejected" }); | ||
| } | ||
| Err(error) => { | ||
| println!(" - Deployment computation {i} failed: {error}"); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The PR description and the documentation of the test makes this very clear. The only guaranteed property being tested is that all verifiers agree on the decision - not that at least one of them accepts (which would in fact defeat the purpose of this PR other verifiers did not).
I'd like to keep the I recall at some point there was a similar false-negative (incorrectly rejecting) case for standard deployments as well. Does Fix 2 address that as well? |
This PR fixes a problematic interaction between an issue in
resolve_dynamic_targetand non-deterministic sampling of asignerin someCallStackmodes.Issue
The function
resolve_dynamic_target, called insynthesizer/process/src/stack/call/dynamic.rs::{execute, evaluate}, returns aResult<Option<ResolvedTarget>>. In the twoCallStackmodes it refers to as "dummy", i.e.SynthesizeandCheckDeployment, it returnsOk<None>in some cases andErrin others (specifically,Erris returned when the target can be resolved but it points to a closure). The correct thing to do would be to returnOk<None>in all cases for those two modes, since in those no dynamic calls are actually followed through and executed (one is synthesising a single function's circuit, and closures cannot be called viacall.dynamic). This is indeed the wayresolve_dynamic_targetwas documented - but not implemented. The documentation has been updated to match actual behaviour.As can be seen in
execute, the target returned byresolve_dynamic_targetis never used in the dummy modes. However,executealways applies?to theResult<<Option<ResolvedTarget>>>, which meansexecutefails (in a controlled manner) when the register values as they exist during synthesis happen to cause the target to be an existing closure. In particular,verify_deploymentrejects it.So far this would mainly be a usability issue, where technically valid programs could not be deployed. However, one element is not sampled deterministically when starting circuit
SynthesizeandCheckDeploymentmodes: the signer, which is sampled from the OS'srnginstead of the deterministicseeded_rngused for function inputs, etc. This makes easy to construct simple programs wherevm.deployandvm.check_deploymentsucceed or fail randomly depending on the OS's unpredictablerng, leading to a disagreement between validators: simply have the target of acall.dynamicinstruction depend on the signer (for instance, it's last bit), being an existing closure in some cases and a non-existent target (or a function) in others. Cf. the test https://github.com/ProvableHQ/snarkVM/blob/fix/dynamic_target_resolution_closure/synthesizer/src/vm/tests/test_v19/closure_dynamic_targets.rs#L21.Fix
Two options present themselves:
seeded_rnginstead ofrng. This means if one validator rejects the deployment, all do. This does not need to be version guarded, since in principle it is switching from "machine-dependent random, sometimes correct, sometimes not" to "deterministic, correct".resolve_dynamic_targetto always returnOk<None>inSynthesizeandCheckDeploymentmodes, as mentioned above. This was done in this PR (ccfe407) but has since been reverted.Point 2 would more robust and future-proof, as it also guards against other sources of randomness we may have missed (none have been found after an AI-aided search) or which may be introduced in the future. However, the call to
resolve_dynamic_targetlives in an internal part of the stack we do not want to be consensus-version-dependent. For that reason, we implement point 1 only (which is a positive change in itself). As a result, pathological Aleo programs such as the one proposed may either be rejected by all validators, or accepted by all, i.e. the risk of forking is avoided. The fact that such a program (which technically has valid Aleo instructions) could be rejected at random is a minor usability issue given its pathological semantics (a dynamic call can never execute pointing to a closure, so a program definition which allows that in some register state is, at best, questionable).A way to implement point 2 and circumvent the
ConsensusVersionthreading issue would be to add a boolean flag toCallStack::SynthesizeandCallStack::CheckDepoymentto mark which version ofresolve_dynamic_targetto use (fixed one in the since reverted ccfe407). @raychu86 let me know if you prefer this option (to the current state, i.e. not implementing point 2).Tests
One small test has been added which tests the narrow type of problematic programs:
test_v19::test_conditional_dynamic_call_target_deployment. Details therein.