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

feat: some charge gas optimisations #664

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion engine-precompiles/src/promise_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub mod costs {
use crate::prelude::types::EthGas;

/// This cost is always charged for calling this precompile.
pub const PROMISE_RESULT_BASE_COST: EthGas = EthGas::new(105);
pub const PROMISE_RESULT_BASE_COST: EthGas = EthGas::new(53);
Copy link
Member Author

Choose a reason for hiding this comment

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

@joshuajbouw @birchmd this change is a bit confusing to me. Could you check it out?

Copy link
Member

Choose a reason for hiding this comment

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

This gas cost is computed from this test. The basic idea of this test is to measure the gas cost by doing a linear regression of the actual cost (measured in Near gas) of calling this precompile for different sizes of promise data. It's actually not much of a linear regression because I just take two points and compute the line through them, but we also don't need this to be super accurate (the gas cost is only to prevent abuse) so I think that's ok.

Anyway, the cost is supposed to exclude overhead that applies to all transactions by subtracting a baseline transaction. So it is surprising to me that this change impacts this value. But the test also allows a 5% wiggle room on the value, so the change might be smaller than it looks if we were on the edge of being outside the 5% range before. Overall, I think this change is ok even if it is unexpected.

/// This is the cost per byte of promise result data.
pub const PROMISE_RESULT_BYTE_COST: EthGas = EthGas::new(1);
}
Expand Down
2 changes: 1 addition & 1 deletion engine-standalone-storage/src/sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ fn non_submit_execute<'db>(

if env.predecessor_account_id == env.current_account_id {
connector::EthConnectorContract::init_instance(io)?
.ft_on_transfer(&engine, args)?;
.ft_on_transfer(&mut engine, args)?;
} else {
engine.receive_erc20_tokens(
&env.predecessor_account_id,
Expand Down
7 changes: 3 additions & 4 deletions engine-tests/src/test_utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -857,11 +857,10 @@ pub fn panic_on_fail(status: TransactionStatus) {
pub fn assert_gas_bound(total_gas: u64, tgas_bound: u64) {
// Add 1 to round up
let tgas_used = (total_gas / 1_000_000_000_000) + 1;
assert!(
tgas_used == tgas_bound,
assert_eq!(
tgas_used, tgas_bound,
"{} Tgas is not equal to {} Tgas",
tgas_used,
tgas_bound,
tgas_used, tgas_bound
);
}

Expand Down
4 changes: 3 additions & 1 deletion engine-tests/src/test_utils/standalone/mocks/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ pub fn mint_evm_account<I: IO + Copy, E: Env>(
hex::encode(address.as_bytes())
),
};
connector.ft_on_transfer(&engine, &transfer_args).unwrap();
connector
.ft_on_transfer(&mut engine, &transfer_args)
.unwrap();

engine.apply(std::iter::once(state_change), std::iter::empty(), false);
}
Expand Down
4 changes: 2 additions & 2 deletions engine-tests/src/tests/contract_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ fn withdraw_eth() {
];
let exit_events = parse_exit_events(result, &schema);

assert!(exit_events.len() == 1);
assert_eq!(exit_events.len(), 1);
assert_eq!(&expected_event, &exit_events[0].params);

// exit to ethereum
Expand All @@ -220,7 +220,7 @@ fn withdraw_eth() {
let schema = aurora_engine_precompiles::native::events::exit_to_eth_schema();
let exit_events = parse_exit_events(result, &schema);

assert!(exit_events.len() == 1);
assert_eq!(exit_events.len(), 1);
assert_eq!(&expected_event, &exit_events[0].params);
}

Expand Down
2 changes: 1 addition & 1 deletion engine-tests/src/tests/repro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ fn repro_FRcorNv() {
block_timestamp: 1650960438774745116,
input_path: "src/tests/res/input_FRcorNv.hex",
evm_gas_used: 1239721,
near_gas_used: 181,
near_gas_used: 180,
});
}

