Skip to content

Commit

Permalink
f
Browse files Browse the repository at this point in the history 
charge as much as possible for aborted sponsored transactiosn at sequence number 0

note this requires an AIP update since the simulation value for this may
be wrong for the minimum required gas, since the fee payer must over
allocate budget in case this transaction creates the account.
  • Loading branch information
@davidiw
davidiw committed Dec 5, 2023
1 parent 58ba18a commit 369a4ad
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 80 deletions.
196 changes: 118 additions & 78 deletions aptos-move/aptos-vm/src/aptos_vm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use anyhow::{anyhow, Result};
use aptos_block_executor::txn_commit_hook::NoOpTransactionCommitHook;
use aptos_crypto::HashValue;
use aptos_framework::natives::code::PublishRequest;
use aptos_gas_algebra::Gas;
use aptos_gas_algebra::{Gas, GasQuantity, Octa};
use aptos_gas_meter::{AptosGasMeter, GasAlgebra, StandardGasAlgebra, StandardGasMeter};
use aptos_gas_schedule::VMGasParameters;
use aptos_logger::{enabled, prelude::*, Level};
Expand Down Expand Up @@ -309,13 +309,6 @@ impl AptosVM {
log_context: &AdapterLogSchema,
change_set_configs: &ChangeSetConfigs,
) -> (VMStatus, VMOutput) {
let transaction_status = TransactionStatus::from_vm_status(
error_code.clone(),
self.vm_impl
.get_features()
.is_enabled(FeatureFlag::CHARGE_INVARIANT_VIOLATION),
);

if self.vm_impl.get_gas_feature_version() >= 12 {
// Check if the gas meter's internal counters are consistent.
//
Expand All @@ -338,52 +331,12 @@ impl AptosVM {
}
}

// Clear side effects: create new session and clear refunds from fee statement.
let mut session = self
.vm_impl
.new_session(resolver, SessionId::run_on_abort(txn_data));

if matches!(transaction_status, TransactionStatus::Keep(_))
&& txn_data.fee_payer().is_some()
&& txn_data.sequence_number == 0
&& self
.vm_impl
match TransactionStatus::from_vm_status(
error_code.clone(),
self.vm_impl
.get_features()
.is_enabled(FeatureFlag::SPONSORED_AUTOMATIC_ACCOUNT_CREATION)
{
if let Err(err) =
create_account_if_does_not_exist(&mut session, gas_meter, txn_data.sender())
.map_err(expect_no_verification_errors)
.or_else(|err| {
expect_only_successful_execution(
err,
&format!("{:?}::{}", ACCOUNT_MODULE, CREATE_ACCOUNT_IF_DOES_NOT_EXIST),
log_context,
)
})
{
return discard_error_vm_status(err);
}
}

// Need to zero this out, because that's the legacy behavior. Otherwise we charge for txn
// size where we used to not.
let mut null_txn_data = txn_data.clone();
null_txn_data.transaction_size = 0.into();
let mut respawned_session = match self.charge_change_set_and_respawn_session(
session,
resolver,
gas_meter,
change_set_configs,
&null_txn_data,
.is_enabled(FeatureFlag::CHARGE_INVARIANT_VIOLATION),
) {
Ok(respawned_session) => respawned_session,
Err(err) => return discard_error_vm_status(err),
};

let fee_statement = AptosVM::fee_statement_from_gas_meter(txn_data, gas_meter, 0);

match transaction_status {
TransactionStatus::Keep(status) => {
// Inject abort info if available.
let status = match status {
Expand All @@ -401,28 +354,21 @@ impl AptosVM {
},
_ => status,
};

// The transaction should be charged for gas, so run the epilogue to do that.
// This is running in a new session that drops any side effects from the
// attempted transaction (e.g., spending funds that were needed to pay for gas),
// so even if the previous failure occurred while running the epilogue, it
// should not fail now. If it somehow fails here, there is no choice but to
// discard the transaction.


if let Err(e) = respawned_session.execute(|session| {
self.vm_impl.run_failure_epilogue(
session,
gas_meter.balance(),
fee_statement,
txn_data,
log_context,
)
}) {
return discard_error_vm_status(e);
}

let txn_output = match respawned_session.finish(change_set_configs) {
Ok(change_set) => {
let txn_output = match self.finish_aborted_transaction(
gas_meter,
txn_data,
resolver,
log_context,
change_set_configs,
) {
Ok((change_set, fee_statement)) => {
VMOutput::new(change_set, fee_statement, TransactionStatus::Keep(status))
},
Err(err) => discard_error_vm_status(err).1,

Check warning on line 374 in aptos-move/aptos-vm/src/aptos_vm.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/aptos_vm.rs#L374 

Added line #L374 was not covered by tests
Expand All @@ -437,6 +383,90 @@ impl AptosVM {
}
}

fn finish_aborted_transaction(
&self,
gas_meter: &mut impl AptosGasMeter,
txn_data: &TransactionMetadata,
resolver: &impl AptosMoveResolver,
log_context: &AdapterLogSchema,
change_set_configs: &ChangeSetConfigs,
) -> Result<(VMChangeSet, FeeStatement), VMStatus> {
if txn_data.fee_payer().is_some()
&& txn_data.sequence_number == 0
&& self
.vm_impl
.get_features()
.is_enabled(FeatureFlag::SPONSORED_AUTOMATIC_ACCOUNT_CREATION)
{
// Clear side effects: create new session and clear refunds from fee statement.
let mut session = self
.vm_impl
.new_session(resolver, SessionId::run_on_abort(txn_data));

create_account_if_does_not_exist(&mut session, gas_meter, txn_data.sender())
.map_err(expect_no_verification_errors)
.or_else(|err| {
expect_only_successful_execution(
err,
&format!("{:?}::{}", ACCOUNT_MODULE, CREATE_ACCOUNT_IF_DOES_NOT_EXIST),
log_context,
)

Check warning on line 413 in aptos-move/aptos-vm/src/aptos_vm.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/aptos_vm.rs#L409-L413 

Added lines #L409 - L413 were not covered by tests
})?;

let mut change_set = session.finish(change_set_configs)?;
if let Err(err) = self.charge_change_set(&mut change_set, gas_meter, txn_data) {
info!(
*log_context,
"Failed during charge_change_set: {:?}. Most likely exceded gas limited.", err,

Check warning on line 420 in aptos-move/aptos-vm/src/aptos_vm.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/aptos_vm.rs#L420 

Added line #L420 was not covered by tests
);
};

// zero out storage refund
let storage_refund = 0;
let fee_statement =
AptosVM::fee_statement_from_gas_meter(txn_data, gas_meter, storage_refund);

let session_id = SessionId::epilogue_meta(txn_data);
let mut respawned_session = RespawnedSession::spawn(
self,
session_id,
resolver,
change_set,
storage_refund.into(),
)?;
respawned_session.execute(|session| {
self.vm_impl.run_failure_epilogue(
session,
gas_meter.balance(),
fee_statement,
txn_data,
log_context,
)
})?;
respawned_session
.finish(change_set_configs)
.map(|set| (set, fee_statement))
} else {
// Clear side effects: create new session and clear refunds from fee statement.
let mut session = self
.vm_impl
.new_session(resolver, SessionId::run_on_abort(txn_data));

let fee_statement = AptosVM::fee_statement_from_gas_meter(txn_data, gas_meter, 0);
self.vm_impl.run_failure_epilogue(
&mut session,
gas_meter.balance(),
fee_statement,
txn_data,
log_context,
)?;
session
.finish(change_set_configs)
.map(|set| (set, fee_statement))
.map_err(|e| e.into())
}
}

fn success_transaction_cleanup(
&self,
mut respawned_session: RespawnedSession,
Expand Down Expand Up @@ -606,16 +636,12 @@ impl AptosVM {
}
}

fn charge_change_set_and_respawn_session<'r, 'l>(
&'l self,
session: SessionExt,
resolver: &'r impl AptosMoveResolver,
fn charge_change_set(
&self,
change_set: &mut VMChangeSet,
gas_meter: &mut impl AptosGasMeter,
change_set_configs: &ChangeSetConfigs,
txn_data: &TransactionMetadata,
) -> Result<RespawnedSession<'r, 'l>, VMStatus> {
let mut change_set = session.finish(change_set_configs)?;

) -> Result<GasQuantity<Octa>, VMStatus> {
for (key, op) in change_set.write_set_iter() {
gas_meter.charge_io_gas_for_write(key, op)?;
}
Expand All @@ -640,10 +666,11 @@ impl AptosVM {
}

let mut storage_refund = gas_meter.process_storage_fee_for_all(
&mut change_set,
change_set,
txn_data.transaction_size,
txn_data.gas_unit_price,
)?;

if !self
.vm_impl
.get_features()
Expand All @@ -652,6 +679,20 @@ impl AptosVM {
storage_refund = 0.into();
}

Ok(storage_refund)
}

fn charge_change_set_and_respawn_session<'r, 'l>(
&'l self,
session: SessionExt,
resolver: &'r impl AptosMoveResolver,
gas_meter: &mut impl AptosGasMeter,
change_set_configs: &ChangeSetConfigs,
txn_data: &TransactionMetadata,
) -> Result<RespawnedSession<'r, 'l>, VMStatus> {
let mut change_set = session.finish(change_set_configs)?;
let storage_refund = self.charge_change_set(&mut change_set, gas_meter, txn_data)?;

// TODO[agg_v1](fix): Charge for aggregator writes
let session_id = SessionId::epilogue_meta(txn_data);
RespawnedSession::spawn(self, session_id, resolver, change_set, storage_refund)
Expand Down Expand Up @@ -1356,7 +1397,6 @@ impl AptosVM {
.new_session(resolver, SessionId::txn_meta(&txn_data));
}

let txn_data = TransactionMetadata::new(txn);
if txn_data.fee_payer().is_some()
&& txn_data.sequence_number == 0
&& self
Expand Down
3 changes: 1 addition & 2 deletions aptos-move/aptos-vm/src/aptos_vm_impl.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,8 +339,7 @@ impl AptosVMImpl {
}

let max_gas_amount: u64 = txn_data.max_gas_amount().into();
let storage_slot_cost: u64 =
txn_gas_params.storage_fee_per_state_slot_create.into();
let storage_slot_cost: u64 = txn_gas_params.storage_fee_per_state_slot_create.into();

// If this is a sponsored transaction for a potentially new account, ensure there's enough
// gas to cover storage, execution, and IO costs.
Expand Down

0 comments on commit 369a4ad

Please sign in to comment.