AA-173: Remove simulation and view-only methods from the EntryPoint contract #321

forshtat · 2023-08-01T12:08:45Z

No description provided.

…revert reason

Makes comments and formatting consistent

fix EntryPoint revert if 'postOp' reverts with short revert reason

.github/workflows/build.yml

test/anvil/entrypointsimulations.test.ts

drortirosh · 2023-08-02T10:27:06Z

test/anvil/entrypointsimulations.test.ts

+        maxFeePerGas: 0
+      }, accountOwner1, entryPoint)
+      // must succeed with enough verification gas.
+      await simulateValidation(op0, entryPoint.address, { gas: '0xF4240' })


The third parameter of simulateValidation is a transaction details object and is fed directly to the eth_call RPC method as {...tx}. It does not pass through the Ethers.js where gasLimit?: string | number | BigNumber; is translated into a hex gas field.

drortirosh · 2023-08-02T10:33:51Z

test/anvil/entrypointsimulations.test.ts

+      expect(error.message).to.match(/initCode failed or OOG/, error)
+    })
+
+    // TODO: this test is impossible to do with the "state override" approach


since anvil does support debug_traceCall, it is possible to write it better.
but this test is very partial.
should instead say:
for actual opcode and storage rule restrictions, see the reference bundler ValidationManager

Added comment.

drortirosh · 2023-08-02T10:36:28Z

test/anvil/entrypointsimulations.test.ts

+    })
+  })
+
+  // describe('#simulateHandleOp', () => {


the one thing missing from simulateHandleOp (which is why we need this "Simulation" contract) is to return the actualGasUsed and success/revert reason of actual call.
they are dumped in an event deep inside innerHandleOp, and the only simple way to propagate them out to the calling "simulateHandleOp" is using storage - a big no-no for a real production code, but good enough for simulate function.

drortirosh · 2023-08-03T14:11:20Z

contracts/interfaces/IEntryPoint.sol

