fix: use simulation to estimate gas used #11211

spypsy · 2025-01-14T11:55:54Z

Relevant to #10991 but not 'Fixes' because anvil doesn't support eth_simulateV1 so we can't yet remove all time-based inputs in L1

Fixes #11341

We now try to simulate the 'propose' & 'proposeAndClaim' transactions, using the appropriate timestamp, instead of using hardcoded 12M gas.
Also some updates to logging where we
1. Try to decode errors from our Rollup ABI
2. truncate large Hexes but still include them in error data so txs can be investigated
Update slot duration to 36 for sepolia deployments

Follow-up from this: #11390

spalladino

Looks good! Love to see this API being put to good use. Left a few comments. And yeah, it'd be nice to have a sort-of-unit-test that spins up a reth (instead of an anvil) to test this, as you mentioned today.

yarn-project/archiver/src/archiver/archiver.test.ts

spalladino · 2025-01-20T20:27:36Z

yarn-project/ethereum/src/l1_tx_utils.test.ts

+      const { message } = formatViemError(err, abi);
+      expect(message).toBe('Test_Error(33)');


spalladino · 2025-01-20T20:33:38Z

yarn-project/ethereum/src/l1_tx_utils.ts

+      } else if (gasConfig.gasLimit) {
+        gasLimit = this.bumpGasLimit(gasConfig.gasLimit, gasConfig);


Suggested change

} else if (gasConfig.gasLimit) {

gasLimit = this.bumpGasLimit(gasConfig.gasLimit, gasConfig);

} else if (gasConfig.gasLimit) {

gasLimit = gasConfig.gasLimit;

I find it confusing that if I set a fixed value, the library then bumps it. I'd expect it to use the exact fixed value I requested. Open to discussion though!

I was on the fence on this, I kept fixedGas for this purpose but I might just be making it confusing.
The reason was, I wanted to bump the result of simulateGasUsed in l1-publisher without having a fixed number.
Probably can just extract the gas settings inside l1-publisher though, and multiply it there

spalladino · 2025-01-20T20:36:27Z

yarn-project/ethereum/src/l1_tx_utils.ts

                }
                return receipt;
              }
            } catch (err) {
              if (err instanceof Error && err.message.includes('reverted')) {
-                throw err;
+                throw formatViemError(err);


What happens if we pass something that is not a ViemError to formatViemError? I'm worried we may get a non-viem error but still something that contains the string "reverted". But if we're fine sending anything into formatViemError, no worries then!

yeah probably a good idea to check inside formatViemError and add a test case for it 👍

spalladino · 2025-01-20T20:39:21Z

yarn-project/ethereum/src/utils.ts

+    if (error instanceof BaseError) {
+      const revertError = error.walk(err => err instanceof ContractFunctionRevertedError);
+      if (revertError instanceof ContractFunctionRevertedError) {
+        const errorName = revertError.data?.errorName ?? '';


Can we extract the error selector here if we don't find the errorName?

huh, I did have a case for that where we extract .signature, probably lost in the sea of commits 🤦
Good catch, will add back

yarn-project/sequencer-client/src/publisher/l1-publisher.ts

spalladino · 2025-01-20T20:42:46Z

yarn-project/sequencer-client/src/publisher/l1-publisher.ts

@@ -983,7 +987,8 @@ export class L1Publisher {
    //        we will fail estimation in the case where we are simulating for the
    //        first ethereum block within our slot (as current time is not in the
    //        slot yet).
-    const gasGuesstimate = blobEvaluationGas + L1Publisher.PROPOSE_GAS_GUESS;
+    // const gasGuesstimate = blobEvaluationGas + L1Publisher.PROPOSE_GAS_GUESS;


Should we delete this and the comment above?

spalladino · 2025-01-20T20:43:46Z

yarn-project/sequencer-client/src/publisher/l1-publisher.ts

+          data,
+        },
+        {
+          time: timestamp + 1n,


Let's add a comment why this +1n

spalladino · 2025-01-20T20:52:27Z

yarn-project/sequencer-client/src/publisher/l1-publisher.ts

+            // @note we override checkBlob to false since blobs are not part simulate()
+            stateDiff: [
+              {
+                slot: toHex(9n, true),


It'd be nice if we tweaked generate-artifacts.sh in yarn-project so we could extract the storage locations from the artifact output. If that ends up being too much, do you think we could add a unit test or something to check that this slot doesn't change?

I think we could probably just use the forge inspect <CONTRACT> storageLayout --json to populate, then it should be fairly easy to fetch from there. Sounds like a good idea to have.

Its a kinda annoying one though, as ideally the variable itself should not exists 😬.

spalladino · 2025-01-20T20:54:32Z

yarn-project/ethereum/src/l1_tx_utils.ts

+      if (err instanceof MethodNotFoundRpcError || err instanceof MethodNotSupportedRpcError) {
+        // Node doesn't support simulation, return -1n gas estimate
+        this.logger?.error('Node does not support simulation API');
+        return -1n;


I'd vote for rethrowing instead of using -1, in case we forget to check for that in the caller and end up propagating an invalid value.

Co-authored-by: Santiago Palladino <[email protected]>

LHerskind · 2025-01-21T11:56:23Z

yarn-project/end-to-end/src/composed/integration_l1_publisher.test.ts

@@ -528,17 +528,19 @@ describe('L1Publisher integration', () => {
      await expect(publisher.proposeL2Block(block)).resolves.toEqual(false);

      // Test for both calls
-      expect(loggerErrorSpy).toHaveBeenCalledTimes(2);
+      // NOTE: First error is from the simulate fn, which isn't supported by anvil


The simulate with time overrides?

LHerskind · 2025-01-21T12:03:30Z

yarn-project/ethereum/src/utils.ts

+ * @param abi - The ABI to use for decoding.
+ * @returns A FormattedViemError instance.
+ */
+export function formatViemError(error: any, abi: Abi = RollupAbi): FormattedViemError {


I altered the script that we use to generate the artifacts such that the errors should be included inside the other ABIs. So they are in the rollup one or whetever is being used. But maybe it should not be specifically the RollupAbi that is being checked against here. Mainly if some other contract were used this formatter gets pretty strange.

https://github.com/AztecProtocol/aztec-packages/pull/10697/files

LHerskind · 2025-01-21T12:08:41Z

yarn-project/sequencer-client/src/publisher/l1-publisher.ts

    //        By simulation issue, I mean the fact that the block.timestamp is equal to the last block, not the next, which
    //        make time consistency checks break.


Should not be the case. Also, we should be able to get rid of the validateBlockForSubmission here if the send can actually simulate it.

LHerskind · 2025-01-21T12:12:06Z

yarn-project/sequencer-client/src/publisher/l1-publisher.ts

+            // @note we override checkBlob to false since blobs are not part simulate()
+            stateDiff: [
+              {
+                slot: toHex(9n, true),


I think we could probably just use the forge inspect <CONTRACT> storageLayout --json to populate, then it should be fairly easy to fetch from there. Sounds like a good idea to have.

Its a kinda annoying one though, as ideally the variable itself should not exists 😬.

LHerskind · 2025-01-21T12:12:48Z

yarn-project/sequencer-client/src/publisher/l1-publisher.ts

+        ],
+      );
+
+      if (simulationResult === -1n) {


Throwing an error as mentioner earlier would make more sense to me here.

This reverts commit ab9ce34.

github-actions · 2025-01-22T16:33:49Z

Changes to public function bytecode sizes

Generated at commit: 4c2833fa0650f18192cd4572b22d5f2ef6000a15, compared to commit: 30a063a65f95403773d13da0d9a896da45d9608d

🧾 Summary (100% most significant diffs)

Program	Bytecode size in bytes (+/-)	%
AvmTest::bulk_testing	+70 ❌	+0.31%
AvmTest::public_dispatch	+59 ❌	+0.09%

Full diff report 👇

Program	Bytecode size in bytes (+/-)	%
AvmTest::bulk_testing	22,676 (+70)	+0.31%
AvmTest::public_dispatch	62,199 (+59)	+0.09%

spypsy and others added 3 commits January 14, 2025 11:54

fix: use simulation to estimate gas used

e99a59a

Merge branch 'master' into spy/simulate-l1-txs

5c61192

undo sequencer version bump

e4a3640

spypsy marked this pull request as ready for review January 14, 2025 12:09

spypsy and others added 21 commits January 14, 2025 12:40

upgrade viem in sequencer-client

94be510

upgrade all viem

caf932c

fix more typings

e3bc173

restore block validation

2ed14da

remove typo 😬

950d9ca

handle node not supporting simulation

05cd25e

rm unused import

499bb74

Merge branch 'master' into spy/simulate-l1-txs

2df6ccc

Merge branch 'master' into spy/simulate-l1-txs

9a08a1c

Merge branch 'master' into spy/simulate-l1-txs

98817bc

fix archiver tests

aa9cb12

bump simulated gas as well

8e966be

add logging to simulation

13b3d89

fix log

111a640

add logging, significantly bump evaluated gas

77968f5

use same timestamp with validateHeader & check simulation status

b5b85e1

temp fix test

aac8611

log blob inputs

6a27978

better logging

506e398

override checkBlobs to false

6a80190

add 1s to block time

dc8198a

spypsy force-pushed the spy/simulate-l1-txs branch from a3586fb to dc8198a Compare January 17, 2025 15:33

spypsy and others added 4 commits January 17, 2025 15:54

Merge branch 'master' into spy/simulate-l1-txs

8061b88

improve custom error logging

d5d5928

Merge branch 'master' into spy/simulate-l1-txs

5fd817b

bump %, return entire error from viem

644d00c

spypsy requested a review from spalladino January 20, 2025 17:41

more logging fixes

0db5e75

spalladino reviewed Jan 20, 2025

View reviewed changes

spypsy and others added 4 commits January 21, 2025 09:10

Update yarn-project/archiver/src/archiver/archiver.test.ts

38fba15

Co-authored-by: Santiago Palladino <[email protected]>

Update yarn-project/prover-node/src/bond/factory.ts

8a76d0e

Co-authored-by: Santiago Palladino <[email protected]>

Update yarn-project/sequencer-client/src/publisher/l1-publisher.ts

ab06ba7

Co-authored-by: Santiago Palladino <[email protected]>

don't bump gasLimit option by default

763cab6

LHerskind reviewed Jan 21, 2025

View reviewed changes

spypsy and others added 21 commits January 21, 2025 12:25

ErrorsAbi

d51c699

throw error on simulation instead of -1n

a50e728

Merge branch 'master' into spy/simulate-l1-txs

b8bcf63

some more log clarifications for L1

b694bfc

handle regular ol' errors

4bd5f1f

update test

abb34ff

fix waitForEthereum

33b6e40

add gasLimit to blockOverride (thx reth)

5f782c8

remove .only

30f8648

fix sepolia values yaml & tf

5a9177e

just ethereum.externalHost

730d120

update 48 vals yaml too

acba1b0

Merge branch 'master' into spy/simulate-l1-txs

4be77a0

nuke validateHeader

ab9ce34

fix imports

d50ba48

Revert "nuke validateHeader"

d432d46

This reverts commit ab9ce34.

re-instate imports

63fd6e9

formatting

7e9c3ce

Merge branch 'master' into spy/simulate-l1-txs

cf85a88

add logging to timed out tx, reduce bot tx interval

4c9ffc2

tweak gas config

7e07713

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use simulation to estimate gas used #11211

fix: use simulation to estimate gas used #11211

spypsy commented Jan 14, 2025 •

edited

Loading

spalladino left a comment

spalladino Jan 20, 2025

spalladino Jan 20, 2025

spypsy Jan 21, 2025 •

edited

Loading

spalladino Jan 20, 2025

spypsy Jan 21, 2025

spalladino Jan 20, 2025

spypsy Jan 21, 2025

spalladino Jan 20, 2025

spalladino Jan 20, 2025

spalladino Jan 20, 2025

LHerskind Jan 21, 2025

spalladino Jan 20, 2025

LHerskind Jan 21, 2025

spypsy Jan 21, 2025

LHerskind Jan 21, 2025

LHerskind Jan 21, 2025

LHerskind Jan 21, 2025

LHerskind Jan 21, 2025

github-actions bot commented Jan 22, 2025

		const { message } = formatViemError(err, abi);
		expect(message).toBe('Test_Error(33)');

		} else if (gasConfig.gasLimit) {
		gasLimit = this.bumpGasLimit(gasConfig.gasLimit, gasConfig);

		// By simulation issue, I mean the fact that the block.timestamp is equal to the last block, not the next, which
		// make time consistency checks break.

fix: use simulation to estimate gas used #11211

Are you sure you want to change the base?

fix: use simulation to estimate gas used #11211

Conversation

spypsy commented Jan 14, 2025 • edited Loading

spalladino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spypsy Jan 21, 2025 • 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

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

github-actions bot commented Jan 22, 2025

Changes to public function bytecode sizes

🧾 Summary (100% most significant diffs)

spypsy commented Jan 14, 2025 •

edited

Loading

spypsy Jan 21, 2025 •

edited

Loading