feat: Simulate enqueued public functions and locate failing constraints on them #1853

sirasistant · 2023-08-29T08:45:58Z

Simulates the public part of transactions before submission to the node.
Introduces a SimulationError to show complete call stacks (aztec function calls and noir function calls)
Reconstructs public function errors to show noir call stacks on public functions after simulation
Fixes a bug where the public tx processor was mutating Tx objects in place.

Now a failing constraint in a public function will be printed as follows during public simulation:

  aztec:sequencer:public-processor Error processing tx 1874643b193126ac29744b17f75f7ace585b26d8c5884325b9f3aff16959ac48: Error: Cannot satisfy constraint 0.48 +84ms
  aztec:rpc_server Simulation error: Assertion failed in public execution: 'inputs.call_context.msg_sender == inputs.call_context.storage_contract_address'
  aztec:rpc_server   at /mnt/user-data/alvaro/aztec-packages/yarn-project/noir-contracts/src/contracts/child_contract/src/main:85 'inputs.call_context.msg_sender == inputs.call_context.storage_contract_address'
  aztec:rpc_server   at Child.pubIncValueInternal
  aztec:rpc_server   at Parent.pubEntryPoint +255ms

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
Every change is related to the PR description.
I have linked this pull request to relevant issues (if any exist).

…packages into arv/simulate_public

sirasistant · 2023-08-29T15:47:55Z

yarn-project/aztec-node/src/aztec-node/server.ts

+    const merkleTrees = new MerkleTrees(this.merkleTreesDb, this.log);
+    await merkleTrees.init(await CircuitsWasm.get(), {
+      globalVariables: prevGlobalVariables,
+    });


Creating a disposable MerkleTrees instance from DB is needed due to #1869

spalladino

Looks great! I added a bunch of comments, but none that prevents this from being merged IMO.

yarn-project/acir-simulator/src/public/executor.ts

yarn-project/aztec-node/src/aztec-node/http-node.ts

spalladino · 2023-08-29T18:01:50Z

yarn-project/aztec-node/src/aztec-node/http-node.ts

