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

Rethink gas spending logic #5148

Open
olonho opened this issue Nov 5, 2021 · 5 comments
Open

Rethink gas spending logic #5148

olonho opened this issue Nov 5, 2021 · 5 comments
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) T-contract-runtime Team: issues relevant to the contract runtime team T-core Team: issues relevant to the core team

Comments

@olonho
Copy link
Contributor

olonho commented Nov 5, 2021

During work on #5121 we identified two questionable places in gas metering.

  1. When transaction fails we still require (by tests) that used amount of gas is sum of promised and burnt gas. Seems more natural to zero out promised part.
  2. When charging for gas, we always burn the gas even if go the error path. It looks strange and not match with normal paying logic: no operation - no payment.
@matklad matklad added T-contract-runtime Team: issues relevant to the contract runtime team T-core Team: issues relevant to the core team labels Nov 5, 2021
@matklad
Copy link
Contributor

matklad commented Nov 5, 2021

If we protocol change this, we should also consider changing how we limit promises gas. Rather than checking gas limit on every deduct_gas operation, we can check it once, after wasm execution finishes. Note that we already postpone actual promise creation in the Rust SDK.

@bowenwang1996 bowenwang1996 added the A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) label Nov 5, 2021
@olonho
Copy link
Contributor Author

olonho commented Nov 5, 2021

It's not hard to keep exact promises gas counter IMO.

@stale
Copy link

stale bot commented Feb 4, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Feb 4, 2022
@stale
Copy link

stale bot commented May 5, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label May 5, 2022
@akhi3030 akhi3030 removed the S-stale label Jul 8, 2022
@jakmeier
Copy link
Contributor

When transaction fails we still require (by tests) that used amount of gas is sum of promised and burnt gas. Seems more natural to zero out promised part.

I agree in principle with that statement but want to make clear that it doesn't matter, since on an error, we refund prepaid - burnt and the promised gas is not relevant:

let gas_refund = if result.result.is_err() {
safe_add_gas(prepaid_gas, prepaid_exec_gas)? - result.gas_burnt
} else {
safe_add_gas(prepaid_gas, prepaid_exec_gas)? - result.gas_used
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) T-contract-runtime Team: issues relevant to the contract runtime team T-core Team: issues relevant to the core team
Projects
None yet
Development

No branches or pull requests

5 participants