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.

Sign up for GitHub

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

Aptos gas payer #8773

Merged
merged 19 commits into from
Jun 23, 2023
Merged

Aptos gas payer #8773

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
054a910
initial commit
gerben-stavenga May 31, 2023
54dbda6
Merge branch 'main' into aptos-gas-payer
gerben-stavenga Jun 21, 2023
af458e4
Support gas payer
gerben-stavenga Jun 21, 2023
e1fcb6b
Remove relics
gerben-stavenga Jun 21, 2023
77504bc
Fix
gerben-stavenga Jun 21, 2023
fe4ad39
Make verification know about gas payer bit
gerben-stavenga Jun 21, 2023
53ed2c8
update comments
gerben-stavenga Jun 21, 2023
60e0d2b
Merge branch 'main' into aptos-gas-payer
gerben-stavenga Jun 21, 2023
63c09f4
Remove merge conflict
gerben-stavenga Jun 21, 2023
43b4d71
Give MSB a good name
gerben-stavenga Jun 21, 2023
df6e2ef
Reformat
gerben-stavenga Jun 21, 2023
dcd31a0
improve
gerben-stavenga Jun 22, 2023
4ae27d6
add comment
gerben-stavenga Jun 22, 2023
ff02599
Fix spec
gerben-stavenga Jun 22, 2023
b716495
Fixed the broken Prover spec
junkil-park Jun 23, 2023
23298a4
Add tests
gerben-stavenga Jun 23, 2023
c0ec675
Fix prover + add tests
gerben-stavenga Jun 23, 2023
9b0b86f
Insufficient funds test
gerben-stavenga Jun 23, 2023
cbcab07
Merge branch 'main' into aptos-gas-payer
gerben-stavenga Jun 23, 2023
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
3 changes: 3 additions & 0 deletions aptos-move/aptos-release-builder/src/components/feature_flags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub enum FeatureFlag {
StorageSlotMetadata,
ChargeInvariantViolation,
DelegationPoolPartialGovernanceVoting,
GasPayerEnabled,
}

fn generate_features_blob(writer: &CodeWriter, data: &[u64]) {
Expand Down Expand Up @@ -160,6 +161,7 @@ impl From<FeatureFlag> for AptosFeatureFlag {
FeatureFlag::DelegationPoolPartialGovernanceVoting => {
AptosFeatureFlag::DELEGATION_POOL_PARTIAL_GOVERNANCE_VOTING
},
FeatureFlag::GasPayerEnabled => AptosFeatureFlag::GAS_PAYER_ENABLED,
}
}
}
Expand Down Expand Up @@ -203,6 +205,7 @@ impl From<AptosFeatureFlag> for FeatureFlag {
AptosFeatureFlag::DELEGATION_POOL_PARTIAL_GOVERNANCE_VOTING => {
FeatureFlag::DelegationPoolPartialGovernanceVoting
},
AptosFeatureFlag::GAS_PAYER_ENABLED => FeatureFlag::GasPayerEnabled,
}
}
}
Expand Down
17 changes: 12 additions & 5 deletions aptos-move/aptos-vm/src/aptos_vm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ use std::{
Arc,
},
};
use crate::aptos_vm_impl::GAS_PAYER_FLAG_BIT;

static EXECUTION_CONCURRENCY_LEVEL: OnceCell<usize> = OnceCell::new();
static NUM_EXECUTION_SHARD: OnceCell<usize> = OnceCell::new();
Expand Down Expand Up @@ -394,6 +395,15 @@ impl AptosVM {
)?)
}

fn get_senders(txn_data: &TransactionMetadata) -> Vec<AccountAddress> {
movekevin marked this conversation as resolved.
Show resolved Hide resolved
let mut res = vec![txn_data.sender];
res.extend(txn_data.secondary_signers());
if txn_data.sequence_number & GAS_PAYER_FLAG_BIT != 0 {
gerben-stavenga marked this conversation as resolved.
Show resolved Hide resolved
res.pop();
}
res
}