+   * Simulates the public part of a transaction with the current state.
+   * @param tx - The transaction to simulate.
+   **/
+  public async simulatePublicPart(tx: Tx) {


Aside from the name, does it make sense to send a full tx object? Shouldn't we be just sending the enqueuedFunctionCalls, plus any side effects from private execution that may affect the public simulation (such as new contract deployment)?

I think it's better to send the whole TX for some reasons:

This method IMO will evolve to be the gas estimator of a TX. In ethereum, gas estimation takes TX objects https://www.quicknode.com/docs/ethereum/eth_estimateGas

This also runs the public kernel simulator, which I think makes this method need most of the fields in the TX

It just feels like a cleaner interface to me. Create your TX, call the node to see if it will succeed / to estimate fees on it!

Fair point. And I know this is probably a bigger discussion, but how does this impact privacy? Should we enable the RPC server to run its own public simulations and gas estimations, by just requiring the state it needs from the node? Or would that leak information as well?

I believe it's ok, since you send to the node for simulation the same data you'd send the node for submission of the TX. I think in this case it's similar to ethereum, if you don't want the node to track which transaction came from which IP, you can run the node yourself and rely on the gossip p2p networking.

spalladino · 2023-08-29T18:04:48Z

yarn-project/aztec-node/src/aztec-node/server.ts

+    const [, failedTxs] = await processor.process([tx]);
+    if (failedTxs.length) {
+      throw failedTxs[0].error;
+    }
+    this.log.info(`Simulated tx ${await tx.getTxHash()} succeeds`);


We should extend this (in another PR!) so we report to the user the effects of a successful execution. Logs emitted is the easiest one, but we can also report back the call return value or any storage updates. Or the full trace even.

yarn-project/aztec-rpc/src/aztec_rpc_server/aztec_rpc_server.ts

yarn-project/rollup-provider/src/app.ts

yarn-project/sequencer-client/src/sequencer/public_processor.ts

spalladino · 2023-08-29T18:24:17Z

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

+        const failedTxData = failedTxs.map(fail => fail.tx);
+        this.log(`Dropping failed txs ${(await Tx.getHashes(failedTxData)).join(', ')}`);
+        await this.p2pClient.deleteTxs(await Tx.getHashes(failedTxData));


Do we have a tracking issue for adding the concept of reverted txs?

No idea, cc'ing @iAmMichaelConnor

🤖 I have created a new Aztec Packages release --- ## [0.1.0-alpha48](v0.1.0-alpha47...v0.1.0-alpha48) (2023-08-30) ### Features * Add ARM build for Mac + cleanup artifacts ([#1837](#1837)) ([270a4ae](270a4ae)) * broadcasting 'public key' and 'partial address' as L1 calldata ([#1801](#1801)) ([78d6444](78d6444)), closes [#1778](#1778) * Check sandbox version matches CLI's ([#1849](#1849)) ([7279730](7279730)) * **docs:** adding some nitpick suggestions before sandbox release ([#1859](#1859)) ([c1144f7](c1144f7)) * More reliable getTxReceipt api. ([#1793](#1793)) ([ad16b22](ad16b22)) * **noir:** use `#[aztec(private)]` and `#[aztec(public)` attributes ([#1735](#1735)) ([89756fa](89756fa)) * Recursive fn calls to spend more notes. ([#1779](#1779)) ([94053e4](94053e4)) * Simulate enqueued public functions and locate failing constraints on them ([#1853](#1853)) ([a065fd5](a065fd5)) * Update safe_math and move to libraries ([#1803](#1803)) ([b10656d](b10656d)) * Write debug-level log to local file in Sandbox ([#1846](#1846)) ([0317e93](0317e93)), closes [#1605](#1605) ### Bug Fixes * Conditionally compile base64 command for bb binary ([#1851](#1851)) ([be97185](be97185)) * default color to light mode ([#1847](#1847)) ([4fc8d39](4fc8d39)) * Disallow unregistered classes in JSON RPC interface and match by name ([#1820](#1820)) ([35b8170](35b8170)) * Set side effect counter on contract reads ([#1870](#1870)) ([1d8881e](1d8881e)), closes [#1588](#1588) * Truncate SRS size to the amount of points that we have downloaded ([#1862](#1862)) ([0a7058c](0a7058c)) ### Miscellaneous * add browser test to canary flow ([#1808](#1808)) ([7f4fa43](7f4fa43)) * **ci:** fix output name in release please workflow ([#1858](#1858)) ([857821f](857821f)) * CLI tests ([#1786](#1786)) ([2987065](2987065)), closes [#1450](#1450) * compile minimal WASM binary needed for blackbox functions ([#1824](#1824)) ([76a30b8](76a30b8)) * fixed linter errors for `ecc`, `numeric` and `common` modules ([#1714](#1714)) ([026273b](026273b)) * Refactor Cli interface to be more unix-like ([#1833](#1833)) ([28d722e](28d722e)) * sync bb master ([#1842](#1842)) ([2c1ff72](2c1ff72)) * sync bb master ([#1852](#1852)) ([f979878](f979878)) * sync bb master ([#1866](#1866)) ([e681a49](e681a49)) * typescript script names should be consistent ([#1843](#1843)) ([eff8fe7](eff8fe7)) * use 2^19 as `MAX_CIRCUIT_SIZE` for NodeJS CLI ([#1834](#1834)) ([c573282](c573282)) ### Documentation * Account contract tutorial ([#1772](#1772)) ([0faefba](0faefba)) * Wallet dev docs ([#1746](#1746)) ([9b4281d](9b4281d)), closes [#1744](#1744) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

sirasistant added 8 commits August 28, 2023 11:46

feat: first working version of simulate pub

a509429

feat: allow parsing nested public exec errors

408c83c

feat: sim errors with fn and noir call stacks

1ff4bd8

Merge branch 'master' into arv/simulate_public

3d14c40

fix: cleanup

8233a82

Merge branch 'arv/simulate_public' of github.com:AztecProtocol/aztec-…

7c09e7a

…packages into arv/simulate_public

test: fix test

7eda8a4

fix: avoid simulations modifying state and txs

aba3018

sirasistant force-pushed the arv/simulate_public branch from 7eb7817 to aba3018 Compare August 29, 2023 10:32

sirasistant added 7 commits August 29, 2023 10:34

fix: clone inside processor to avoid the mutation

4894747

Merge branch 'master' into arv/simulate_public

e706da1

refactor: improve printing of simulation errors

213e465

Merge branch 'arv/simulate_public' of github.com:AztecProtocol/aztec-…

7e9fad9

…packages into arv/simulate_public

feat: initial approach of transactional merkle db

21e7a0b

feat: option skip public sim & refactor trees txs

87b53b2

Merge branch 'master' into arv/simulate_public

ff46ce9

sirasistant requested review from spalladino, LeilaWang and PhilWindle August 29, 2023 14:40

sirasistant marked this pull request as ready for review August 29, 2023 14:41

sirasistant removed request for spalladino, LeilaWang and PhilWindle August 29, 2023 14:47

sirasistant marked this pull request as draft August 29, 2023 14:47

docs: added comment pointing to the created issue

204386f

sirasistant force-pushed the arv/simulate_public branch from 51325d9 to 204386f Compare August 29, 2023 15:44

Merge branch 'master' into arv/simulate_public

c90822a

sirasistant requested review from spalladino, LeilaWang and PhilWindle August 29, 2023 15:46

sirasistant marked this pull request as ready for review August 29, 2023 15:46

sirasistant commented Aug 29, 2023

View reviewed changes

spalladino approved these changes Aug 29, 2023

View reviewed changes

sirasistant added 3 commits August 30, 2023 08:15

refactor: improvements after peer review

98f2690

Merge branch 'master' into arv/simulate_public

c11a01f

fix: improve enrichment

e0db983

sirasistant merged commit a065fd5 into master Aug 30, 2023

sirasistant deleted the arv/simulate_public branch August 30, 2023 09:49

github-actions bot mentioned this pull request Aug 30, 2023

chore(master): release 0.1.0-alpha48 #1804

Merged

sirasistant mentioned this pull request Sep 5, 2023

Simulate should simulate full transactions not only private part #1377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Simulate enqueued public functions and locate failing constraints on them #1853

feat: Simulate enqueued public functions and locate failing constraints on them #1853

sirasistant commented Aug 29, 2023 •

edited

Loading

sirasistant Aug 29, 2023 •

edited

Loading

spalladino left a comment

spalladino Aug 29, 2023

sirasistant Aug 30, 2023

spalladino Aug 30, 2023

sirasistant Aug 30, 2023

spalladino Aug 29, 2023

spalladino Aug 29, 2023

sirasistant Aug 30, 2023

feat: Simulate enqueued public functions and locate failing constraints on them #1853

feat: Simulate enqueued public functions and locate failing constraints on them #1853

Conversation

sirasistant commented Aug 29, 2023 • edited Loading

Checklist:

sirasistant Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

spalladino left a comment

Choose a reason for hiding this comment

spalladino Aug 29, 2023

Choose a reason for hiding this comment

sirasistant Aug 30, 2023

Choose a reason for hiding this comment

spalladino Aug 30, 2023

Choose a reason for hiding this comment

sirasistant Aug 30, 2023

Choose a reason for hiding this comment

spalladino Aug 29, 2023

Choose a reason for hiding this comment

spalladino Aug 29, 2023

Choose a reason for hiding this comment

sirasistant Aug 30, 2023

Choose a reason for hiding this comment

sirasistant commented Aug 29, 2023 •

edited

Loading

sirasistant Aug 29, 2023 •

edited

Loading