Skip to content
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

[aptosvm] charge more for keyless #13005

Merged
merged 2 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions aptos-move/aptos-gas-meter/src/meter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -545,13 +545,19 @@ where
.map_err(|e| e.finish(Location::Undefined))
}

fn charge_intrinsic_gas_for_transaction(&mut self, txn_size: NumBytes) -> VMResult<()> {
fn charge_intrinsic_gas_for_transaction(
&mut self,
txn_size: NumBytes,
multiplier: NumBytes,
) -> VMResult<()> {
let excess = txn_size
.checked_sub(self.vm_gas_params().txn.large_transaction_cutoff)
.unwrap_or_else(|| 0.into());

self.algebra
.charge_execution(MIN_TRANSACTION_GAS_UNITS + INTRINSIC_GAS_PER_BYTE * excess)
.charge_execution(
MIN_TRANSACTION_GAS_UNITS * multiplier + INTRINSIC_GAS_PER_BYTE * excess,
)
.map_err(|e| e.finish(Location::Undefined))
}
}
9 changes: 7 additions & 2 deletions aptos-move/aptos-gas-meter/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,13 @@ 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.
fn charge_intrinsic_gas_for_transaction(&mut self, txn_size: NumBytes) -> VMResult<()>;
/// for bigger ones. THe multiplier can be used to increase the unit cost for exceptional
/// transactions like keyless.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fn charge_intrinsic_gas_for_transaction(
&mut self,
txn_size: NumBytes,
multiplier: NumBytes,
) -> VMResult<()>;

/// Charges IO gas for the transaction itself.
fn charge_io_gas_for_transaction(&mut self, txn_size: NumBytes) -> VMResult<()>;
Expand Down
11 changes: 8 additions & 3 deletions aptos-move/aptos-gas-profiling/src/profiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -642,9 +642,14 @@ where
Ok(total_refund)
}

fn charge_intrinsic_gas_for_transaction(&mut self, txn_size: NumBytes) -> VMResult<()> {
let (cost, res) =
self.delegate_charge(|base| base.charge_intrinsic_gas_for_transaction(txn_size));
fn charge_intrinsic_gas_for_transaction(
&mut self,
txn_size: NumBytes,
multiplier: NumBytes,
) -> VMResult<()> {
let (cost, res) = self.delegate_charge(|base| {
base.charge_intrinsic_gas_for_transaction(txn_size, multiplier)
});

self.intrinsic_cost = Some(cost);

Expand Down
11 changes: 9 additions & 2 deletions aptos-move/aptos-gas-schedule/src/gas_schedule/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ crate::gas_schedule::macros::define_gas_parameters!(
[
// The flat minimum amount of gas required for any transaction.
// Charged at the start of execution.
// It is variable to charge more for more expensive authenticators, e.g., keyless
[
min_transaction_gas_units: InternalGas,
min_transaction_gas_units: InternalGasPerByte,
"min_transaction_gas_units",
2_760_000
],
Expand Down Expand Up @@ -230,6 +231,11 @@ crate::gas_schedule::macros::define_gas_parameters!(
{ 15.. => "max_total_dependency_size" },
1024 * 1024 * 12 / 10, // 1.2 MB
],
[
keyless_multiplier: NumBytes,
{ 16.. => "keyless_multiplier" },
vgao1996 marked this conversation as resolved.
Show resolved Hide resolved
150,
vgao1996 marked this conversation as resolved.
Show resolved Hide resolved
]
]
);

Expand All @@ -249,12 +255,13 @@ impl TransactionGasParameters {
pub fn calculate_intrinsic_gas(
&self,
transaction_size: NumBytes,
multiplier: NumBytes,
) -> impl GasExpression<VMGasParameters, Unit = InternalGasUnit> {
let excess = transaction_size
.checked_sub(self.large_transaction_cutoff)
.unwrap_or_else(|| 0.into());

MIN_TRANSACTION_GAS_UNITS + INTRINSIC_GAS_PER_BYTE * excess
MIN_TRANSACTION_GAS_UNITS * multiplier + INTRINSIC_GAS_PER_BYTE * excess
vgao1996 marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
2 changes: 1 addition & 1 deletion aptos-move/aptos-memory-usage-tracker/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,6 @@ where
gas_unit_price: FeePerGasUnit,
) -> PartialVMResult<()>;

fn charge_intrinsic_gas_for_transaction(&mut self, txn_size: NumBytes) -> VMResult<()>;
fn charge_intrinsic_gas_for_transaction(&mut self, txn_size: NumBytes, multiplier: NumBytes) -> VMResult<()>;
}
}
16 changes: 14 additions & 2 deletions aptos-move/aptos-vm/src/aptos_vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,13 @@ impl AptosVM {
})
});

gas_meter.charge_intrinsic_gas_for_transaction(txn_data.transaction_size())?;
let multiplier = if txn_data.is_keyless() {
let gas_params = get_or_vm_startup_failure(&self.gas_params, log_context)?;
gas_params.vm.txn.keyless_multiplier
} else {
NumBytes::one()
};
gas_meter.charge_intrinsic_gas_for_transaction(txn_data.transaction_size(), multiplier)?;