fn execute_script_or_entry_function(
&self,
resolver: &impl MoveResolverExt,
Expand All @@ -418,8 +428,7 @@ impl AptosVM {

match payload {
TransactionPayload::Script(script) => {
let mut senders = vec![txn_data.sender()];
senders.extend(txn_data.secondary_signers());
let senders = Self::get_senders(txn_data);
let loaded_func =
session.load_script(script.code(), script.ty_args().to_vec())?;
let args =
Expand All @@ -440,9 +449,7 @@ impl AptosVM {
)?;
},
TransactionPayload::EntryFunction(script_fn) => {
let mut senders = vec![txn_data.sender()];

senders.extend(txn_data.secondary_signers());
let senders = Self::get_senders(txn_data);
self.validate_and_execute_entry_function(
&mut session,
gas_meter,
Expand Down
95 changes: 57 additions & 38 deletions aptos-move/aptos-vm/src/aptos_vm_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ use aptos_types::{
use aptos_vm_logging::{log_schema::AdapterLogSchema, prelude::*};
use aptos_vm_types::output::VMOutput;
use fail::fail_point;
use move_binary_format::{errors::VMResult, CompiledModule};
use move_binary_format::{
errors::{Location, PartialVMError, VMResult},
CompiledModule,
};
use move_core_types::{
gas_algebra::NumArgs,
language_storage::ModuleId,
Expand All @@ -43,6 +46,8 @@ use move_vm_types::gas::UnmeteredGasMeter;
use std::sync::Arc;

pub const MAXIMUM_APPROVED_TRANSACTION_SIZE: u64 = 1024 * 1024;
pub const GAS_PAYER_FLAG_BIT: u64 = 1u64 << 63; // MSB of sequence number is used to flag a gas payer tx


/// A wrapper to make VMRuntime standalone
pub struct AptosVMImpl {
Expand Down Expand Up @@ -502,28 +507,19 @@ impl AptosVMImpl {
.or_else(|err| convert_prologue_error(transaction_validation, err, log_context))
}

/// Run the epilogue of a transaction by calling into `EPILOGUE_NAME` function stored
/// in the `ACCOUNT_MODULE` on chain.
pub(crate) fn run_success_epilogue(
fn run_epiloque(
&self,
session: &mut SessionExt,
gas_remaining: Gas,
txn_data: &TransactionMetadata,
log_context: &AdapterLogSchema,
) -> Result<(), VMStatus> {
fail_point!("move_adapter::run_success_epilogue", |_| {
Err(VMStatus::Error(
StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR,
None,
))
});

let transaction_validation = self.transaction_validation();
transaction_validation: &TransactionValidation,
) -> VMResult<()> {
let txn_sequence_number = txn_data.sequence_number();
let txn_gas_price = txn_data.gas_unit_price();
let txn_max_gas_units = txn_data.max_gas_amount();
session
.execute_function_bypass_visibility(
if txn_sequence_number & GAS_PAYER_FLAG_BIT == 0 {
gerben-stavenga marked this conversation as resolved.
Show resolved Hide resolved
// Regular tx
session.execute_function_bypass_visibility(
&transaction_validation.module_id(),
&transaction_validation.user_epilogue_name,
// TODO: Deprecate this once we remove gas currency on the Move side.
Expand All @@ -537,8 +533,50 @@ impl AptosVMImpl {
]),
&mut UnmeteredGasMeter,
)
.map(|_return_vals| ())
.map_err(expect_no_verification_errors)
} else {
// Gas payer tx
let gas_payer = *txn_data.secondary_signers.last().ok_or_else(|| {
PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't use UNKNOWN_INVARIANT_VIOLATION_ERROR here as this case can happen if user sets the gas payer bit for a normal tx with no secondary signers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's invariant error, as we shouldn't even get to the epilogue if the gas payer account is missing. Was tempted to do unwrap but decided invariant error is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a proper abort code in the multi_agent prologue to verify this

Copy link
Contributor

Choose a reason for hiding this comment

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

We're being a bit strict and trying to avoid invariant violation as much as possible. cc @runtian-zhou regarding whether this is a legit case

.finish(Location::Undefined)
})?;
session.execute_function_bypass_visibility(
&transaction_validation.module_id(),
&transaction_validation.user_epilogue_gas_payer_name,
movekevin marked this conversation as resolved.
Show resolved Hide resolved
// TODO: Deprecate this once we remove gas currency on the Move side.
vec![],
serialize_values(&vec![
MoveValue::Signer(txn_data.sender),
MoveValue::Address(gas_payer),
MoveValue::U64(txn_sequence_number),
MoveValue::U64(txn_gas_price.into()),
MoveValue::U64(txn_max_gas_units.into()),
MoveValue::U64(gas_remaining.into()),
]),
&mut UnmeteredGasMeter,
)
}
.map(|_return_vals| ())
.map_err(expect_no_verification_errors)
}

/// Run the epilogue of a transaction by calling into `EPILOGUE_NAME` function stored
/// in the `ACCOUNT_MODULE` on chain.
pub(crate) fn run_success_epilogue(
&self,
session: &mut SessionExt,
gas_remaining: Gas,
txn_data: &TransactionMetadata,
log_context: &AdapterLogSchema,
) -> Result<(), VMStatus> {
fail_point!("move_adapter::run_success_epilogue", |_| {
Err(VMStatus::Error(
StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR,
None,
))
});

let transaction_validation = self.transaction_validation();
self.run_epiloque(session, gas_remaining, txn_data, transaction_validation)
.or_else(|err| convert_epilogue_error(transaction_validation, err, log_context))
}

Expand All @@ -552,26 +590,7 @@ impl AptosVMImpl {
log_context: &AdapterLogSchema,
) -> Result<(), VMStatus> {
let transaction_validation = self.transaction_validation();
let txn_sequence_number = txn_data.sequence_number();
let txn_gas_price = txn_data.gas_unit_price();
let txn_max_gas_units = txn_data.max_gas_amount();
session
.execute_function_bypass_visibility(
&transaction_validation.module_id(),
&transaction_validation.user_epilogue_name,
// TODO: Deprecate this once we remove gas currency on the Move side.
vec![],
serialize_values(&vec![
MoveValue::Signer(txn_data.sender),
MoveValue::U64(txn_sequence_number),
MoveValue::U64(txn_gas_price.into()),
MoveValue::U64(txn_max_gas_units.into()),
MoveValue::U64(gas_remaining.into()),
]),
&mut UnmeteredGasMeter,
)
.map(|_return_vals| ())
.map_err(expect_no_verification_errors)
self.run_epiloque(session, gas_remaining, txn_data, transaction_validation)
.or_else(|e| {
expect_only_successful_execution(
e,
Expand Down
8 changes: 4 additions & 4 deletions aptos-move/aptos-vm/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ pub fn convert_prologue_error(
},
(category, reason) => {
speculative_error!(
log_context,
gerben-stavenga marked this conversation as resolved.
Show resolved Hide resolved
format!("[aptos_vm] Unexpected prologue Move abort: {:?}::{:?} (Category: {:?} Reason: {:?})",
location, code, category, reason),
);
log_context,
format!("[aptos_vm] Unexpected prologue Move abort: {:?}::{:?} (Category: {:?} Reason: {:?})",
location, code, category, reason),
);
return Err(VMStatus::Error(
StatusCode::UNEXPECTED_ERROR_FROM_KNOWN_MOVE_FUNCTION,
None,
Expand Down
72 changes: 72 additions & 0 deletions aptos-move/e2e-move-tests/src/tests/scripts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use aptos_language_e2e_tests::account::TransactionBuilder;
use aptos_types::{
account_address::AccountAddress,
account_config::CoinStoreResource,
on_chain_config::FeatureFlag,
transaction::{Script, TransactionArgument},
};
use move_core_types::move_resource::MoveStructType;
Expand Down Expand Up @@ -80,3 +81,74 @@ fn read_coin(h: &MoveHarness, account: &AccountAddress) -> u64 {
.unwrap()
.coin()
}

#[test]
fn test_two_to_two_transfer_gas_payer() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some more tests around cases such as:

  1. Feature flag is not enabled.
  2. Gas payer doesn't have enough gas
  3. Extra senders without gas payer bit set.
  4. etc. Any other edge cases? Let's be as thorough as possible here to make sure there are no unforeseen issues

Copy link
Contributor

Choose a reason for hiding this comment

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

Still missing some failure cases:

  1. Gas payer doesn't have sufficient balances.
  2. Invalid gas payer signature
  3. Etc.
    I'll leave it to you to determine what can be covered with unit vs move e2e tests. We'll also have a more concise integration test later in TS as well for a true e2e.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that this code is just using multi-agent signing and reusing the epilogue. Most of these flows are tested by regular tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tests that cover (scripts/entry functions)/(no signer/single signer/multiple signers)/(gas bit/no-gas bit)(feature/no feature)

let mut h = MoveHarness::new_with_features(vec![FeatureFlag::GAS_PAYER_ENABLED], vec![]);

let alice = h.new_account_at(AccountAddress::from_hex_literal("0xa11ce").unwrap());
let bob = h.new_account_at(AccountAddress::from_hex_literal("0xb0b").unwrap());
let carol = h.new_account_at(AccountAddress::from_hex_literal("0xca501").unwrap());
let david = h.new_account_at(AccountAddress::from_hex_literal("0xda51d").unwrap());
let payer = h.new_account_at(AccountAddress::from_hex_literal("0xea51d").unwrap());

let amount_alice = 100;
let amount_bob = 200;
let amount_carol = 50;
let amount_david = amount_alice + amount_bob - amount_carol;

let build_options = aptos_framework::BuildOptions {
with_srcs: false,
with_abis: false,
with_source_maps: false,
with_error_map: false,
..aptos_framework::BuildOptions::default()
};

let package = aptos_framework::BuiltPackage::build(
common::test_dir_path("../../../move-examples/scripts/two_by_two_transfer"),
build_options,
)
.expect("building package must succeed");

let alice_start = read_coin(&h, alice.address());
let bob_start = read_coin(&h, bob.address());
let carol_start = read_coin(&h, carol.address());
let david_start = read_coin(&h, david.address());
let payer_start = read_coin(&h, payer.address());

let code = package.extract_script_code()[0].clone();
let script = Script::new(code, vec![], vec![
TransactionArgument::U64(amount_alice),
TransactionArgument::U64(amount_bob),
TransactionArgument::Address(*carol.address()),
TransactionArgument::Address(*david.address()),
TransactionArgument::U64(amount_carol),
]);

let transaction = TransactionBuilder::new(alice.clone())
.secondary_signers(vec![bob.clone(), payer.clone()])
movekevin marked this conversation as resolved.
Show resolved Hide resolved
.script(script)
.sequence_number(h.sequence_number(alice.address()) | (1u64 << 63))
.max_gas_amount(1_000_000)
.gas_unit_price(1)
.sign_multi_agent();

let output = h.executor.execute_transaction(transaction);
assert_success!(output.status().to_owned());
h.executor.apply_write_set(output.write_set());

let alice_end = read_coin(&h, alice.address());
let bob_end = read_coin(&h, bob.address());
let carol_end = read_coin(&h, carol.address());
let david_end = read_coin(&h, david.address());
let payer_end = read_coin(&h, payer.address());

// Make sure sender alice doesn't pay gas
assert_eq!(alice_start - amount_alice, alice_end);
assert_eq!(bob_start - amount_bob, bob_end);
assert_eq!(carol_start + amount_carol, carol_end);
assert_eq!(david_start + amount_david, david_end);
// Make sure payer pays
assert_eq!(payer_start - output.gas_used(), payer_end);
}
Loading