[gas] allow higher limits for approved governance proposals #13244
Conversation
vgao1996
force-pushed
the
relax-gov-exec-limit
branch
3 times, most recently
from
e7fd32a
to
dc61d12
Compare
vgao1996
force-pushed
the
relax-gov-exec-limit
branch
from
dc61d12
to
10e5dc1
Compare
vgao1996
requested review from
davidiw,
wrwg,
zekun000 and
georgemitenkov
as code owners
aptos-move/e2e-move-tests/src/tests/infinite_loop.data/empty_loop_script/sources/test.move
Outdated
Show resolved
Hide resolved
| ))), | ||
| ); | ||
| let gas_used = output.gas_used(); | ||
| assert!(max_gas_regular - 1 <= gas_used && gas_used <= max_gas_regular + 1); |
There was a problem hiding this comment.
why not gas_used == max_gas_regular? I do not understand why we allow off by 1 here...
There was a problem hiding this comment.
Good catch. I was originally thinking that there was an off by 1 error caused by rounding, but that was the wrong reason.
The reason why we're having this is because when a transaction gets terminated due to hitting execution gas limit, there could an overshoot, as long as the overall gas budget hasn't been depleted.
I've changed the formula to let overshoot = (max_gas_regular.min(max_gas_gov) / 5).max(1);, which should be a bit more robust than a constant value 1.
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| fn validate_signed_transaction( | ||
| &self, | ||
| session: &mut SessionExt, | ||
| resolver: &impl AptosMoveResolver, | ||
| transaction: &SignedTransaction, | ||
| transaction_data: &TransactionMetadata, | ||
| log_context: &AdapterLogSchema, | ||
| is_governance_proposal: bool, |
There was a problem hiding this comment.
Can this be part of TransactionMetadata?
There was a problem hiding this comment.
No, because in order to determine if a transaction is an approved governance proposal, you need to check whether its hash exists in ApprovedExecutionHashes, which is not something you can derive from the transaction itself like other transaction metadata does.
There was a problem hiding this comment.
All usages of is_governance_proposal() function are where we create the metadata, e.g.
let txn_metadata = TransactionMetadata::new(txn);
let is_governance_proposal = is_governance_proposal(resolver, txn, &txn_metadata);
This is true for exactly 2 places where we create metadata. It seems cleaner to have this as a field, instead of passing a flag everywhere, no?
There was a problem hiding this comment.
Yeah, again I guess the real deciding factor is how we want to define transaction metadata.
- If we want it to contain info strictly derivable from the transaction itself, then the is_governance_proposal flag has to be passed around
- Otherwise, we may include it in the txn metadata, and deal with the implication of requiring a resolver to compute metadata.
There was a problem hiding this comment.
Tbh, I am not exactly sure what this metadata is, I think the idea was to have transaction-representation agnostic data structure for the VM, which also caches some information. We already store keyless/randomness related info there:
pub required_deposit: Option<u64>,
pub is_keyless: bool,
There was a problem hiding this comment.
Again, is_keyless is information you can derive from the txn itself. is_governance_proposal depends on the on-chain state.
|
@georgemitenkov I've addressed pretty much all of your comments. The new changes are in a separate commit. Please take another look! |
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 adds separate limits that applies to approved governance proposals in all gas categories (exec, io & storage), eliminating the need to relax these limits during releases, which is error-prone.
Additionally, this also makes the transaction size limit for governance proposals configurable.
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
E2E test added.
Key Areas to Review
aptos-gas-scheduleaptos-vmChecklist