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

Jump to bottom

AA-216 Proposal: Packing UserOperation and paymaster gas limits #363

Merged
merged 28 commits into from
Jan 8, 2024
Merged

AA-216 Proposal: Packing UserOperation and paymaster gas limits #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
3181436
Packing gas limits
shahafn Nov 2, 2023
1e2fe50
Packing paymaster gas fields into paymasterAndData
shahafn Nov 30, 2023
bef7e4d
bug fixes
shahafn Dec 3, 2023
41bad54
Fixing entrypoint tests
shahafn Dec 4, 2023
f27b2d6
Fixing (almost all) tests
shahafn Dec 4, 2023
287fe60
Fixing TokenPaymaster tests
shahafn Dec 4, 2023
b2a9e61
Fixing safe tests
shahafn Dec 4, 2023
46fb3ba
Lint
shahafn Dec 4, 2023
7494872
Fixing gas-calc, tsc
shahafn Dec 5, 2023
0cc1d67
Lint
shahafn Dec 5, 2023
6ffd042
ethers tsc
shahafn Dec 5, 2023
56a5710
gas calc again
shahafn Dec 5, 2023
2e3ceff
Fixes
shahafn Dec 20, 2023
d95187b
Rename UserOperation to PackedUserOperation
shahafn Dec 20, 2023
415ef69
Gas calc
shahafn Dec 20, 2023
ffdb104
Merge remote-tracking branch 'origin/develop' into packed-gas-limits
shahafn Dec 21, 2023
7b6949a
Merge branch 'develop' into packed-gas-limits
drortirosh Jan 1, 2024
47a57fa
remove debug comments
drortirosh Jan 1, 2024
a54d10b
Merge branch 'develop' into packed-gas-limits
drortirosh Jan 3, 2024
08ef38e
changes
drortirosh Jan 3, 2024
052a8ad
fix tokenpaymaster test
drortirosh Jan 3, 2024
87a8eac
Merge pull request #393 from eth-infinitism/dror-packeduserop
drortirosh Jan 3, 2024
277c379
fixed pr comments
drortirosh Jan 3, 2024
effa64f
add paymaster constants to BasePaymaster
drortirosh Jan 4, 2024
8ff7490
PR fix
shahafn Jan 7, 2024
24cf39e
Removing redundant initializer
shahafn Jan 7, 2024
b9504f4
gas calc
shahafn Jan 7, 2024
6252387
Fix test
shahafn Jan 7, 2024
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
6 changes: 3 additions & 3 deletions contracts/core/BaseAccount.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import "./UserOperationLib.sol";
* Specific account implementation should inherit it and provide the account-specific logic.
*/
abstract contract BaseAccount is IAccount {
using UserOperationLib for UserOperation;
using UserOperationLib for PackedUserOperation;

/**
* Return value in case of signature failure, with no time-range.
Expand Down Expand Up @@ -48,7 +48,7 @@ abstract contract BaseAccount is IAccount {
* to pay for the user operation.
*/
function validateUserOp(
UserOperation calldata userOp,
PackedUserOperation calldata userOp,
bytes32 userOpHash,
uint256 missingAccountFunds
) external virtual override returns (uint256 validationData) {
Expand Down Expand Up @@ -83,7 +83,7 @@ abstract contract BaseAccount is IAccount {
* Note that the validation code cannot use block.timestamp (or block.number) directly.
*/
function _validateSignature(
UserOperation calldata userOp,
PackedUserOperation calldata userOp,
bytes32 userOpHash
) internal virtual returns (uint256 validationData);

Expand Down
10 changes: 7 additions & 3 deletions contracts/core/BasePaymaster.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import "@openzeppelin/contracts/utils/introspection/IERC165.sol";
import "../interfaces/IPaymaster.sol";
import "../interfaces/IEntryPoint.sol";
import "./Helpers.sol";

import "./UserOperationLib.sol";
/**
* Helper class for creating a paymaster.
* provides helper methods for staking.
Expand All @@ -17,6 +17,10 @@ import "./Helpers.sol";
abstract contract BasePaymaster is IPaymaster, Ownable {
IEntryPoint public immutable entryPoint;

uint256 internal constant PAYMASTER_VALIDATION_GAS_OFFSET = UserOperationLib.PAYMASTER_VALIDATION_GAS_OFFSET;
uint256 internal constant PAYMASTER_POSTOP_GAS_OFFSET = UserOperationLib.PAYMASTER_POSTOP_GAS_OFFSET;
uint256 internal constant PAYMASTER_DATA_OFFSET = UserOperationLib.PAYMASTER_DATA_OFFSET;

constructor(IEntryPoint _entryPoint) Ownable(msg.sender) {
_validateEntryPointInterface(_entryPoint);
entryPoint = _entryPoint;
Expand All @@ -30,7 +34,7 @@ abstract contract BasePaymaster is IPaymaster, Ownable {

/// @inheritdoc IPaymaster
function validatePaymasterUserOp(
UserOperation calldata userOp,
PackedUserOperation calldata userOp,
bytes32 userOpHash,
uint256 maxCost
) external override returns (bytes memory context, uint256 validationData) {
Expand All @@ -45,7 +49,7 @@ abstract contract BasePaymaster is IPaymaster, Ownable {
* @param maxCost - The maximum cost of the user operation.
*/
function _validatePaymasterUserOp(
UserOperation calldata userOp,
PackedUserOperation calldata userOp,
bytes32 userOpHash,
uint256 maxCost
) internal virtual returns (bytes memory context, uint256 validationData);
Expand Down
86 changes: 37 additions & 49 deletions contracts/core/EntryPoint.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
*/
contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, OpenZeppelin.ERC165 {

using UserOperationLib for UserOperation;
using UserOperationLib for PackedUserOperation;

SenderCreator private senderCreator = new SenderCreator();

Expand Down Expand Up @@ -71,7 +71,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
*/
function _executeUserOp(
uint256 opIndex,
UserOperation calldata userOp,
PackedUserOperation calldata userOp,
UserOpInfo memory opInfo
)
internal
Expand Down Expand Up @@ -141,7 +141,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,

/// @inheritdoc IEntryPoint
function handleOps(
UserOperation[] calldata ops,
PackedUserOperation[] calldata ops,
address payable beneficiary
) public nonReentrant {
uint256 opslen = ops.length;
Expand Down Expand Up @@ -183,7 +183,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
uint256 totalOps = 0;
for (uint256 i = 0; i < opasLen; i++) {
UserOpsPerAggregator calldata opa = opsPerAggregator[i];
UserOperation[] calldata ops = opa.userOps;
PackedUserOperation[] calldata ops = opa.userOps;
IAggregator aggregator = opa.aggregator;

//address(1) is special marker of "signature error"
Expand All @@ -207,7 +207,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
uint256 opIndex = 0;
for (uint256 a = 0; a < opasLen; a++) {
UserOpsPerAggregator calldata opa = opsPerAggregator[a];
UserOperation[] calldata ops = opa.userOps;
PackedUserOperation[] calldata ops = opa.userOps;
IAggregator aggregator = opa.aggregator;

uint256 opslen = ops.length;
Expand All @@ -234,7 +234,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
for (uint256 a = 0; a < opasLen; a++) {
UserOpsPerAggregator calldata opa = opsPerAggregator[a];
emit SignatureAggregatorChanged(address(opa.aggregator));
UserOperation[] calldata ops = opa.userOps;
PackedUserOperation[] calldata ops = opa.userOps;
uint256 opslen = ops.length;

for (uint256 i = 0; i < opslen; i++) {
Expand All @@ -254,8 +254,10 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
struct MemoryUserOp {
address sender;
uint256 nonce;
uint256 callGasLimit;
uint256 verificationGasLimit;
uint128 verificationGasLimit;
uint128 callGasLimit;
uint128 paymasterVerificationGasLimit;
uint128 paymasterPostOpGasLimit;
uint256 preVerificationGas;
address paymaster;
uint256 maxFeePerGas;
Expand Down Expand Up @@ -290,7 +292,10 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
unchecked {
// handleOps was called with gas limit too low. abort entire bundle.
if (
gasleft() < callGasLimit + mUserOp.verificationGasLimit + 5000
gasleft() <
callGasLimit +
mUserOp.paymasterPostOpGasLimit +
Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point we already have a context, so if it is empty the postOp will not be called. I think it may make sense to allow the transaction to go on in this case, instead of always adding paymasterPostOpGasLimit to the required gas left?
Say, the UserOp have requested 1'000'000 gas for a postOp but it does not need it any more, why should this check fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

why should we (entrypoint) waste gas to check it? it is the paymaster's job to put a valid postOpGasLimit, even if context is empty.
There is another edge case: validatePaymasterUserOp() returns a context, and the postOpGasLimit` is zero: in this case, we still execute the callData and revert the postOp (on OOG) - again, because it is not the EntryPoint's job to validate the userop fields.

5000
) {
assembly {
mstore(0, INNER_OUT_OF_GAS)
Expand Down Expand Up @@ -325,7 +330,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,

/// @inheritdoc IEntryPoint
function getUserOpHash(
UserOperation calldata userOp
PackedUserOperation calldata userOp
) public view returns (bytes32) {
return
keccak256(abi.encode(userOp.hash(), address(this), block.chainid));
Expand All @@ -337,25 +342,26 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
* @param mUserOp - The memory user operation.
*/
function _copyUserOpToMemory(
UserOperation calldata userOp,
PackedUserOperation calldata userOp,
MemoryUserOp memory mUserOp
) internal pure {
mUserOp.sender = userOp.sender;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not have the entire UserOperation => MemoryUserOp conversion in the UserOperationLib library? Construction of a MemoryUserOp takes a lot of space and will call to the UserOperationLib at least twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

This copy is an internal optimization of EntryPoint. we might even remove copying some fields if we find they are not needed (e.g. used exactly once)
UserOperationLib is for general-use functions, that are expected to be used even outside the EntryPoint itself.

mUserOp.nonce = userOp.nonce;
mUserOp.callGasLimit = userOp.callGasLimit;
mUserOp.verificationGasLimit = userOp.verificationGasLimit;
(mUserOp.verificationGasLimit, mUserOp.callGasLimit) = UserOperationLib.unpackAccountGasLimits(userOp.accountGasLimits);
mUserOp.preVerificationGas = userOp.preVerificationGas;
mUserOp.maxFeePerGas = userOp.maxFeePerGas;
mUserOp.maxPriorityFeePerGas = userOp.maxPriorityFeePerGas;
bytes calldata paymasterAndData = userOp.paymasterAndData;
if (paymasterAndData.length > 0) {
require(
paymasterAndData.length >= 20,
paymasterAndData.length >= UserOperationLib.PAYMASTER_DATA_OFFSET,
"AA93 invalid paymasterAndData"
);
mUserOp.paymaster = address(bytes20(paymasterAndData[:20]));
(mUserOp.paymaster, mUserOp.paymasterVerificationGasLimit, mUserOp.paymasterPostOpGasLimit) = UserOperationLib.unpackPaymasterStaticFields(paymasterAndData);
} else {
mUserOp.paymaster = address(0);
mUserOp.paymasterVerificationGasLimit = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Solidity structs are pre-initialized to all-zeros.

Copy link
Contributor

Choose a reason for hiding this comment

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

Except when one does crazy stuff like recycling memory by using the free-pointer back - which we do :) So we can't assume zeros anywhere.

Copy link
Contributor

@drortirosh drortirosh Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we can assume zeros
all storage variables in solidity are initialized to zero - either "stack" or memory variables (structs and arrays).
(there are Yul injected functions "allocate_and_zero_memory..." for that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The paymaster initialization to address(0) was there, I only added the other fields. Do you want to remove all of them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can assume zeros all storage variables in solidity are initialized to zero - either "stack" or memory variables (structs and arrays). (there are Yul injected functions "allocate_and_zero_memory..." for that)

Interesting, I wonder why they do that. Solidity always allocates new memory (containing zeros) so why do they need to explicitly write zeros? (We do, due to our asm code, but normal solidity doesn't)

mUserOp.paymasterPostOpGasLimit = 0;
}
}

Expand All @@ -367,12 +373,10 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
MemoryUserOp memory mUserOp
) internal pure returns (uint256 requiredPrefund) {
unchecked {
// When using a Paymaster, the verificationGasLimit is used also to as a limit for the postOp call.
// Our security model might call postOp eventually twice.
uint256 mul = mUserOp.paymaster != address(0) ? 2 : 1;
uint256 requiredGas = mUserOp.callGasLimit +
mUserOp.verificationGasLimit *
mul +
uint256 requiredGas = mUserOp.verificationGasLimit +
mUserOp.callGasLimit +
mUserOp.paymasterVerificationGasLimit +
mUserOp.paymasterPostOpGasLimit +
mUserOp.preVerificationGas;

requiredPrefund = requiredGas * mUserOp.maxFeePerGas;
Expand Down Expand Up @@ -430,18 +434,16 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
*/
function _validateAccountPrepayment(
uint256 opIndex,
UserOperation calldata op,
PackedUserOperation calldata op,
UserOpInfo memory opInfo,
uint256 requiredPrefund
)
internal
returns (
uint256 gasUsedByValidateAccountPrepayment,
uint256 validationData
)
{
unchecked {
uint256 preGas = gasleft();
MemoryUserOp memory mUserOp = opInfo.mUserOp;
address sender = mUserOp.sender;
_createSenderIfNeeded(opIndex, opInfo, op.initCode);
Expand Down Expand Up @@ -470,7 +472,6 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
}
senderInfo.deposit = deposit - requiredPrefund;
}
gasUsedByValidateAccountPrepayment = preGas - gasleft();
}
}

Expand All @@ -484,25 +485,15 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
* @param op - The user operation.
* @param opInfo - The operation info.
* @param requiredPreFund - The required prefund amount.
* @param gasUsedByValidateAccountPrepayment - The gas used by _validateAccountPrepayment.
*/
function _validatePaymasterPrepayment(
uint256 opIndex,
UserOperation calldata op,
PackedUserOperation calldata op,
UserOpInfo memory opInfo,
uint256 requiredPreFund,
uint256 gasUsedByValidateAccountPrepayment
uint256 requiredPreFund
) internal returns (bytes memory context, uint256 validationData) {
unchecked {
MemoryUserOp memory mUserOp = opInfo.mUserOp;
uint256 verificationGasLimit = mUserOp.verificationGasLimit;
require(
verificationGasLimit > gasUsedByValidateAccountPrepayment,
"AA41 too little verificationGas"
);
uint256 gas = verificationGasLimit -
gasUsedByValidateAccountPrepayment;

address paymaster = mUserOp.paymaster;
DepositInfo storage paymasterInfo = deposits[paymaster];
uint256 deposit = paymasterInfo.deposit;
Expand All @@ -511,7 +502,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
}
paymasterInfo.deposit = deposit - requiredPreFund;
try
IPaymaster(paymaster).validatePaymasterUserOp{gas: gas}(
IPaymaster(paymaster).validatePaymasterUserOp{gas: mUserOp.paymasterVerificationGasLimit}(
shahafn marked this conversation as resolved.
Show resolved Hide resolved
op,
opInfo.userOpHash,
requiredPreFund
Expand Down Expand Up @@ -586,7 +577,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
*/
function _validatePrepayment(
uint256 opIndex,
UserOperation calldata userOp,
PackedUserOperation calldata userOp,
UserOpInfo memory outOpInfo
)
internal
Expand All @@ -602,16 +593,14 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
uint256 maxGasValues = mUserOp.preVerificationGas |
mUserOp.verificationGasLimit |
mUserOp.callGasLimit |
mUserOp.paymasterVerificationGasLimit |
mUserOp.paymasterPostOpGasLimit |
userOp.maxFeePerGas |
userOp.maxPriorityFeePerGas;
require(maxGasValues <= type(uint120).max, "AA94 gas values overflow");

uint256 gasUsedByValidateAccountPrepayment;
uint256 requiredPreFund = _getRequiredPrefund(mUserOp);
(
gasUsedByValidateAccountPrepayment,
validationData
) = _validateAccountPrepayment(
validationData = _validateAccountPrepayment(
opIndex,
userOp,
outOpInfo,
Expand All @@ -628,14 +617,13 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
opIndex,
userOp,
outOpInfo,
requiredPreFund,
gasUsedByValidateAccountPrepayment
requiredPreFund
);
}
unchecked {
uint256 gasUsed = preGas - gasleft();

if (userOp.verificationGasLimit < gasUsed) {
if (mUserOp.verificationGasLimit + mUserOp.paymasterVerificationGasLimit < gasUsed) {
revert FailedOp(opIndex, "AA40 over verificationGasLimit");
}
outOpInfo.prefund = requiredPreFund;
Expand Down Expand Up @@ -676,7 +664,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
actualGasCost = actualGas * gasPrice;
if (mode != IPaymaster.PostOpMode.postOpReverted) {
try IPaymaster(paymaster).postOp{
gas: mUserOp.verificationGasLimit
gas: mUserOp.paymasterPostOpGasLimit
}(mode, context, actualGasCost)
// solhint-disable-next-line no-empty-blocks
{} catch {
shahafn marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -693,7 +681,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
uint256 executionGasLimit = mUserOp.callGasLimit;
// Note that 'verificationGasLimit' here is the limit given to the 'postOp' which is part of execution
if (context.length > 0){
executionGasLimit += mUserOp.verificationGasLimit;
executionGasLimit += mUserOp.paymasterPostOpGasLimit;
}
uint256 executionGasUsed = actualGas - opInfo.preOpGas;
// this check is required for the gas used within EntryPoint and not covered by explicit gas limits
Expand Down
6 changes: 3 additions & 3 deletions contracts/core/EntryPointSimulations.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ contract EntryPointSimulations is EntryPoint, IEntryPointSimulations {

/// @inheritdoc IEntryPointSimulations
function simulateValidation(
UserOperation calldata userOp
PackedUserOperation calldata userOp
)
external
returns (
Expand Down Expand Up @@ -85,7 +85,7 @@ contract EntryPointSimulations is EntryPoint, IEntryPointSimulations {

/// @inheritdoc IEntryPointSimulations
function simulateHandleOp(
UserOperation calldata op,
PackedUserOperation calldata op,
address target,
bytes calldata targetCallData
)
Expand Down Expand Up @@ -121,7 +121,7 @@ contract EntryPointSimulations is EntryPoint, IEntryPointSimulations {
}

function _simulationOnlyValidations(
UserOperation calldata userOp
PackedUserOperation calldata userOp
)
internal
view
Expand Down
Loading
Loading