-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[Fix] Make deployment verification consistent by deterministically seeding the RNG. #2535
[Fix] Make deployment verification consistent by deterministically seeding the RNG. #2535
Conversation
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test rev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend to not use the rng argument at all within verify_deployment. The existing assumptions which this PR makes also applies to the burner_private_key: its value is not important and if god forbid there's a bug in the key generation code then we'd rather want all validators to abort equally.
That being said, this recommendation is not essential, so if we're in a time crunch, we can also leave it for later. In a future PR, we can also consider removing the rng argument in check_next_block entirely to prevent future developers from introducing non-determinism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🫡
7e8005c
Added a tweak to minimize change-set and clean up the testing; the logic should be equivalent. Unfortunately this dismissed the previous reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR:
Failing Case
This issue was found in a deployed program with a similar structure to:
At a high-level, deployments are verified by randomly generating inputs, executing the program, extracting the assignment, and checking that the associated certificates match the structure of the assignment. Note that the actual values do not matter. Source
The core issue is that RNG used in generating random inputs can differ across different runs of verify_deployment.
The failing case manifests in the following way:
cast r0 into r1 as scalar;
creates an invalid scalar field element whenr0
is larger than the scalar field modulus.r1
.Consequently, a deployment of the above program can fail to verify depending on the random inputs that were used.
Solution
To fix this issue, this PR deterministically seeds the RNG used in deployment verification with the lower 64 bits of the deployment ID. This ensures that validators will use the same seed for a particular deployment, which ensures that they will always agree on whether the deployment is valid or not.
Testing
This PR includes a test that attempts to deploy a program that loads and stores data types with invalid scalar field elements. The test verifies that the status of the deployment is consistent: always valid or always invalid, independent of the RNG.
CI for this branch is running here.
Considerations
In addition to this PR, additional mitigations can be put in place to ensure that:
Scalar::eject_value
does not halteject_value
. This is dependent on whether or not the invocations ofeject_value
are intended to serve as a value check or a less strict type check.Related PRs
Use this PR over #2534