Expand Down
41 changes: 41 additions & 0 deletions engine-tests/src/tests/sanity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,47 @@ fn test_eth_transfer_charging_gas_not_enough_balance_sim() {
);
}

#[test]
fn test_eth_double_spending_trying_sim() {
let (mut runner, mut source_account, dest_address) = initialize_transfer();
let transfer = Wei::new_u64(800_000);
let source_address = test_utils::address_from_secret_key(&source_account.secret_key);
let transaction = |nonce| {
let mut tx = test_utils::transfer(dest_address, transfer, nonce);
tx.gas_limit = 30_000.into();
tx.gas_price = GAS_PRICE.into();
tx
};

// validate pre-state
test_utils::validate_address_balance_and_nonce(
&runner,
source_address,
INITIAL_BALANCE,
INITIAL_NONCE.into(),
);
test_utils::validate_address_balance_and_nonce(&runner, dest_address, Wei::zero(), 0.into());

// do transfer
let result = runner
.submit_with_signer(&mut source_account, transaction)
.unwrap();
// The status is `OutOfFund` because we try to spend 800_000 (transfer) + 300_000 (gas prepaid)
birchmd marked this conversation as resolved.
Show resolved Hide resolved
// = 1_100_000, but initial balance is 1_000_000
assert_eq!(result.status, TransactionStatus::OutOfFund);

// validate post-state
test_utils::validate_address_balance_and_nonce(
&runner,
source_address,
// Cover the EVM gas spent on the transaction before failing. 21_000 is the base cost of
// initiating a transaction in the EVM.
INITIAL_BALANCE - Wei::new_u64(21_000 * GAS_PRICE),
(INITIAL_NONCE + 1).into(),
birchmd marked this conversation as resolved.
Show resolved Hide resolved
);
test_utils::validate_address_balance_and_nonce(&runner, dest_address, Wei::zero(), 0.into());
}

fn initialize_evm_sim() -> (state_migration::AuroraAccount, test_utils::Signer, Address) {
let aurora = state_migration::deploy_evm();
let signer = test_utils::Signer::random();
Expand Down
12 changes: 8 additions & 4 deletions engine/src/connector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,13 +322,15 @@ impl<I: IO + Copy> EthConnectorContract<I> {
}

/// Mint ETH tokens
fn mint_eth_on_aurora(
fn mint_eth_on_aurora<'env, E: Env>(
&mut self,
engine: &mut Engine<'env, I, E>,
owner_id: Address,
amount: Wei,
) -> Result<(), fungible_token::error::DepositError> {
sdk::log!("Mint {} ETH tokens for: {}", amount, owner_id.encode());
self.ft.internal_deposit_eth_to_aurora(owner_id, amount)
self.ft
.internal_deposit_eth_to_aurora(engine, owner_id, amount)
}

/// Burn ETH tokens
Expand Down Expand Up @@ -575,7 +577,7 @@ impl<I: IO + Copy> EthConnectorContract<I> {
/// ft_on_transfer callback function
pub fn ft_on_transfer<'env, E: Env>(
&mut self,
engine: &Engine<'env, I, E>,
engine: &mut Engine<'env, I, E>,
args: &NEP141FtOnTransferArgs,
) -> Result<(), error::FtTransferCallError> {
sdk::log!("Call ft_on_transfer");
Expand All @@ -590,12 +592,14 @@ impl<I: IO + Copy> EthConnectorContract<I> {
match (wei_fee, relayer) {
(fee, Some(evm_relayer_address)) if fee > ZERO_WEI => {
self.mint_eth_on_aurora(
engine,
message_data.recipient,
Wei::new(U256::from(args.amount.as_u128())) - fee,
)?;
self.mint_eth_on_aurora(evm_relayer_address, fee)?;
self.mint_eth_on_aurora(engine, evm_relayer_address, fee)?;
}
_ => self.mint_eth_on_aurora(
engine,
message_data.recipient,
Wei::new(U256::from(args.amount.as_u128())),
)?,
Expand Down
Loading