Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: enqueued calls processor -> public tx simulator #9919

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

dbanks12
Copy link
Collaborator

@dbanks12 dbanks12 commented Nov 13, 2024

  • Rename EnqueuedCallsProcessor -> PublicTxSimulator
  • Introduce PublicTxContext
  • Refactor EnqueuedCallsProcessor with functions per phase that use PublicTxContext
  • Split up tracing of enqueued calls and the merging of their state & traces (too confusing if they're coupled)
  • Lots of prep for removal of public kernels
  • Misc cleanup

Peek at the other PRs in this stack to see how things will simplify further!

I am open to all feedback when it comes to the refactor of EnqueuedCallsProcessor, this new PublicTxContext, state forking, etc.

@dbanks12 dbanks12 requested a review from fcarreiro as a code owner November 13, 2024 03:01
@dbanks12 dbanks12 removed the request for review from fcarreiro November 13, 2024 13:38
@dbanks12 dbanks12 force-pushed the db/refactor-enqueued-calls-processor branch from de18982 to 1ff73fb Compare November 14, 2024 17:20
Copy link
Collaborator Author

dbanks12 commented Nov 14, 2024

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

@dbanks12 dbanks12 force-pushed the db/refactor-enqueued-calls-processor branch from 1ff73fb to eeba295 Compare November 14, 2024 18:17
@dbanks12 dbanks12 force-pushed the db/refactor-enqueued-calls-processor branch 4 times, most recently from 405bc84 to 065a139 Compare November 14, 2024 20:48
@dbanks12 dbanks12 force-pushed the db/refactor-enqueued-calls-processor branch from 065a139 to 889d1c9 Compare November 14, 2024 20:50
@dbanks12 dbanks12 changed the title [WIP] refactor: enqueued calls processor refactor: enqueued calls processor -> public tx simulator Nov 15, 2024
Comment on lines +177 to +178
const callRequests = context.getCallRequestsForCurrentPhase();
const executionRequests = context.getExecutionRequestsForCurrentPhase();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all we need from PublicExecutionRequests is args, we probably can just work with PublicCallRequests & args instead of PublicCallRequests & PublicExecutionRequests

Copy link
Contributor

@fcarreiro fcarreiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think it looks better. It would be great if we could get rid of some of the duplicated phase-tracking logic though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some docstrings on the classes?

import { PublicSideEffectTrace } from './side_effect_trace.js';
import { getCallRequestsByPhase, getExecutionRequestsByPhase, getPublicKernelCircuitPublicInputs } from './utils.js';

class PhaseStateManager {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this class?

yarn-project/simulator/src/public/utils.ts Show resolved Hide resolved
Copy link
Collaborator

@LeilaWang LeilaWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Facundo's comments on explicitly specifying phase and making only one component manage the phases (if possible). Rest looks great to me! Nice refactoring 🎉

@dbanks12 dbanks12 requested a review from fcarreiro November 15, 2024 14:40
Copy link
Contributor

@fcarreiro fcarreiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving under the oath that the comments will be resolved in another PR ;)

@dbanks12 dbanks12 merged commit cae7279 into master Nov 15, 2024
69 of 70 checks passed
@dbanks12 dbanks12 deleted the db/refactor-enqueued-calls-processor branch November 15, 2024 15:10
TomAFrench added a commit that referenced this pull request Nov 18, 2024
* master: (281 commits)
  fix: don't take down runners with faulty runner check (#10019)
  feat(docs): add transaction profiler docs (#9932)
  chore: hotfix runner wait (#10018)
  refactor: remove EnqueuedCallSimulator (#10015)
  refactor: stop calling public kernels (#9971)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  chore: drop info to verbose in sequencer hot loop (#9983)
  refactor: Trace structure is an object (#10003)
  refactor: enqueued calls processor -> public tx simulator (#9919)
  chore: World state tech debt cleanup 1 (#9561)
  chore(ci): run noir tests in parallel to building e2e tests (#9977)
  Revert "chore: lower throughput of ebs disks" (#9996)
  feat: new proving broker implementation (#9400)
  chore: replace `to_radix` directive with brillig (#9970)
  chore: disable failing 48validator kind test (#9920)
  test: prove one epoch in kind (#9886)
  fix: formatting (#9979)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants