chore: share state manager and side effect tracer for all enqueued calls in tx #9565

dbanks12 · 2024-10-30T01:30:53Z

This PR is a bit of a mashup of things as I made sense of what needs to happen to start tracking side effects and state changes at the transaction level.

Refactor of PublicExecutionResult, PublicExecutor, EnqueuedCallSimulator & EnqueuedCallsProcessor. A lot of this PR is prep for removal of the public kernel and generation of TX-level AVM public inputs & hints.

Pulls the AvmStateManager (needs rename) up to the EnqueuedCallsProcessor and uses it for state checkpointing/forking & rollbacks instead of WorldStateDB. This will let us track side effects at the transaction level and eventually generate AVM circuit hints & public inputs at the transaction level.
Simplifies the return type (public execution result) from PublicExecutor.simulate(). The only consumers of PublicExecutor.simulate() are the TXE and the EnqueuedCallSimulator. The TXE needs side effects, but the EnqueuedCallSimulator shouldn't (although it does until the public kernel is deleted). I provided a minimal return type for each case. EnqueuedCallSimulator doesn't receive side effects in the return type from PublicExecutor.simulate() and instead makes a call to the trace to retrieve VMCircuitPublicInputs. Eventually that call to the trace will happen in the EnqueuedCallsProcessor just once for the entire transaction.
Moves some PublicProcessor tests into EnqueudCallsProcessor tests as they don't really belong at that high a level.
Adds tracing of enqueued calls in side effect trace for future use of enqueued call hints.

Followup work:

Rename AvmStateManager -> PublicStateManager, rename file accordingly, & move to simulator/src/public
Once public kernel is removed and new AVMPublicInputs are introduced, pull the calls to stateManager.trace.toVMCircuitPublicInputs() up to the EnqueuedCallsProcessor & modify that function to return the new AVMPublicInputs
Remove the deprecated PublicFunctionCallResult and the old PublicSideEffectTrace (to be fully replaced by the PublicEnqueuedCallSideEffectTrace which will need a rename). This will require disconnecting the AVM circuit completely until it supports the tx-level AVMPublicInputs.

dbanks12 · 2024-10-30T01:31:07Z

[WIP] chore: refactoring public executor interface #9701
chore: share state manager and side effect tracer for all enqueued calls in tx #9565 👈
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @dbanks12 and the rest of your teammates on Graphite

dbanks12 · 2024-11-05T18:30:11Z

...ir-protocol-circuits/crates/public-kernel-lib/src/components/enqueued_call_data_validator.nr


-    fn validate_counters(self, previous_kernel: PublicKernelCircuitPublicInputs) {
-        assert_eq(
-            self.enqueued_call.data.start_side_effect_counter,
-            previous_kernel.end_side_effect_counter + 1,
-            "enqueued call must start from the end counter of the previous call",
-        );
-    }
-
    // Validates that the start gas injected into the vm circuit matches the remaining gas.


This constraint was failing because when an enqueued call had no side effects, it would start with start == prev.end. I was trying to figure out the proper way to modify this constraint or modify the AVM/tracing when I realized.... We're going to delete this anyway since side effect counters shouldn't be a critical part of the protocol with everything in the AVM.

dbanks12 · 2024-11-05T18:31:12Z

noir-projects/noir-protocol-circuits/crates/types/src/constants.nr

-global DEFAULT_TEARDOWN_GAS_LIMIT: u32 = 100_000_000;
+global DEFAULT_TEARDOWN_GAS_LIMIT: u32 = 12_000_000;


There is no reason the teardown limit should be higher than the enqueued call limit

It maybe doesn't make sense to change this here, but I did so while debugging a test failure and thought... why change it back?

then maybe you can move the line past MAX_L2_GAS_PER_ENQUEUED_CALL and do

global DEFAULT_TEARDOWN_GAS_LIMIT = MAX_L2_GAS_PER_ENQUEUED_CALL;

?

dbanks12 · 2024-11-05T18:36:00Z

yarn-project/prover-client/src/orchestrator/orchestrator_public_functions.test.ts

@@ -56,93 +54,5 @@ describe('prover/orchestrator/public-functions', () => {
        expect(block.number).toEqual(context.blockNumber);
      },
    );
