feat: some charge gas optimisations #664

aleksuss · 2023-01-24T10:59:31Z

Description

The PR adds some gas cost optimisations by decreasing the number of reads/saves calls using cache.

Performance / NEAR gas cost considerations

Gas costs should be decreased.

Testing

Units and integrational tests cover the changes.

engine-tests/src/tests/sanity.rs

birchmd

Looks to me like the balance used to cover the gas limit is not deducted from the user's account before executing the transaction.

Could you add a test where the user attempts a double-spend in this way? The set up is that the user has enough ETH to cover their gas limit, and enough ETH to cover a transfer they want to do, but not enough ETH to cover both. The expected behaviour is that the EVM transaction reverts with OutOfFunds when the transfer is attempted (the Near transaction is still a success so the user is charged for the gas they consumed in attempting the transfer; it is only EVM transaction that fails).

mrLSD · 2023-02-06T17:25:30Z

engine/src/engine.rs

-            .ok_or(GasPaymentError::OutOfFund)?;
-
-        set_balance(&mut self.io, sender, &new_balance);
+        if balance < prepaid_amount {


The logic was changed. It's not similar to:

get_balance(&self.io, sender) .checked_sub(prepaid_amount)

Correct is:

if balance <= prepaid_amount { ...

Correct, the gas must be deducted THEN stored in memory. From there Sputnik MUST get the balance from memory, and no longer from storage in the basic method of the Backend impl for Engine. That also is missing.

engine/src/engine.rs

joshuajbouw · 2023-02-06T18:16:41Z

engine/src/engine.rs

-            .ok_or(GasPaymentError::OutOfFund)?;
-
-        set_balance(&mut self.io, sender, &new_balance);
+        if balance < prepaid_amount {


Correct, the gas must be deducted THEN stored in memory. From there Sputnik MUST get the balance from memory, and no longer from storage in the basic method of the Backend impl for Engine. That also is missing.

mrLSD · 2023-02-06T23:40:43Z

engine/src/engine.rs

@@ -431,13 +431,12 @@ impl<'env, I: IO + Copy, E: Env> Engine<'env, I, E> {
            .checked_mul(effective_gas_price)


if effective_gas_price is zero, we can do a bit optimize it:

prepaid_amount - in that case always = 0, we don't need mul it

new_balance is the same as balance, we don't need sub it

we don't need cache it, as the balance was not changed

I don't think that checks are much much cheaper than mul and sub. IMO, it's not worse it.

And the goal of this PR is to reduce the number of operations with storage.

So, we do not need to do mathematical calculations, but you still need to do these calculations because there is no benefit from this? Given that these mathematical operations are strict with verification (checked)?

What exactly concerns this PR is that there is no need to update the cache. It's definitely a redundant operation.

You just reduce 2 strict math operations, and 1 memory operation. Why it's not cheaper?

You just reduce 2 strict math operations, and 1 memory operation. Why it's not cheaper?

Because checking values is not free.

Not free. but 4 math instructions are cheaper and memory operation flow than 1 JUMP instruction?!!

When using the Aurora+, zero gas operations are common. In this case, update_cached_balance is a guaranteed read of the Storage. And it is not needed in that particular case.

joshuajbouw

I would like to request one last test, just to ensure that Sputnik is getting the correct information. May sound redundant, but will protect us from potential changes to Engine::basic which could cause double spends:

Charge gas
Use method Engine::basic to get the balance (nonce is irrelevant for this test)
Assert gas is deducted from basic

engine-tests/src/tests/sanity.rs

birchmd

One question I have about this: it's supposed to be a gas optimization, but none of the benchmark tests (repro, uniswap, etc) needed to be updated after this change. So did this optimization really have an impact on our gas usage?

aleksuss · 2023-02-08T12:23:30Z

I should mention before. Upcoming commit brings this changes.

aleksuss · 2023-02-08T12:40:54Z

engine-precompiles/src/promise_result.rs

@@ -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);


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

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.

aleksuss · 2023-02-08T13:46:07Z

One question I have about this: it's supposed to be a gas optimization, but none of the benchmark tests (repro, uniswap, etc) needed to be updated after this change. So did this optimization really have an impact on our gas usage?

Yes. As I said, the last commit brings some TGas decreasing. And I think a more significant decrease in the costs we would see in the process of working the aurora because, in the tests, we don't use the cache effectively (the first call gets value from the storage rather than from the cache).

birchmd · 2023-02-08T13:49:41Z

in the tests, we don't use the cache effectively

Could you elaborate on this point more? I thought the tests should be accurate compared with the production deployment of the contract because they are both running in the Near runtime.

joshuajbouw · 2023-02-09T08:37:19Z

in the tests, we don't use the cache effectively

Could you elaborate on this point more?

I'm curious about this as well, if so, then tests would need to be updated to accurately reflect this. We should be seeing gas reductions though.

aleksuss · 2023-02-09T08:39:10Z

in the tests, we don't use the cache effectively

Could you elaborate on this point more?

I'm curious about this as well, if so, then tests would need to be updated to accurately reflect this. We should be seeing gas reductions though.

No. It's ok. It was my misunderstanding of how exactly the contract works.

aleksuss · 2023-02-09T11:41:05Z

Some comparisons. First value is a result from repro_* test from develop branch and second one is from current branch.

repro_GdASJ3KESs:
122721043002795 - 122596036542627 = 125006460168 
repro_FRcorNv:
181030562726220 - 180905374436976 = 125188289244
repro_D98vwmi:
182732573474634 - 182607493789182 = 125079685452 
repro_8ru7VEA:
223178543337084 - 223056899480688 = 121643856396
repro_Emufid2:
306428381815360 - 306038787160204 = 389594655156
repro_5bEgfRQ:
657955355677707 - 657565031237979 = 390324439728

joshuajbouw · 2023-07-19T02:06:24Z

I feel like we should go forward with this, if possible, for the release @aleksuss thoughts?

aleksuss requested a review from joshuajbouw as a code owner January 24, 2023 10:59

joshuajbouw suggested changes Feb 1, 2023

View reviewed changes

engine-tests/src/tests/sanity.rs Outdated Show resolved Hide resolved

aleksuss marked this pull request as draft February 1, 2023 10:35

joshuajbouw force-pushed the chore/aleksuss/charge_gas_optim branch from e45e098 to 19f3a12 Compare February 1, 2023 15:10

aleksuss changed the base branch from feat/erc20-gas-token to develop February 6, 2023 15:39

aleksuss force-pushed the chore/aleksuss/charge_gas_optim branch from b30b5db to 3a22fb0 Compare February 6, 2023 15:40

aleksuss changed the title ~~chore: some charge gas optimisations~~ feat: some charge gas optimisations Feb 6, 2023

aleksuss requested review from birchmd and mrLSD February 6, 2023 15:51

aleksuss marked this pull request as ready for review February 6, 2023 15:51

birchmd reviewed Feb 6, 2023

View reviewed changes

mrLSD reviewed Feb 6, 2023

View reviewed changes

joshuajbouw suggested changes Feb 6, 2023

View reviewed changes

mrLSD requested changes Feb 6, 2023

View reviewed changes

joshuajbouw suggested changes Feb 7, 2023

View reviewed changes

joshuajbouw suggested changes Feb 8, 2023

View reviewed changes

engine-tests/src/tests/sanity.rs Show resolved Hide resolved

engine-tests/src/tests/sanity.rs Show resolved Hide resolved

birchmd reviewed Feb 8, 2023

View reviewed changes

aleksuss added 5 commits February 8, 2023 13:26

feat: some gas costs optimisation

066fb35

chore: save balance in cache before transaction

9c148ed

chore: add test for checking cached balance

095c728

chore: add test for checking double spending

a8afee1

chore: add post transaction balance validation

ee56b0a

aleksuss force-pushed the chore/aleksuss/charge_gas_optim branch from 76452c1 to 9c91d91 Compare February 8, 2023 12:29

aleksuss commented Feb 8, 2023

View reviewed changes

chore: use cached nonce

3b46826

aleksuss force-pushed the chore/aleksuss/charge_gas_optim branch from 9c91d91 to 3b46826 Compare February 8, 2023 12:53

aleksuss added the C-research Category: Research, may not end up as a feature. label Mar 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: some charge gas optimisations #664

feat: some charge gas optimisations #664

aleksuss commented Jan 24, 2023 •

edited

Loading

birchmd left a comment •

edited

Loading

mrLSD Feb 6, 2023

joshuajbouw Feb 6, 2023

joshuajbouw Feb 6, 2023

mrLSD Feb 6, 2023

aleksuss Feb 7, 2023

aleksuss Feb 7, 2023

mrLSD Feb 7, 2023

mrLSD Feb 7, 2023

aleksuss Feb 7, 2023

mrLSD Feb 7, 2023 •

edited

Loading

joshuajbouw left a comment •

edited

Loading

birchmd left a comment

aleksuss commented Feb 8, 2023

aleksuss Feb 8, 2023

birchmd Feb 8, 2023

aleksuss commented Feb 8, 2023

birchmd commented Feb 8, 2023

joshuajbouw commented Feb 9, 2023 •

edited

Loading

aleksuss commented Feb 9, 2023

aleksuss commented Feb 9, 2023

joshuajbouw commented Jul 19, 2023

		@@ -431,13 +431,12 @@ impl<'env, I: IO + Copy, E: Env> Engine<'env, I, E> {
		.checked_mul(effective_gas_price)

feat: some charge gas optimisations #664

Are you sure you want to change the base?

feat: some charge gas optimisations #664

Conversation

aleksuss commented Jan 24, 2023 • edited Loading

Description

Performance / NEAR gas cost considerations

Testing

birchmd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrLSD Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

joshuajbouw left a comment • edited Loading

Choose a reason for hiding this comment

birchmd left a comment

Choose a reason for hiding this comment

aleksuss commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksuss commented Feb 8, 2023

birchmd commented Feb 8, 2023

joshuajbouw commented Feb 9, 2023 • edited Loading

aleksuss commented Feb 9, 2023

aleksuss commented Feb 9, 2023

joshuajbouw commented Jul 19, 2023

aleksuss commented Jan 24, 2023 •

edited

Loading

birchmd left a comment •

edited

Loading

mrLSD Feb 7, 2023 •

edited

Loading

joshuajbouw left a comment •

edited

Loading

joshuajbouw commented Feb 9, 2023 •

edited

Loading