-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[release-builder] local simulation of governance proposals #13949
Conversation
⏱️ 2h 21m total CI duration on this PR
|
a62dd2d
to
ffe4265
Compare
let txn_gas_params = &mut gas_params.vm.txn; | ||
// Use the alternative limits for governance proposals | ||
// TODO: In the future, consider adding the execution hashes of the scripts to the approval list. | ||
txn_gas_params.max_execution_gas = txn_gas_params.max_execution_gas_gov; |
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.
Doesn't your recent change to automatically bump these numbers to _gov
if it is a governance proposal work here?
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.
This is one of the more ugly parts of the current implementation.
Doesn't your recent change to automatically bump these numbers to _gov if it is a governance proposal work here?
Yes, but the alt limits will only kick in if the script has its hash added to the list of approved execution hashes, which gets skipped by the mock version of resolve_multi_step_proposal
as well.
It's possible for us to manually add it in Rust and I tried it, but there are some complexities involved.
Made a major update to the PR
|
&log_context, | ||
); | ||
// We require all governance scripts to trigger reconfiguration so check it here. | ||
if AptosVM::should_restart_execution(vm_output.events()) { |
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.
Should it be the negation of this?
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.
Good catch.
However looks like this would not be an easy fix. With dkg, reconfiguration is started by the script but may not actually happen until the next epoch.
I guess I'll have to remove this check for now, and we'll need to discuss what the proper solution might be. I'm thinking that maybe reconfiguration_with_dkg::try_start
should emit its own event indicating this.
let script_name = script_path.file_name().unwrap().to_string_lossy(); | ||
println!(" {}", script_name); | ||
|
||
// Create a new VM to ensure the loader is clean. |
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.
Have you checked this? Because I think warm vm cache does load PackageMetadata for core packages to see if it has changed... or this is the reason, we patch code and package metadata is the same?
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.
Yes, I spent a few hours debugging this yesterday.
or this is the reason, we patch code and package metadata is the same?
Exactly, the patching done here does not change the metadata, so the warm vm cache fails to see it needs to reload everything.
030dd06
to
8f3c907
Compare
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.
This is fucking magical, a really important tool for release verification <3
/// | ||
/// Possible values: devnet, testnet, mainnet, <url to rest endpoint> | ||
#[clap(long)] | ||
network: NetworkSelection, |
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.
nice! validate proposal also has an "endpoint" arguement, it would be great could replace its usage of URL with network so we dont have to specify the testnet / mainnet / devnet url all the time there either
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 think the overall logic makes sense to me. The patching logic looked ugly but I don't see a way round for now. We still need a separate PR in the aptos-network to use this command btw.
state_view.apply_write_set(write_set); | ||
} | ||
|
||
println!("All scripts succeeded!"); |
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.
We need to check there's no pending script hash there. Can you add the check or at least add a todo here? I think it would be important for our release safety.
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 think I've implemented this check, no?
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.
Oh nvm. I thought you didn't.
if forbid_next_execution_hash { | ||
// If it is needed to forbid a next execution hash, inject additional Move | ||
// code at the beginning that aborts with a magic number if the vector | ||
// representing the hash is not empty. | ||
// | ||
// if (!vector::is_empty(&next_execution_hash)) { | ||
// abort MAGIC_FAILED_NEXT_EXECUTION_HASH_CHECK; | ||
// } | ||
// | ||
// The magic number can later be checked in Rust to determine if such violation | ||
// has happened. | ||
code.code.extend([ | ||
ImmBorrowLoc(2), | ||
VecLen(sig_u8_idx), | ||
LdU64(0), | ||
Eq, | ||
BrTrue(7), | ||
LdU64(MAGIC_FAILED_NEXT_EXECUTION_HASH_CHECK), | ||
Abort, | ||
]); | ||
} |
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.
@runtian-zhou here I check if the next execution hash is empty, and if so, abort with a magic number
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.
amazing!
@perryjrandall I've made some updates to the PR
Additionally I've also created this issue #14044 for tracking follow-up items that I don't plan to address immediately. |
Update the PR again.
Given that the PR is rather polished right now I'll proceed to landing. If there are additional feature requests I'll address them separately. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
This introduces a new release builder command that enables the simulation of governance proposals. Currently only multi-step proposals are supported.
It utilizes the the remote debugger infrastructure to fetch real chain states for local simulation, but adds another in-memory database to store the new side effects generated by the governance scripts.
Normally, governance scripts needs to be approved through on-chain governance before they could be executed. This process involves setting up various states (e.g., staking pool, delegated voter), which can be quite complex.
This simulation bypasses these challenges by patching specific Move functions with mock versions, most notably
fun resolve_multi_step_proposal
, thus allowing the governance process to be skipped altogether. In other words, this simulation is intended for checking whether a governance proposal will execute successfully, assuming it gets approved.How to run simulation
First generate the proposal
Then run simulation via the following command
Here's how the output should look like
Type of Change
Which Components or Systems Does This Change Impact?
Key Areas to Review
simulate.rs
Checklist