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

AA-217: Charge a penalty payment for unused execution gas limit of a UserOp #356

Merged
merged 37 commits into from
Nov 2, 2023

Conversation

forshtat
Copy link
Collaborator

@forshtat forshtat commented Oct 18, 2023

After a call, the EntryPoint refunds the account's deposit with the excess gas cost that was pre-charged.
A penalty of 10% (UNUSED_GAS_PENALTY_PERCENT) is applied on the amount of gas that is refunded.
This penalty is necessary to prevent the UserOps from reserving large parts of the gas space in the bundle but leaving it unused and preventing the bundler from including other UserOperations.

gigamesh and others added 28 commits March 4, 2023 15:14
Makes comments and formatting consistent
fix EntryPoint revert if 'postOp' reverts with short revert reason
@@ -19,6 +19,8 @@ import "./UserOperationLib.sol";
import "@openzeppelin/contracts/utils/introspection/ERC165.sol" as OpenZeppelin;
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

import "hardhat/console.sol";

Choose a reason for hiding this comment

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

I would think you forgot the console.log

Base automatically changed from remove_second_postop_retry to develop October 22, 2023 14:29
if (context.length > 0){
executionGasLimit += mUserOp.verificationGasLimit;
}
uint256 executionGasUsed = actualGas - opInfo.preOpGas;

This comment was marked as resolved.

return
}
const iterations = 10
const count = await counter.populateTransaction.gasWaster(iterations, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that a good estimate to the callGasUsed is
estimateGas({from:entryPoint, to: account.address, data: op1.callData})

if (context.length > 0){
executionGasLimit += mUserOp.verificationGasLimit;
}
uint256 executionGasUsed = actualGas - opInfo.preOpGas;

This comment was marked as resolved.

@drortirosh drortirosh merged commit 8b06d35 into develop Nov 2, 2023
8 checks passed
@drortirosh drortirosh deleted the AA-217-payment-for-unused-gas branch January 8, 2024 11:05
@jayden-sudo
Copy link
Contributor

uint256 executionGasLimit = mUserOp.callGasLimit + mUserOp.paymasterPostOpGasLimit;
uint256 executionGasUsed = actualGas - opInfo.preOpGas;
// this check is required for the gas used within EntryPoint and not covered by explicit gas limits
if (executionGasLimit > executionGasUsed) {
  uint256 unusedGas = executionGasLimit - executionGasUsed;
  uint256 unusedGasPenalty = (unusedGas * PENALTY_PERCENT) / 100;
  actualGas += unusedGasPenalty;
}

'Charging a penalty fee for unused execution gas limit of a UserOp' is good! but in the implemented code, it uses 10% and callGasLimit. I'm thinking whether it would be more reasonable to increase the 10% because it includes callGasLimit.

Using methods like eth_estimateGas to estimate the calldata gasLimit may not match the actual execution gasLimit. From my understanding, some ETH clients estimate gas fees in a more resource-efficient way, leading to an underestimation of the gas fee needed (especially in complex nested contracts with create/create2). This is one reason why many DApps fix a gas fee for certain operations. Here's a list of projects on GitHub that use fixed gas fees:

use fixed gas fees on GitHub

Additionally, the issue of inaccurate gas fee estimation is not limited to ETH clients but also influenced by contract logic and the current storage state. For example, many slots in smart contracts may or may not be initialized, and the length of dynamic linked lists greatly affects gas fee estimation. For example, during the execution of a smart contract where a linked list is looped through, if the list is periodically reset, the gas fee estimated for a length of 10 might be 50k, but when a user submits the transaction, the gas needed might only be 30k due to the list being reset. Here are some related resources:

Overcoming Gas Estimation Challenges

MetaMask Incorrectly Estimates 100,000 Gas for ERC20 Transfer

From a wallet client perspective, when a DApp provides a gas limit for a transaction, it often doesn't accept the bundler's gas estimation (because the DApp's gas limit takes into account inaccuracies in estimation and extreme cases where the user may or may not be able to successfully send a transaction). In this scenario, if the wallet client directly accepts the gas limit provided by the DApp, it could lead to penalties, which would be unfair to the user.

(I may not have organized the information very clearly, but I believe my ideas are expressed clearly.)

Wating for response ^^

@drortirosh
Copy link
Contributor

drortirosh commented Feb 3, 2024

The penalty for unused gaslimit was selected as a balance between a mechanism to prevent large unused gas blocks within a bundle, and overcharging a user that added a "cushion" to compensate for possible gas usage fluctuations.
There is no exact "right" or "wrong" here. A 10% overcharge seems reasonable for user operations.
As a deterrent of abuse (forcing bundlers to leave a large portion of the bundle empty) - only time could tell...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants