[aptosvm] charge more for keyless #13005
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
add a 150x multiplier for intrinsic gas for keyless txns * Swap `min_transaction_gas_units` from `InternalGas` to `InternalGasPerByte` -- that has no underlying change but the semantic change allows for it to be multiplied * Add in a multiplier for all intrinsic gas charges which will be 1 by default * Set a gas schedule of 150 for keyless -- defaults to 150 since this is live on testnet already...
davidiw
requested review from
wrwg,
zekun000,
vgao1996 and
georgemitenkov
as code owners
alinush
approved these changes
Apr 24, 2024
igor-aptos
added
CICD:run-execution-performance-test
igor-aptos
approved these changes
Apr 24, 2024
There was a problem hiding this comment.
added labels to see if performance will be impacted by the call to check if txns is keyless
from the cleanliness of the code, I would've preferred:
- if it wasn't a multiplier, but an addition. i.e. keyless transactions cost that much more, and we take current intrinsic cost and multiply by 149 to initialize it
- if charge_intrinsic_gas_for_transaction was receiving the context (i.e. a is_keyless: bool, or authenticator_type: AuthenticatorTypeForCharging, with AuthenticatorTypeForCharging being an enum with Normal and Keyless values), and have it access the value. That way we don't have multiple places doing get_or_vm_startup_failure and accessing keyless_multiplier.
but these are nits, and can be changed later as well, so given time, looks good to me as is as well, up to you if you want to apply any of those or not.
vgao1996
reviewed
Apr 24, 2024
davidiw
changed the title
This reimplements the gas charging for keyless in a more proper way, decoupling the keyless charge from the exisitng intrinsic charge.
davidiw
commented
Apr 24, 2024
| [ | ||
| keyless_base_cost: InternalGas, | ||
| { 17.. => "keyless.base" }, | ||
| 414_000_000, |
There was a problem hiding this comment.
might have been cleaner to make this x*y, but still works
vgao1996
reviewed
Apr 24, 2024
| @@ -110,9 +110,14 @@ pub trait AptosGasMeter: MoveGasMeter { | |||
| /// Charges an intrinsic cost for executing the transaction. | |||
| /// | |||
| /// The cost stays constant for transactions below a certain size, but will grow proportionally | |||
| /// for bigger ones. | |||
| /// for bigger ones. THe multiplier can be used to increase the unit cost for exceptional | |||
| /// transactions like keyless. | |||
There was a problem hiding this comment.
Forgot to delete the extra comment here, but let's not bother with it right now as that'll trigger a rerun for all the tests
igor-aptos
approved these changes
Apr 24, 2024
There was a problem hiding this comment.
update looks good.
@vgao1996 for gas calibration utilities, we should see whether to treat keyless gas charge as part of the intrinsic or not
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
✅ Forge suite
|
✅ Forge suite
|
21 tasks
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.
Description
add a 150x multiplier for intrinsic gas for keyless txns
min_transaction_gas_unitsfromInternalGastoInternalGasPerByte-- that has no underlying change but the semantic change allows for it to be multipliedType of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
No other impacts otherwise some goldens would have been modified.
Key Areas to Review
Checklist