feat: Speed up transaction execution by PhilWindle · Pull Request #10172 · AztecProtocol/aztec-packages

PhilWindle · 2024-11-24T23:15:44Z

This PR make a number of optimisation related to the speed up of transaction execution. Namely:

We don't re-initialise the instruction set mapping with each instruction decode.
We now compute public bytecode commitments at the point of receiving a contract and persist them, meaning they don't need to be computed at the point of execution.
We only store and iterate opcode and program counter tally information when in debug.
Function names are also cached at the point contract artifacts are shared with the node.
World state status summary and previous block archive roots are cached to reduce the impact on the world state DB whilst execution is taking place.

fcarreiro · 2024-11-25T09:54:48Z

-    bytecodeCommitment = poseidon2Hash([encodedBytecode[i + 1], bytecodeCommitment]);
-  }
-  return bytecodeCommitment;
+  return bytecodeLength == 0 ? new Fr(0) : poseidon2HashAccumulate(encodedBytecode.slice(1, bytecodeLength + 1));


@IlyasRidhuan do we have any test (e.g. fixed hash) to know that this still works exactly as before?

Not directly - but it'll be covered in the avm_proving.test because the witgen will compute it in the loop and ASSERTs they are the same

fcarreiro · 2024-11-25T09:57:26Z

        `Contract class not found in DB, but a contract instance was found with this class ID (${instance.contractClassId}). This should not happen!`,
      );

