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 #11440

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%

spalladino

LGTM! Just an issue with the things being thrown.

spalladino · 2025-01-23T12:49:47Z

spartan/aztec-network/templates/_helpers.tpl

+if [ -n "${EXTERNAL_ETHEREUM_HOST}" ]; then
+  export ETHEREUM_HOST="${EXTERNAL_ETHEREUM_HOST}"
+fi


Curious why we have two different env vars

spalladino · 2025-01-23T12:50:34Z

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

-      const formattedError = formatViemError(err);
-
+      const res = err;
+      // console.log('res', res);


Suggested change

// console.log('res', res);

spalladino · 2025-01-23T12:54:39Z

yarn-project/ethereum/src/l1_tx_utils.ts

-      throw formattedErr;
+      const { message, metaMessages } = formatViemError(err);
+      this.logger?.error(`Failed to send L1 transaction`, message, { metaMessages });
+      throw { ...err, message, metaMessages };


Heads up that you're not throwing an Error here, but a plain object. So any checks downstream for instanceof Error will fail, and any formatting specific to errors (such as printing the stack trace) will get lost. We should always throw a proper Error (or subclass) instance to avoid issues.

spalladino · 2025-01-23T12:54:49Z

yarn-project/ethereum/src/l1_tx_utils.ts

        if (err.message?.includes('reverted')) {
-          throw formattedErr;
+          throw { ...error, message };


Same as above

spalladino · 2025-01-23T12:59:16Z

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

+        if (simErr instanceof MethodNotFoundRpcError || simErr instanceof MethodNotSupportedRpcError) {
+          // @note node doesn't support simulation API, we will use gas guesstimate
+          this.log.warn('Using gas guesstimate');
+          simulationResult = L1Publisher.PROPOSE_GAS_GUESS;
+        } else {
+          throw simErr;
+        }


It seems that we'll be doing this pretty often. Should we have simulateGasUsed accept a fallbackGasIfMethodNotSupported option to handle this automatically?

spalladino · 2025-01-23T13:00: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.


I take it this will be addressed in another PR then, right?

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 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

spypsy requested review from LHerskind and spalladino January 23, 2025 11:37

spalladino reviewed Jan 23, 2025

View reviewed changes

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

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

spalladino left a comment

spalladino Jan 23, 2025

spalladino Jan 23, 2025

spalladino Jan 23, 2025

spalladino Jan 23, 2025

spalladino Jan 23, 2025

spalladino Jan 23, 2025

		// 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

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)

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

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

spypsy commented Jan 14, 2025 •

edited

Loading