@@ -87,57 +87,17 @@ interface IEntryPoint is IStakeManager, INonceManager {
     *                  so a failure can be attributed to the correct entity.
     */
    error FailedOp(uint256 opIndex, string reason);
+    error FailedOp2(address opIndex, string reason);


what is this for?

drortirosh · 2023-08-03T16:10:09Z

also, to fix the code-coverage, check the COVERAGE env. variable to skip the failed test

drortirosh · 2023-08-03T14:20:41Z

test/entrypoint.test.ts

-        await expect(entryPoint.simulateValidation(userOp, { gasLimit: 1e6 }))
-          .to.revertedWith('ValidationResult')
+        const snapshot = await ethers.provider.send('evm_snapshot', [])
+        await entryPoint.simulateValidation(userOp, { gasLimit: 1e6 })


use callStatic, and no need for snapshot/revert

drortirosh · 2023-08-03T14:32:00Z

test/testutils.ts

-}
-
-export async function deployEntryPoint (provider = ethers.provider): Promise<EntryPoint> {
+export async function deployEntryPointSimulations (provider = ethers.provider): Promise<EntryPointSimulations> {


why rename the method? leave the name "deployEntryPoint", and it would avoid a lot of source modifications...

drortirosh · 2023-08-03T14:32:34Z

test/entrypoint.test.ts

-      await expect(entryPoint.simulateValidation(badOp, { gasLimit: 3e5 }))
-        .to.revertedWith('ValidationResult')
+      const snapshot = await ethers.provider.send('evm_snapshot', [])
+      await entryPoint.simulateValidation(badOp, { gasLimit: 3e5 })


use callStatic, and no need for snapshot/revert

drortirosh · 2023-08-15T13:32:36Z

contracts/core/EntryPointSimulations.sol

+                aggregator,
+                _getStakeInfo(aggregator)
+            );
+            return ValidationResult(


better "merge" the 2 separate struct returns.

drortirosh · 2023-08-15T13:33:57Z

contracts/core/EntryPointSimulations.sol

+
+    function _simulationOnlyValidations(
+        UserOperation calldata userOp
+    ) private view {


drortirosh · 2023-08-16T16:28:52Z

Comments by @yoavw :

A couple of questions:

I don't see any gas measurements in the EntryPointSimulations so I'm not sure I understand how it works. I understand it's now an eth_call rather than traceCall, so how are the different gas parameters passed to the caller?
Did everyone review it and look for redpill attacks? Let's all make sure there's no way to detect simulation due to this change.
In simulateHandleOp there are numberMarker() calls. I thought we removed tracing for this, and just use eth_call. Are these markers still used for something?
Part of our censorship resistance is that if bundlers are censoring a user, the user can always submit a single-op bundle directly. Does this method make it possible for wallets to estimate gas without a running a full bundler? Do public nodes allow the required state injection for this simulation method?
We to doc the new method in the EIP, and maybe also a separate doc about simulation to communicate the change to all bundler devs.

drortirosh · 2023-08-16T16:33:37Z

gas measurements: this PR is a first step: it moves the view functions into a separate contract, and remove the need for "Revert" structure, but otherwise, doesn't change their calculation. a separate PR should change simulateHandleOp so that it could return out the actualGasUsed and success (which are currently only emitted in a log)
@shahafn - please also review the code.
numberMarkers will go away, in a next PR (as explained above)
stateOverride is a parameter of eth_call, and do not require tracing (so in a way, this change actually makes it easier for wallets to estimate gas, since for actual callGasLimit, calculation they currently require traceCall - at least for estimating first UserOp, since you can "hack" if the account is already deployed
right. need to update the EIP to accomodate with this change. @forshtat ?

shahafn · 2023-08-17T13:05:42Z

There is one way that is a potential exploit:

The new approach means that there are two different EntryPoint contracts, one for simulation and one for execution.
This means that calls to the very same function (e.g. depositTo()) may have slightly different gas costs (by a diff of ~30 gas).
This depends on the code diff between EntryPoint and EntryPointSimulations, and it may also change between each modification we make to either of them.

In the case where EntryPointSimulations.depositTo() costs 30 gas less than EntryPoint.depositTo(), for instance, an account can call depositTo() during validation 20 times, amplifying the diff to 600 gas, then emit an event. This will pass in simulation since it's cheaper, but fail on actual EntryPoint.

A possible mitigation is to allow only one call to depositTo() during validation, but I think that with gas calculation precise enough, even 30 gas diff can be abused (e.g. through emitting events).

Wdyt?

yoavw · 2023-08-17T17:55:33Z

@shahafn , @drortirosh and I discussed this a couple of days ago and I believe the conclusion was that every function in EntryPointSimulations must take at least the same amount of gas as its EntryPoint implementation. If it's marginal, add an extra SSTORE and deduct this fixed cost from the reported gas cost. This way there are no cases where something results in OOG on chain after passing simulation successfully. We're ok if OOG happens during simulation - we just increase the gas limit. What we're trying to prevent is on-chain OOG.

don't assume EntryPointSimulations is installed.

gigamesh and others added 18 commits March 4, 2023 15:14

Makes comments and formatting consistent

ebbf21a

@inheritdoc IEntryPoint

64cbdf6

Reverts contracts/test/TestHelpers.sol

d43bc94

fix: entrypoint does not revert even first postOp reverts with short …

105d122

…revert reason

lint

3a6b0a5

gas-report

66688cb

Merge branch 'next_v0.7' into gigamesh/comments

b56ef7b

Update IEntryPoint.sol - add missing 'INonceManager'

74ed53b

Update EntryPoint.sol - fix lint errors

649daf8

Update gas-checker.txt manually

aa20d5d

Update gas-checker.txt - add newline for the 'diff'

e3111fe

Merge pull request #235 from gigamesh/gigamesh/comments

10f0502

Makes comments and formatting consistent

Merge branch 'next_v0.7' into develop

6a5ce09

Update gas-checker.txt manually

8116f81

Merge pull request #293 from leekt/develop

74a3c37

fix EntryPoint revert if 'postOp' reverts with short revert reason

WIP: add 'anvil' node to github actions; first test with state override

c0dd3ff

Skip anvil-specific test when running against a hardhat node

38c14be

WIP: move simulation-related tests to anvil and run with state override

9a9f380

forshtat changed the title ~~WIP: Separate simulation entry point~~ AA-173: Remove simulation and view-only methods from the EntryPoint contract Aug 1, 2023

drortirosh reviewed Aug 2, 2023

View reviewed changes

forshtat added 7 commits August 2, 2023 18:50

Deploy EntryPointSimulations for Hardhat tests - temporary solution

1a701cc

Uncomment 'simulateHandleOp' test

5ecd0f4

Update gas checks

bc69fe7

Mine and 'evm_revert' a call to "simulateValidation"

d16f337

Remove also internal simulation-only functions

5b1fb1a

Fix: even with "state override" keep 'validateSenderAndPaymaster' revert

31dcc37

Update gas checks

29d56f9

drortirosh reviewed Aug 3, 2023

View reviewed changes

Address PR comments

856eb9b

Base automatically changed from next_v0.7 to tmpdev August 15, 2023 11:44

Base automatically changed from tmpdev to develop August 15, 2023 12:07

forshtat added 2 commits August 15, 2023 14:10

Merge branch 'develop' into separate_simulation_entry_point

2d47f78

Update 'gas-checker.txt'

d930552

drortirosh reviewed Aug 15, 2023

View reviewed changes

forshtat added 2 commits August 15, 2023 21:49

Address PR comments

99a74ce

Missed file

17e39df

drortirosh and others added 4 commits August 17, 2023 21:19

verify that EntryPoint calls never cost less during simulation (#328)

887bfab

Add comment that 'EntryPointSimulations' should not be deployed

74dc7fb

Merge branch 'develop' into separate_simulation_entry_point

4887d64

waste some gas

e2f4703

drortirosh force-pushed the separate_simulation_entry_point branch from fb0261d to e2f4703 Compare August 21, 2023 14:59

drortirosh added 2 commits August 29, 2023 00:13

use latest hardhat, with support for eth_call state-override

28e0fe7

lints, coverage

c254faf

drortirosh force-pushed the separate_simulation_entry_point branch from f43a535 to c254faf Compare August 29, 2023 09:34

yoavw mentioned this pull request Sep 2, 2023

updated EIP diagrams #335

Merged

simulator should not be deployed

ac5a4c9

drortirosh force-pushed the separate_simulation_entry_point branch from a9caa9c to ac5a4c9 Compare September 6, 2023 13:03

drortirosh added 2 commits September 6, 2023 17:45

use "simulateValidation" helper, with state-override

4c8a7f1

don't assume EntryPointSimulations is installed.

update EIP for simulation

3a6b270

drortirosh requested a review from shahafn September 10, 2023 15:54

shahafn approved these changes Sep 12, 2023

View reviewed changes

drortirosh merged commit e996139 into develop Sep 12, 2023
8 checks passed

mmv08 mentioned this pull request Jan 25, 2024

Evaluate 4337 Module Changes For EntryPoint 0.7.0 safe-global/safe-modules#215

Closed

drortirosh deleted the separate_simulation_entry_point branch January 28, 2024 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AA-173: Remove simulation and view-only methods from the EntryPoint contract #321

AA-173: Remove simulation and view-only methods from the EntryPoint contract #321

forshtat commented Aug 1, 2023

drortirosh Aug 2, 2023

forshtat Aug 3, 2023

drortirosh Aug 2, 2023

forshtat Aug 3, 2023

drortirosh Aug 2, 2023

drortirosh Aug 3, 2023

forshtat Aug 3, 2023

drortirosh commented Aug 3, 2023

drortirosh Aug 3, 2023

forshtat Aug 15, 2023

drortirosh Aug 3, 2023

forshtat Aug 15, 2023

drortirosh Aug 3, 2023

forshtat Aug 15, 2023

drortirosh Aug 15, 2023

forshtat Aug 15, 2023

drortirosh Aug 15, 2023

forshtat Aug 15, 2023

drortirosh commented Aug 16, 2023

drortirosh commented Aug 16, 2023

shahafn commented Aug 17, 2023

yoavw commented Aug 17, 2023

AA-173: Remove simulation and view-only methods from the EntryPoint contract #321

AA-173: Remove simulation and view-only methods from the EntryPoint contract #321

Conversation

forshtat commented Aug 1, 2023

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

drortirosh commented Aug 3, 2023

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

drortirosh commented Aug 16, 2023

drortirosh commented Aug 16, 2023

shahafn commented Aug 17, 2023

yoavw commented Aug 17, 2023