match payload {
TransactionPayload::Script(script) => {
Expand Down Expand Up @@ -1000,7 +1006,13 @@ impl AptosVM {
))
});

gas_meter.charge_intrinsic_gas_for_transaction(txn_data.transaction_size())?;
let multiplier = if txn_data.is_keyless() {
vgao1996 marked this conversation as resolved.
Show resolved Hide resolved
let gas_params = get_or_vm_startup_failure(&self.gas_params, log_context)?;
gas_params.vm.txn.keyless_multiplier
} else {
NumBytes::one()
};
gas_meter.charge_intrinsic_gas_for_transaction(txn_data.transaction_size(), multiplier)?;

// Step 1: Obtain the payload. If any errors happen here, the entire transaction should fail
let invariant_violation_error = || {
Expand Down
9 changes: 7 additions & 2 deletions aptos-move/aptos-vm/src/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use aptos_vm_types::storage::{
io_pricing::IoPricing, space_pricing::DiskSpacePricing, StorageGasParameters,
};
use move_core_types::{
gas_algebra::NumArgs,
gas_algebra::{NumArgs, NumBytes},
language_storage::CORE_CODE_ADDRESS,
vm_status::{StatusCode, VMStatus},
};
Expand Down Expand Up @@ -186,8 +186,13 @@ pub(crate) fn check_gas(
// The submitted transactions max gas units needs to be at least enough to cover the
// intrinsic cost of the transaction as calculated against the size of the
// underlying `RawTransaction`.
let multiplier = if txn_metadata.is_keyless() {
txn_gas_params.keyless_multiplier
} else {
NumBytes::one()
};
let intrinsic_gas = txn_gas_params
.calculate_intrinsic_gas(raw_bytes_len)
.calculate_intrinsic_gas(raw_bytes_len, multiplier)
.evaluate(gas_feature_version, &gas_params.vm)
.to_unit_round_up_with_params(txn_gas_params);

Expand Down
8 changes: 8 additions & 0 deletions aptos-move/aptos-vm/src/transaction_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub struct TransactionMetadata {
pub script_hash: Vec<u8>,
pub script_size: NumBytes,
pub required_deposit: Option<u64>,
pub is_keyless: bool,
}

impl TransactionMetadata {
Expand Down Expand Up @@ -65,6 +66,9 @@ impl TransactionMetadata {
_ => NumBytes::zero(),
},
required_deposit: None,
is_keyless: aptos_types::keyless::get_authenticators(txn)
.map(|res| !res.is_empty())
.unwrap_or(false),
}
}

Expand Down Expand Up @@ -125,4 +129,8 @@ impl TransactionMetadata {
pub fn set_required_deposit(&mut self, required_deposit: Option<u64>) {
self.required_deposit = required_deposit;
}

pub fn is_keyless(&self) -> bool {
self.is_keyless
}
}
4 changes: 2 additions & 2 deletions aptos-move/e2e-tests/src/account_universe/bad_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use aptos_crypto::{
ed25519::{Ed25519PrivateKey, Ed25519PublicKey},
test_utils::KeyPair,
};
use aptos_gas_algebra::{FeePerGasUnit, Gas, GasExpression};
use aptos_gas_algebra::{FeePerGasUnit, Gas, GasExpression, NumBytes};
use aptos_gas_schedule::{AptosGasParameters, InitialGasSchedule, LATEST_GAS_FEATURE_VERSION};
use aptos_proptest_helpers::Index;
use aptos_types::{
Expand Down Expand Up @@ -92,7 +92,7 @@ impl AUTransactionGen for InsufficientBalanceGen {
let txn_gas_params = &gas_params.vm.txn;
let raw_bytes_len = txn.raw_txn_bytes_len() as u64;
let min_cost: Gas = txn_gas_params
.calculate_intrinsic_gas(raw_bytes_len.into())
.calculate_intrinsic_gas(raw_bytes_len.into(), NumBytes::one())
.evaluate(LATEST_GAS_FEATURE_VERSION, &gas_params.vm)
.to_unit_round_up_with_params(txn_gas_params);

Expand Down
5 changes: 2 additions & 3 deletions aptos-move/e2e-testsuite/src/tests/verify_txn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use aptos_cached_packages::aptos_stdlib;
use aptos_crypto::{ed25519::Ed25519PrivateKey, PrivateKey, Uniform};
use aptos_gas_algebra::Gas;
use aptos_gas_algebra::{Gas, NumBytes};
use aptos_gas_schedule::{InitialGasSchedule, TransactionGasParameters};
use aptos_language_e2e_tests::{
assert_prologue_disparity, assert_prologue_parity, common_transactions::EMPTY_SCRIPT,
Expand Down Expand Up @@ -304,8 +304,7 @@ fn verify_simple_payment() {

// Test for a max_gas_amount that is insufficient to pay the minimum fee.
// Find the minimum transaction gas units and subtract 1.
let mut gas_limit: Gas = txn_gas_params
.min_transaction_gas_units
let mut gas_limit: Gas = (txn_gas_params.min_transaction_gas_units * NumBytes::one())
.to_unit_round_up_with_params(&txn_gas_params);

if gas_limit > 0.into() {
Expand Down
Loading