-
-    it('nested public calls', async () => {


test doesn't make sense any more because we aren't generating proofs per nested public call

dbanks12 · 2024-11-05T18:38:26Z

yarn-project/simulator/src/avm/avm_simulator.test.ts

 describe('AVM simulator: transpiled Noir contracts', () => {
+  it('bulk testing', async () => {


Made some debugging easier so i didn't have to run the bb-prover tests. This is probably good to have here anyway.

dbanks12 · 2024-11-05T18:39:50Z

yarn-project/simulator/src/avm/index.ts

@@ -1 +1,2 @@
 export * from './avm_simulator.js';
+export * from './journal/index.js';


I think the journal needs to be renamed StateManager and moved to the src/public/ folder. Will do so in a follow-up PR.

dbanks12 · 2024-11-05T18:41:50Z

yarn-project/simulator/src/avm/journal/journal.ts

    // TODO(5818): make private once no longer accessed in executor
-    public readonly publicStorage: PublicStorage,
+    public readonly trace: PublicSideEffectTraceInterface,


Made public for now.... Otherwise I need to keep adding passthrough functions to the trace. Eventually, the only place outside the AVM where the trace will be accessed will be in the enqueued calls processor where the trace instance is first constructed. At that point we should be able to make this private again since the instance is directly accessible there.

dbanks12 · 2024-11-05T19:27:00Z

yarn-project/txe/src/oracle/txe_oracle.ts

@@ -219,11 +220,36 @@ export class TXE implements TypedOracle {
    return this.txeDatabase.addAuthWitness(authWitness.requestHash, authWitness.witness);
  }

-  async addNullifiers(contractAddress: AztecAddress, nullifiers: Fr[]) {
+  async addPublicDataWrites(writes: PublicDataUpdateRequest[]) {


public executor no longer commits to worldStateDB, so we need to manually perform tree insertions for public data writes here

TY David! LGTM, provided the current TXE test suite passes (which it does!)

Seems like we don't have to build the CombinedConstantData in its entirety now, is this going to stay that way? If it is, I'll leave just the GlobalVariables struct in executePublicFunction in an upcoming refactor PR, should clean it up quite nicely.

Hmm I think it will stay that way, at least for the TXE's interface to the executor

…lls in tx

yarn-project/simulator/src/public/side_effect_trace.ts

LeilaWang

LGTM! 🚀

yarn-project/simulator/src/public/enqueued_call_simulator.ts

yarn-project/simulator/src/public/enqueued_calls_processor.ts

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

fcarreiro

I think this is a good step forwards.

I still don't have a lot of clarity on how things will end up looking so I think the best (for me) is to see where we land and then once we think we have the final version, I can judge it better :)

yarn-project/simulator/src/avm/avm_simulator.test.ts

yarn-project/simulator/src/avm/journal/journal.ts

yarn-project/simulator/src/mocks/fixtures.ts

yarn-project/simulator/src/public/enqueued_call_side_effect_trace.ts

yarn-project/simulator/src/public/enqueued_calls_processor.ts

fcarreiro · 2024-11-06T15:27:51Z

yarn-project/simulator/src/public/enqueued_calls_processor.ts

@@ -265,17 +331,28 @@ export class EnqueuedCallsProcessor {
        );
        throw enqueuedCallResult.revertReason;
      }
+      await txStateManager.mergeStateForEnqueuedCall(


Note to self: I'm wondering why we can't have a generic accept/reject and we need a special one for enqueued call?

Because this merge function traces the enqueued call under the hood, adding it to hints & vector that ultimately becomes public inputs

Right now, this is how we handle all "merges of forked state". We merge the forked state, and we trace the thing that caused the fork. It's not pretty....

Maybe the merge and trace should be separate functions? So we first do mergeForkedState which is always the same no matter what. And then we call a tailored trace function?

yarn-project/simulator/src/public/public_processor.test.ts

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

fcarreiro · 2024-11-06T15:46:21Z

noir-projects/noir-protocol-circuits/crates/types/src/constants.nr

-global DEFAULT_TEARDOWN_GAS_LIMIT: u32 = 100_000_000;
+global DEFAULT_TEARDOWN_GAS_LIMIT: u32 = 12_000_000;


then maybe you can move the line past MAX_L2_GAS_PER_ENQUEUED_CALL and do

global DEFAULT_TEARDOWN_GAS_LIMIT = MAX_L2_GAS_PER_ENQUEUED_CALL;

?

dbanks12 · 2024-11-06T17:55:44Z

Resolving comments in #9785

dbanks12 · 2024-11-06T17:56:22Z

then maybe you can move the line past MAX_L2_GAS_PER_ENQUEUED_CALL and do
global DEFAULT_TEARDOWN_GAS_LIMIT = MAX_L2_GAS_PER_ENQUEUED_CALL;

I could, but I think that the max-per-enqueued call might go away and be replaced by a limit for the whole TX (minus teardown)

dbanks12 requested a review from fcarreiro as a code owner October 30, 2024 01:30

dbanks12 changed the title ~~chore: share state manager and side effect tracer for all enqueued calls in tx~~ [WIP] chore: share state manager and side effect tracer for all enqueued calls in tx Oct 30, 2024

dbanks12 force-pushed the db/tx-level-state-and-trace branch from 75e6b4b to 15de5d9 Compare October 30, 2024 02:00

dbanks12 removed the request for review from fcarreiro October 30, 2024 02:11

dbanks12 force-pushed the db/tx-level-state-and-trace branch from 15de5d9 to f3891e6 Compare October 30, 2024 18:59

dbanks12 requested review from jeanmon and IlyasRidhuan as code owners October 30, 2024 18:59

dbanks12 force-pushed the db/tx-level-state-and-trace branch 17 times, most recently from 26df580 to 46bd46f Compare November 2, 2024 16:23

dbanks12 mentioned this pull request Nov 3, 2024

[WIP] chore: refactoring public executor interface #9701

Merged

dbanks12 commented Nov 5, 2024

View reviewed changes

dbanks12 force-pushed the db/tx-level-state-and-trace branch 2 times, most recently from 5b22d9f to 58bb2b7 Compare November 5, 2024 19:26

dbanks12 commented Nov 5, 2024

View reviewed changes

dbanks12 force-pushed the db/tx-level-state-and-trace branch 2 times, most recently from 84c628a to 8227f99 Compare November 5, 2024 21:04

dbanks12 removed request for jeanmon and IlyasRidhuan November 5, 2024 21:12

chore: share state manager and side effect tracer for all enqueued ca…

21635e6

…lls in tx

dbanks12 force-pushed the db/tx-level-state-and-trace branch from 8227f99 to c5f6d3a Compare November 6, 2024 00:49

dbanks12 changed the title ~~[WIP] chore: share state manager and side effect tracer for all enqueued calls in tx~~ chore: share state manager and side effect tracer for all enqueued calls in tx Nov 6, 2024

dbanks12 requested review from LeilaWang, fcarreiro and Thunkar November 6, 2024 00:59

dbanks12 force-pushed the db/tx-level-state-and-trace branch from c5f6d3a to 9bf1802 Compare November 6, 2024 01:06

dbanks12 commented Nov 6, 2024

View reviewed changes

yarn-project/simulator/src/public/side_effect_trace.ts Show resolved Hide resolved

[WIP] chore: refactoring public executor interface (#9701)

c4b624d

dbanks12 force-pushed the db/tx-level-state-and-trace branch from 9bf1802 to c4b624d Compare November 6, 2024 01:36

LeilaWang approved these changes Nov 6, 2024

View reviewed changes

yarn-project/simulator/src/public/enqueued_call_simulator.ts Show resolved Hide resolved

yarn-project/simulator/src/public/enqueued_calls_processor.ts Show resolved Hide resolved

yarn-project/simulator/src/public/executor.ts Show resolved Hide resolved

fcarreiro approved these changes Nov 6, 2024

View reviewed changes

dbanks12 merged commit e3756fd into master Nov 6, 2024
68 checks passed

dbanks12 deleted the db/tx-level-state-and-trace branch November 6, 2024 16:00

AztecBot mentioned this pull request Nov 6, 2024

chore(master): Release 0.63.0 #9651

Merged

LeilaWang linked an issue Nov 12, 2024 that may be closed by this pull request

Remove public kernel inner. #9594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: share state manager and side effect tracer for all enqueued calls in tx #9565

chore: share state manager and side effect tracer for all enqueued calls in tx #9565

dbanks12 commented Oct 30, 2024 •

edited

Loading

dbanks12 commented Oct 30, 2024 •

edited

Loading

dbanks12 Nov 5, 2024

dbanks12 Nov 5, 2024

dbanks12 Nov 6, 2024

fcarreiro Nov 6, 2024

dbanks12 Nov 5, 2024

dbanks12 Nov 5, 2024

dbanks12 Nov 5, 2024

dbanks12 Nov 5, 2024 •

edited

Loading

dbanks12 Nov 5, 2024

Thunkar Nov 6, 2024

dbanks12 Nov 6, 2024

LeilaWang left a comment

fcarreiro left a comment

fcarreiro Nov 6, 2024

dbanks12 Nov 6, 2024

dbanks12 Nov 6, 2024

fcarreiro Nov 6, 2024

dbanks12 commented Nov 6, 2024

dbanks12 commented Nov 6, 2024

		global DEFAULT_TEARDOWN_GAS_LIMIT: u32 = 100_000_000;
		global DEFAULT_TEARDOWN_GAS_LIMIT: u32 = 12_000_000;

		describe('AVM simulator: transpiled Noir contracts', () => {
		it('bulk testing', async () => {

		@@ -1 +1,2 @@
		export * from './avm_simulator.js';
		export * from './journal/index.js';

chore: share state manager and side effect tracer for all enqueued calls in tx #9565

chore: share state manager and side effect tracer for all enqueued calls in tx #9565

Conversation

dbanks12 commented Oct 30, 2024 • edited Loading

dbanks12 commented Oct 30, 2024 • 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

dbanks12 Nov 5, 2024 • 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

LeilaWang left a comment

Choose a reason for hiding this comment

fcarreiro 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

dbanks12 commented Nov 6, 2024

dbanks12 commented Nov 6, 2024

dbanks12 commented Oct 30, 2024 •

edited

Loading

dbanks12 commented Oct 30, 2024 •

edited

Loading

dbanks12 Nov 5, 2024 •

edited

Loading