-
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] Selection of transition ID in finalize. #2596
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ljedrz
reviewed
Jan 16, 2025
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 engineering-wise
vicsn
approved these changes
Jan 16, 2025
raychu86
approved these changes
Jan 16, 2025
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The motivating issue for this PR is here.
The original PR is here.
From the original discussion:
This PR proposes a solution to this issue by introducing a nonce to
FinalizeRegisters
. This nonce is used to seed therand
commands. Furthermore, instead of attempting to determine thechild_transition_id
that corresponds to an awaitedFuture
from the call graph, this approach uses the main transition ID to initialize allFinalizeRegisters
for a given transaction. The main transition ID along with the nonce ensures that each finalize context uses a unique seed for the RNG, while removing the need to correctly determine the transition ID for a givenFuture
(a complicated process).Migration
This proposal has been written to migrate at
N::CONSENSUS_V2_HEIGHT
.Given timelines, it's more likely that a new
N::CONSENSUS_V3_HEIGHT
would need to be introduced. The migration would follow the process introduced in ARC-0042.Test Plan
This PR includes a test, whose expected output demonstrates the the failure and the fix after
CONSENSUS_V2_HEIGHT
is reached.Included is the CI branch and the CI diff.
Note that the
test_rand
integration test expectation changed because the program integration tests now use thetest
feature which has different block metadata, thus changing the blockhash, thus changing the on-chain RNG seed.Impact
As stated above, there are two classes of programs that are impacted by this issue:
rand.chacha
but await the futures in a different order that the async functions were called.In scanning all Aleo programs deployed on Mainnet as of 2/11/25 8AM PT:
There are 13 functions among all programs deployed to mainnet, which contain a non-async call, followed by an async call. In the above functions, none of the async calls take a
Future
as input, thus do not exhibit this issue.There are 28 programs that use the
rand.chacha
command and 0 programs that import, and consequently, call them.This was confirmed by a static analyzer on all programs on mainnet at the above date and time and double-checked by manual audit.
Related PRs
StaticAnalysis
pass and add checks on usage of async code for safety. leo#28443To Reviewers
An important function of this PR is to provide an understanding of how async execution mechanism works and why this PR is needed. If reviewers need context or clarification, please feel to post your questions in this thread. I am also happy to do a call explaining the original and proposed design/code.