+      assert(


@dbanks12 we'll have to add this to the list of errors to handle properly. (same with the contract class)

I don't think we want to handle this error. This means something is broken because the contract instance "exists", but it's contract class or bytecode are missing in the DB. So there is a bug somewhere.

fcarreiro · 2024-11-25T09:58:08Z


 // Specifies how to read and write each operand type.
-const OPERAND_SPEC = new Map<OperandType, [number, () => OperandNativeType, OperandWriter]>([
+const OPERAND_SPEC = new Map<OperandType, [number, (offset: number) => OperandNativeType, OperandWriter]>([


@PhilWindle does having the offset here help a lot?

We could probably remove this if we think this is undesirable. I flamegraphed the buffer/cursor stuff and creating subarrays for each instruction decode came out as the most costly part of this process. So I switched it to basically keep the same buffer with a single offset that is moved around. However, I later found that this buffer/cursor latency was marginal in the grand scheme of the decoding process.

No worries, it's ok if it helps.

fcarreiro

LGTM for the avm side. I'll let Palla aprove the archiver/db side.

spalladino

Looks good! There are some changes I'd push for, but they are not critical and can be addressed in another PR if you prefer.

spalladino · 2024-11-26T12:52:29Z

+    if (process.env.LOG_LEVEL === 'debug') {
+      this.tallyPrintFunction = this.printOpcodeTallies;
+      this.tallyInstructionFunction = this.tallyInstruction;
+    }


This will ignore namespaces set in the debug env var, but I can take care of it as part of the upcoming logs refactor.

spalladino · 2024-11-26T12:53:28Z

    assert(n < this._buffer.length);
  }

+  public peakUint8(): number {


Suggested change

public peakUint8(): number {

public peekUint8(): number {

spalladino · 2024-11-26T13:01:17Z

  #contractClassStore: ContractClassStore;
  #contractInstanceStore: ContractInstanceStore;
  #contractArtifactStore: ContractArtifactsStore;
+  private functionNames = new Map<string, string>();


Shouldn't this be persisted, as opposed to an in-memory Map? If we do it memory-only as a cache, then the getter should recalculate it on a miss.

As discussed. These are currently in memory only as they are a debugging feature that is perhaps not a full feature of the node or it's api. I've commented that this needs to be reviewed along with other similar features.

spalladino · 2024-11-26T13:04:55Z

+    contract.functions.forEach(f => {
+      this.functionNames.set(FunctionSelector.fromNameAndParameters(f.name, f.parameters).toString(), f.name);
+    });


This will cause issues down the road with selector clashes. A selector is just 4 bytes (we're borrowing this from Ethereum), so it's relatively easy to stumble upon a collision. Indexing the functionNames collection by selector only will mean that the most recently stored selector will overwrite any previous ones.

Unfortunately, I think the only recourse is to store both contract class id and selector as key.

Fun fact: proxies built using the DELEGATECALL pattern in Ethereum were subject to a vulnerability caused by selector clashing.

https://forum.openzeppelin.com/t/beware-of-the-proxy-learn-how-to-exploit-function-clashing/1070

spalladino · 2024-11-26T13:06:33Z

+  getBytecodeCommitment(contractClassId: Fr): Promise<Fr | undefined> {
+    return this.store.getBytecodeCommitment(contractClassId);
+  }


I don't think there are any cases where we want the bytecode commitment but not the bytecode, right? If so, instead of having a separate getter for the bytecode commitment, I'd just store it along with the rest of the contract class in the same store, so we don't have to query for them separately. But this is just a cleanup, no need to do now.

…kages into pw/check-times-2

As #10172 got merged right after #10176 it had not run the new test against the updated master so the issue with fee payment was not caught. To fix it, we fetch the current base fees and use those in our settings.

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.65.1</summary> ## [0.65.1](aztec-package-v0.65.0...aztec-package-v0.65.1) (2024-11-27) ### Miscellaneous * Delete old serialization methods ([#9951](#9951)) ([10d3f6f](10d3f6f)) </details> <details><summary>barretenberg.js: 0.65.1</summary> ## [0.65.1](barretenberg.js-v0.65.0...barretenberg.js-v0.65.1) (2024-11-27) ### Features * Speed up transaction execution ([#10172](#10172)) ([da265b6](da265b6)) ### Bug Fixes * Add pako as a dependency in bb.js ([#10186](#10186)) ([b773c14](b773c14)) </details> <details><summary>aztec-packages: 0.65.1</summary> ## [0.65.1](aztec-packages-v0.65.0...aztec-packages-v0.65.1) (2024-11-27) ### Features * Add total mana used to header ([#9868](#9868)) ([2478d19](2478d19)) * Assert metrics in network tests ([#10215](#10215)) ([9380c0f](9380c0f)) * Avm inserts nullifiers from private ([#10129](#10129)) ([3fc0c7c](3fc0c7c)) * Burn congestion fee ([#10231](#10231)) ([20a33f2](20a33f2)) * Configure world state block history ([#10216](#10216)) ([01eb392](01eb392)) * Integrate fee into rollup ([#10176](#10176)) ([12744d6](12744d6)) * Speed up transaction execution ([#10172](#10172)) ([da265b6](da265b6)) * Using current gas prices in cli-wallet ([#10105](#10105)) ([15ffeea](15ffeea)) ### Bug Fixes * Add pako as a dependency in bb.js ([#10186](#10186)) ([b773c14](b773c14)) * **avm:** Execution test ordering ([#10226](#10226)) ([49b4a6c](49b4a6c)) * Deploy preview master ([#10227](#10227)) ([321a175](321a175)) * Use current base fee for public fee payment ([#10230](#10230)) ([f081d80](f081d80)) ### Miscellaneous * Add traces and histograms to avm simulator ([#10233](#10233)) ([e83726d](e83726d)), closes [#10146](#10146) * Avm-proving and avm-integration tests do not require simulator to export function with jest mocks ([#10228](#10228)) ([f28fcdb](f28fcdb)) * **avm:** Handle parsing error ([#10203](#10203)) ([3c623fc](3c623fc)), closes [#9770](#9770) * **avm:** Zero initialization in avm public inputs and execution test fixes ([#10238](#10238)) ([0c7c4c9](0c7c4c9)) * Bump timeout for after-hook for data store test again ([#10240](#10240)) ([52047f0](52047f0)) * CIVC VK ([#10223](#10223)) ([089c34c](089c34c)) * Declare global types ([#10206](#10206)) ([7b2e343](7b2e343)) * Delete old serialization methods ([#9951](#9951)) ([10d3f6f](10d3f6f)) * Fix migration notes ([#10252](#10252)) ([05bdcd5](05bdcd5)) * Pull out some sync changes ([#10245](#10245)) ([1bfc15e](1bfc15e)) * Remove docs from sync ([#10241](#10241)) ([eeea0aa](eeea0aa)) * Replace relative paths to noir-protocol-circuits ([e7690ca](e7690ca)) </details> <details><summary>barretenberg: 0.65.1</summary> ## [0.65.1](barretenberg-v0.65.0...barretenberg-v0.65.1) (2024-11-27) ### Features * Add total mana used to header ([#9868](#9868)) ([2478d19](2478d19)) * Configure world state block history ([#10216](#10216)) ([01eb392](01eb392)) * Speed up transaction execution ([#10172](#10172)) ([da265b6](da265b6)) ### Bug Fixes * **avm:** Execution test ordering ([#10226](#10226)) ([49b4a6c](49b4a6c)) ### Miscellaneous * **avm:** Handle parsing error ([#10203](#10203)) ([3c623fc](3c623fc)), closes [#9770](#9770) * **avm:** Zero initialization in avm public inputs and execution test fixes ([#10238](#10238)) ([0c7c4c9](0c7c4c9)) * CIVC VK ([#10223](#10223)) ([089c34c](089c34c)) * Pull out some sync changes ([#10245](#10245)) ([1bfc15e](1bfc15e)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.65.1</summary> ## [0.65.1](AztecProtocol/aztec-packages@aztec-package-v0.65.0...aztec-package-v0.65.1) (2024-11-27) ### Miscellaneous * Delete old serialization methods ([#9951](AztecProtocol/aztec-packages#9951)) ([10d3f6f](AztecProtocol/aztec-packages@10d3f6f)) </details> <details><summary>barretenberg.js: 0.65.1</summary> ## [0.65.1](AztecProtocol/aztec-packages@barretenberg.js-v0.65.0...barretenberg.js-v0.65.1) (2024-11-27) ### Features * Speed up transaction execution ([#10172](AztecProtocol/aztec-packages#10172)) ([da265b6](AztecProtocol/aztec-packages@da265b6)) ### Bug Fixes * Add pako as a dependency in bb.js ([#10186](AztecProtocol/aztec-packages#10186)) ([b773c14](AztecProtocol/aztec-packages@b773c14)) </details> <details><summary>aztec-packages: 0.65.1</summary> ## [0.65.1](AztecProtocol/aztec-packages@aztec-packages-v0.65.0...aztec-packages-v0.65.1) (2024-11-27) ### Features * Add total mana used to header ([#9868](AztecProtocol/aztec-packages#9868)) ([2478d19](AztecProtocol/aztec-packages@2478d19)) * Assert metrics in network tests ([#10215](AztecProtocol/aztec-packages#10215)) ([9380c0f](AztecProtocol/aztec-packages@9380c0f)) * Avm inserts nullifiers from private ([#10129](AztecProtocol/aztec-packages#10129)) ([3fc0c7c](AztecProtocol/aztec-packages@3fc0c7c)) * Burn congestion fee ([#10231](AztecProtocol/aztec-packages#10231)) ([20a33f2](AztecProtocol/aztec-packages@20a33f2)) * Configure world state block history ([#10216](AztecProtocol/aztec-packages#10216)) ([01eb392](AztecProtocol/aztec-packages@01eb392)) * Integrate fee into rollup ([#10176](AztecProtocol/aztec-packages#10176)) ([12744d6](AztecProtocol/aztec-packages@12744d6)) * Speed up transaction execution ([#10172](AztecProtocol/aztec-packages#10172)) ([da265b6](AztecProtocol/aztec-packages@da265b6)) * Using current gas prices in cli-wallet ([#10105](AztecProtocol/aztec-packages#10105)) ([15ffeea](AztecProtocol/aztec-packages@15ffeea)) ### Bug Fixes * Add pako as a dependency in bb.js ([#10186](AztecProtocol/aztec-packages#10186)) ([b773c14](AztecProtocol/aztec-packages@b773c14)) * **avm:** Execution test ordering ([#10226](AztecProtocol/aztec-packages#10226)) ([49b4a6c](AztecProtocol/aztec-packages@49b4a6c)) * Deploy preview master ([#10227](AztecProtocol/aztec-packages#10227)) ([321a175](AztecProtocol/aztec-packages@321a175)) * Use current base fee for public fee payment ([#10230](AztecProtocol/aztec-packages#10230)) ([f081d80](AztecProtocol/aztec-packages@f081d80)) ### Miscellaneous * Add traces and histograms to avm simulator ([#10233](AztecProtocol/aztec-packages#10233)) ([e83726d](AztecProtocol/aztec-packages@e83726d)), closes [#10146](AztecProtocol/aztec-packages#10146) * Avm-proving and avm-integration tests do not require simulator to export function with jest mocks ([#10228](AztecProtocol/aztec-packages#10228)) ([f28fcdb](AztecProtocol/aztec-packages@f28fcdb)) * **avm:** Handle parsing error ([#10203](AztecProtocol/aztec-packages#10203)) ([3c623fc](AztecProtocol/aztec-packages@3c623fc)), closes [#9770](AztecProtocol/aztec-packages#9770) * **avm:** Zero initialization in avm public inputs and execution test fixes ([#10238](AztecProtocol/aztec-packages#10238)) ([0c7c4c9](AztecProtocol/aztec-packages@0c7c4c9)) * Bump timeout for after-hook for data store test again ([#10240](AztecProtocol/aztec-packages#10240)) ([52047f0](AztecProtocol/aztec-packages@52047f0)) * CIVC VK ([#10223](AztecProtocol/aztec-packages#10223)) ([089c34c](AztecProtocol/aztec-packages@089c34c)) * Declare global types ([#10206](AztecProtocol/aztec-packages#10206)) ([7b2e343](AztecProtocol/aztec-packages@7b2e343)) * Delete old serialization methods ([#9951](AztecProtocol/aztec-packages#9951)) ([10d3f6f](AztecProtocol/aztec-packages@10d3f6f)) * Fix migration notes ([#10252](AztecProtocol/aztec-packages#10252)) ([05bdcd5](AztecProtocol/aztec-packages@05bdcd5)) * Pull out some sync changes ([#10245](AztecProtocol/aztec-packages#10245)) ([1bfc15e](AztecProtocol/aztec-packages@1bfc15e)) * Remove docs from sync ([#10241](AztecProtocol/aztec-packages#10241)) ([eeea0aa](AztecProtocol/aztec-packages@eeea0aa)) * Replace relative paths to noir-protocol-circuits ([e7690ca](AztecProtocol/aztec-packages@e7690ca)) </details> <details><summary>barretenberg: 0.65.1</summary> ## [0.65.1](AztecProtocol/aztec-packages@barretenberg-v0.65.0...barretenberg-v0.65.1) (2024-11-27) ### Features * Add total mana used to header ([#9868](AztecProtocol/aztec-packages#9868)) ([2478d19](AztecProtocol/aztec-packages@2478d19)) * Configure world state block history ([#10216](AztecProtocol/aztec-packages#10216)) ([01eb392](AztecProtocol/aztec-packages@01eb392)) * Speed up transaction execution ([#10172](AztecProtocol/aztec-packages#10172)) ([da265b6](AztecProtocol/aztec-packages@da265b6)) ### Bug Fixes * **avm:** Execution test ordering ([#10226](AztecProtocol/aztec-packages#10226)) ([49b4a6c](AztecProtocol/aztec-packages@49b4a6c)) ### Miscellaneous * **avm:** Handle parsing error ([#10203](AztecProtocol/aztec-packages#10203)) ([3c623fc](AztecProtocol/aztec-packages@3c623fc)), closes [#9770](AztecProtocol/aztec-packages#9770) * **avm:** Zero initialization in avm public inputs and execution test fixes ([#10238](AztecProtocol/aztec-packages#10238)) ([0c7c4c9](AztecProtocol/aztec-packages@0c7c4c9)) * CIVC VK ([#10223](AztecProtocol/aztec-packages#10223)) ([089c34c](AztecProtocol/aztec-packages@089c34c)) * Pull out some sync changes ([#10245](AztecProtocol/aztec-packages#10245)) ([1bfc15e](AztecProtocol/aztec-packages@1bfc15e)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

PhilWindle added 7 commits November 21, 2024 09:28

First set of stats

bf362b8

WIP

759143b

WIP

c52e2fd

WIP

3c8e324

WIP

7d46400

WIP

2843505

WIP

0da28dc

PhilWindle requested review from dbanks12 and fcarreiro as code owners November 24, 2024 23:15

WIP

b9dc6ac

PhilWindle added the e2e-all label Nov 24, 2024

PhilWindle added 3 commits November 24, 2024 23:31

Merge remote-tracking branch 'origin/master' into pw/check-times-2

92cdace

WIP

a0c21fc

Formatting

f932cf3

fcarreiro reviewed Nov 25, 2024

View reviewed changes

This was linked to issues Nov 25, 2024

Store cache public bytecode commitments #10137

Closed

Rationalise requests to world state during simulation #10138

Closed

Speed up instruction decoding #10142

Closed

PhilWindle added 7 commits November 25, 2024 13:36

Keep instruction set iniitialisation within AVM simulator

05159b8

Function bind fixes

d01a9e3

Merge remote-tracking branch 'origin/master' into pw/check-times-2

95adcfc

Merge remote-tracking branch 'origin/master' into pw/check-times-2

3b73deb

New test

8455cbf

Test fixes

57f5749

Test fixes

0cc95a4

fcarreiro approved these changes Nov 26, 2024

View reviewed changes

spalladino reviewed Nov 26, 2024

View reviewed changes

PhilWindle added 2 commits November 26, 2024 13:46

Review fixes

3e5c8c5

Review fixes

332894f

PhilWindle enabled auto-merge (squash) November 26, 2024 13:51

PhilWindle and others added 6 commits November 26, 2024 14:49

Merge branch 'master' into pw/check-times-2

41bca92

Also process world state errors on the job queue

a5e9c16

Merge branch 'pw/check-times-2' of github.com:AztecProtocol/aztec-pac…

cd6ba1c

…kages into pw/check-times-2

Test fix

98b957d

Updated comment

9439407

Merge branch 'pw/check-times-2' of github.com:AztecProtocol/aztec-pac…

ae26b46

…kages into pw/check-times-2

PhilWindle merged commit da265b6 into master Nov 26, 2024

PhilWindle deleted the pw/check-times-2 branch November 26, 2024 20:01

AztecBot mentioned this pull request Nov 26, 2024

chore(master): Release 0.65.1 #10219

Merged

LHerskind mentioned this pull request Nov 26, 2024

fix: use current base fee for public fee payment #10230

Merged

Conversation

PhilWindle commented Nov 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fcarreiro left a comment

Choose a reason for hiding this comment

Uh oh!

spalladino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

PhilWindle commented Nov 24, 2024 •

edited

Loading