randtxn custom max gas #13269
randtxn custom max gas #13269
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13269 +/- ##
===========================================
- Coverage 71.9% 57.5% -14.4%
===========================================
Files 2315 833 -1482
Lines 454561 198666 -255895
===========================================
- Hits 326875 114281 -212594
+ Misses 127686 84385 -43301 ☔ View full report in Codecov by Sentry. |
zjma
force-pushed
the
zjma/randtxn_custom_max_gas
branch
from
fb9248a
to
81d96a9
Compare
zjma
requested review from
davidiw,
movekevin,
junkil-park,
wrwg,
zekun000,
vgao1996 and
georgemitenkov
as code owners
zjma
added
the
CICD:run-e2e-tests
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.
This comment has been minimized.
This comment has been minimized.
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| @@ -217,6 +219,9 @@ impl AptosVM { | |||
| let _timer = TIMER.timer_with(&["AptosVM::new"]); | |||
| let randomness_api_v0_required_deposit = RequiredGasDeposit::fetch_config(resolver) | |||
| .unwrap_or_else(RequiredGasDeposit::default_if_missing); | |||
| let allow_rand_txn_custom_max_gas = AllowCustomMaxGasFlag::fetch_config(resolver) | |||
There was a problem hiding this comment.
Why do we use structs for configs and not Features? What is the difference?
There was a problem hiding this comment.
Im just leaning to putting related configs close to each other.
There was a problem hiding this comment.
I think features are cleaner. Imagine we have more and more features and everyone adds a new struct for those... this can quickly become a mess. Either you have a config stored as vector<u8>, e.g. gas schedule, which has multiple fields which evolve, or a feature flag. Otherwise this stays forever...
There was a problem hiding this comment.
I prefer a normal feature flag in this case as well.
There was a problem hiding this comment.
if the flag was controlling an entire standalone feature, it makes more sense for the flag to live in the feature flag vec.
But in this case, technically it is part of another config which has been separated out.
There was a problem hiding this comment.
I also found the features flag vec is very limited: it only support boolean flags, but more and more features need configs like this:
enum FeatureXConfig {
Off,
OnMode1 { limit_a0: u64 },
OnMode2 { limit_a1: u64, threshold_b: u64 },
}
There was a problem hiding this comment.
It's true that we could have foreseen this situation and merged RequiredGasDeposit and this new flag into a single struct. Unfortunately we didn't...
Anyway, nothing stops new config structs to be added for other features, so having an extra one here is fine?
Would like to continue with the current implementation, if you don't mind :)
There was a problem hiding this comment.
Feature flags are only for booleans, yes. If you need the config like you want above, with options and fields, there are two ways:
- Define a rust struct which is initialised based on boolean flags. Please check how
VMConfigorVerifierConfigare created. They have built-in values which we can upgrade per release, and also use feature flags to make stricter. - Have a config stored on chain as a serialized vector blob. Our gas schedule is exactly that, it is just an enum of BTreeMaps. This way new versions can be added, and new gas params. Another example: initialized based on gas feature version. Another example: , but seems like it is not initialized from on-chain yet. But this is a good one: .
| if has_randomness_attribute(resolver, session, entry_func).unwrap_or(false) { | ||
| if gas_amount != u64::from(txn_metadata.max_gas_amount) { | ||
| if let Some(annotation) = | ||
| get_randomness_annotation(resolver, session, entry_func).unwrap_or(None) |
There was a problem hiding this comment.
Use ?, not unwrap? This function fails on module loading so we should return an error
There was a problem hiding this comment.
i don't know if changing it now breaks compatibility... current code has been shipped with 1.12.
There was a problem hiding this comment.
anyway, why is it not fine to have this function focus on required deposit calculation?
if module loading failed, other existing steps should catch it anyway?
There was a problem hiding this comment.
I don't like relying on "other existing steps"... we do not have clear semantics of what are the next steps, and this can bite us in the future. It is ok here, I guess...
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| let required_gas_amount = if self.allow_rand_txn_custom_max_gas { | ||
| annotation.max_gas.unwrap_or(default_gas_amount) | ||
| } else { | ||
| default_gas_amount | ||
| }; |
There was a problem hiding this comment.
nit:
self.allow_...gas.then(annotation.max_gas).flatten().unwrap_or(default_gas_amount)
| @@ -9,11 +9,17 @@ module aptos_framework::randomness_api_v0_config { | |||
| gas_amount: Option<u64>, | |||
| } | |||
|
|
|||
| /// If this flag is set, `max_gas` specified inside `#[randomness()]` will be used as the required deposit. | |||
| struct AllowCustomMaxGasFlag has key, drop, store { | |||
There was a problem hiding this comment.
Isn't a feature flag cleaner? Why have a single struct like that?
There was a problem hiding this comment.
i thought about it, but putting related configs close to each other also has its benefit...
There was a problem hiding this comment.
What is the benefit? We have more and more different configs we have to load from storage. I think it is better to have everything in one place, so no errors can be made. We have features, and it is clear what is enabled what is disabled.
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.
This comment has been minimized.
This comment has been minimized.
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| @@ -197,6 +198,11 @@ pub(crate) fn get_or_vm_startup_failure<'a, T>( | |||
| }) | |||
| } | |||
|
|
|||
| struct AptosVMRandomnessConfig { | |||
| randomness_api_v0_required_deposit: Option<u64>, | |||
| allow_rand_txn_custom_max_gas: bool, | |||
There was a problem hiding this comment.
nit: it's allow rand contract not the txn to custom max gas?
| } | ||
|
|
||
| let msg = | ||
| "Property `max_gas` should be a positive integer less than 2^64".to_string(); |
There was a problem hiding this comment.
it probably should be less than the maximum gas we allowed, as least in the error message
There was a problem hiding this comment.
but that limit is an on-chain config that's hard to know at compile time?
There was a problem hiding this comment.
yes, that's why I only suggested changing error message lol
There was a problem hiding this comment.
what's the exact error msg in your mind?
There was a problem hiding this comment.
less than maximum_number_of_gas_units allowed?
There was a problem hiding this comment.
how would dev figure out maximum_number_of_gas_units?
There was a problem hiding this comment.
I don't know, we can also just put aptos_global_constants::MAX_GAS_AMOUNT here, but anyway it's weird since it only errors out when it exceeds u64::max
There was a problem hiding this comment.
if dev specifies u64::max + 1, it's surely won't work because we use u64 to represent gas amount throughout the codebase.
If dev specifies u64::max - 1, one will not be sure if it will work at compile time, depending on on-chain gas params. So it should be a runtime error?
| @@ -38,6 +39,7 @@ const LEGACY_ENTRY_FUN_ATTRIBUTE: &str = "legacy_entry_fun"; | |||
| const ERROR_PREFIX: &str = "E"; | |||
| const EVENT_STRUCT_ATTRIBUTE: &str = "event"; | |||
| const RANDOMNESS_ATTRIBUTE: &str = "randomness"; | |||
| const RANDOMNESS_MAX_GAS_CLAIM: &str = "max_gas"; | |||
There was a problem hiding this comment.
this may not be very clear to user, how about "gas_deposit"?
There was a problem hiding this comment.
no way the keyword itself will be self-explanatory. we should rely on the documentation?
There was a problem hiding this comment.
we should have both, but isn't "gas_deposit" better? maybe it's just me
There was a problem hiding this comment.
gas_deposit sounds more like implementation details of undergasing prevention, which dev doesn't need to know?
There was a problem hiding this comment.
this is the amount that user needs to deposit? but I don't feel very strongly
There was a problem hiding this comment.
yeah, let's just explain more in the doc.
debug debug debug debug debug debug debug debug debug clean up debug update lint update update
zjma
force-pushed
the
zjma/randtxn_custom_max_gas
branch
from
259f5c9
to
c2513ed
Compare
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.
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.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I would like to have more thoughts on how randomness configs are organised. Do we plan to add more things? Do we plan to update them often?
I think designs similar to execution configs, or gas schedule are the best because they will allow you to have a single rust struct and also a single on-chain resource, as well as version and mutate it. Having multiple configs scattered around the code makes reasoning harder. I do not think we should do it.
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| @@ -216,6 +217,11 @@ fn is_approved_gov_script( | |||
| } | |||
| } | |||
|
|
|||
| struct AptosVMRandomnessConfig { | |||
There was a problem hiding this comment.
This should go into separate file, it's ok for now but please add a TODO. For example, you can put it in types where you define randomness OnChainConfig
There was a problem hiding this comment.
fixed, but something still has to stay inside crate aptos-vm because fetch requires a resolver: &impl AptosMoveResolver which is only available in this crate.
There was a problem hiding this comment.
Btw, you can use &impl ConfigStorage trait and move it out (not too important I guess). The only reason why resolver is used is because we have one config which is inside resource group
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| @@ -235,7 +241,15 @@ impl AptosVM { | |||
| ) -> Self { | |||
| let _timer = TIMER.timer_with(&["AptosVM::new"]); | |||
| let randomness_api_v0_required_deposit = RequiredGasDeposit::fetch_config(resolver) | |||
| .unwrap_or_else(RequiredGasDeposit::default_if_missing); | |||
| .unwrap_or_else(RequiredGasDeposit::default_if_missing) | |||
There was a problem hiding this comment.
This can also be factored out to that helper config file, VM only needs to know how to create a randomness config, but not the exact resources it has to fetch. Gas schedule is a good example for that.
| if has_randomness_attribute(resolver, session, entry_func).unwrap_or(false) { | ||
| if gas_amount != u64::from(txn_metadata.max_gas_amount) { | ||
| if let Some(annotation) = | ||
| get_randomness_annotation(resolver, session, entry_func).unwrap_or(None) |
There was a problem hiding this comment.
I don't like relying on "other existing steps"... we do not have clear semantics of what are the next steps, and this can bite us in the future. It is ok here, I guess...
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.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
Description
#[randomness(max_gas=<x>)]and build it into module info.randomness_api_v0_config::AllowCustomMaxGasFlag.AllowCustomMaxGasFlagis on, ifmax_gasis set in randomness attribute, use it as the required gas amount instead of the default value 10000.Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
New smoke tests in
testsuite/smoke-test/src/randomness/entry_func_attrs.rs.Key Areas to Review
Please start review with the smoke test in
testsuite/smoke-test/src/randomness/entry_func_attrs.rsand confirm the assertions made there.Checklist