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.

Sign up for GitHub

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

Jump to bottom

refactor: stop calling public kernels #9971

Merged
merged 2 commits into from
Nov 18, 2024
Merged

refactor: stop calling public kernels #9971

merged 2 commits into from
Nov 18, 2024

Conversation

dbanks12
Copy link
Collaborator

No description provided.

@dbanks12 Graphite App
Copy link
Collaborator Author

dbanks12 commented Nov 14, 2024 •
edited
Loading

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

@dbanks12 dbanks12 mentioned this pull request Nov 14, 2024
Merged
@dbanks12 dbanks12 force-pushed the db/refactor-enqueued-calls-processor branch 6 times, most recently from 065a139 to 889d1c9 Compare November 14, 2024 20:50
@dbanks12 dbanks12 force-pushed the db/stop-calling-kernels branch from 0ba3950 to 269acb7 Compare November 14, 2024 22:26
@dbanks12 dbanks12 changed the title refactor: stop calling public kernels [WIP] refactor: stop calling public kernels Nov 14, 2024
@dbanks12 dbanks12 marked this pull request as ready for review November 14, 2024 22:27
@dbanks12 dbanks12 requested a review from fcarreiro as a code owner November 14, 2024 22:27
@dbanks12 dbanks12 removed the request for review from fcarreiro November 14, 2024 22:27
@dbanks12 dbanks12 changed the title [WIP] refactor: stop calling public kernels [WIP] refactor: use AvmCircuitPublicInputs generated by simulator instead of kernel tail Nov 14, 2024
@dbanks12 dbanks12 force-pushed the db/stop-calling-kernels branch from 269acb7 to 4627edb Compare November 15, 2024 01:54
@dbanks12 dbanks12 mentioned this pull request Nov 15, 2024
Merged
@dbanks12 dbanks12 changed the title [WIP] refactor: use AvmCircuitPublicInputs generated by simulator instead of kernel tail refactor: use AvmCircuitPublicInputs generated by simulator instead of kernel tail Nov 15, 2024
@dbanks12 dbanks12 requested review from fcarreiro and LeilaWang and removed request for fcarreiro and LeilaWang November 15, 2024 03:03
Base automatically changed from db/refactor-enqueued-calls-processor to master November 15, 2024 15:10
@dbanks12 dbanks12 marked this pull request as draft November 15, 2024 15:38
@dbanks12 dbanks12 force-pushed the db/stop-calling-kernels branch from 4627edb to 1d3aaa8 Compare November 15, 2024 15:39
@dbanks12 dbanks12 added the e2e-all CI: Enables this CI job. label Nov 15, 2024
@dbanks12 dbanks12 changed the title refactor: use AvmCircuitPublicInputs generated by simulator instead of kernel tail [WIP] refactor: use AvmCircuitPublicInputs generated by simulator instead of kernel tail Nov 15, 2024
@dbanks12 dbanks12 marked this pull request as ready for review November 15, 2024 17:32
@dbanks12 dbanks12 force-pushed the db/stop-calling-kernels branch from 1d3aaa8 to 9a4706b Compare November 15, 2024 17:41
@dbanks12 dbanks12 changed the title [WIP] refactor: use AvmCircuitPublicInputs generated by simulator instead of kernel tail refactor: stop calling public kernels Nov 16, 2024
@dbanks12 dbanks12 force-pushed the db/stop-calling-kernels branch from 5a77cd3 to 7e3c14c Compare November 16, 2024 20:52
@dbanks12 dbanks12 mentioned this pull request Nov 17, 2024
Merged
fcarreiro
fcarreiro reviewed Nov 18, 2024
View reviewed changes
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.

LGTM but I'll let Leila approve due to the kernel related changes.

yarn-project/prover-node/src/prover-node.ts
@@ -56,7 +56,7 @@ export class ProverNode implements ClaimsMonitorHandler, EpochMonitorHandler, Pr
private readonly contractDataSource: ContractDataSource,
private readonly worldState: WorldStateSynchronizer,
private readonly coordination: ProverCoordination & Maybe<Service>,
private readonly simulator: SimulationProvider,
private readonly _simulator: SimulationProvider,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be marked as "to be deleted"?

yarn-project/sequencer-client/src/client/sequencer-client.ts
@@ -40,13 +40,13 @@ export class SequencerClient {
contractDataSource: ContractDataSource,
l2BlockSource: L2BlockSource,
l1ToL2MessageSource: L1ToL2MessageSource,
simulationProvider: SimulationProvider,
_simulationProvider: SimulationProvider,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Do we have plans to remove this? Or will it be used in another PR?

yarn-project/simulator/src/avm/journal/journal.ts
Comment on lines +118 to +119
// sanity check to avoid merging the same forked trace twice
assert(!this.alreadyMergedIntoParent, 'Cannot merge forked state that has already been merged into its parent!');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so, this.merge merges forkedState INTO this, IIUC? Why does it matter if this. alreadyMergedIntoParent is true? Shouldn't we be checking for forkedState. alreadyMergedIntoParent ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where alreadyMergedIntoParent is set to true (for the journal itself, I see it for the trace).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The parent doesn't keep track of its forks. And it could have children formed and merged multiple times.

We just don't want to let a specific fork get merged multiple times.

And good catch, I must've missed that in journal

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, you're right. I was confused. It should check forkedState.alreadyMerged! Good catch.

yarn-project/simulator/src/public/enqueued_call_side_effect_trace.ts
Comment on lines +184 to +186
// sanity check to avoid merging the same forked trace twice
assert(!this.alreadyMergedIntoParent, 'Cannot merge a forked trace that has already been merged into its parent!');
forkedTrace.alreadyMergedIntoParent = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about this vs forkedTrace

LeilaWang
LeilaWang approved these changes Nov 18, 2024
View reviewed changes
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.

LGTM!

@dbanks12 dbanks12 merged commit d86a9d9 into master Nov 18, 2024
100 checks passed
@dbanks12 dbanks12 deleted the db/stop-calling-kernels branch November 18, 2024 12:56
@AztecBot AztecBot mentioned this pull request Nov 18, 2024
Merged
TomAFrench added a commit that referenced this pull request Nov 18, 2024
@TomAFrench
Merge branch 'master' into sync-noir
4d49f2b 
* 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)
  ...
TomAFrench added a commit that referenced this pull request Nov 19, 2024
@TomAFrench
Merge branch 'master' into tf/evaluator-changes
afda45d 
* master: (67 commits)
  chore: Fix bad merge on AztecLMDBStore initializer
  feat: add persisted database of proving jobs (#9942)
  chore: Clean up data configuration (#9973)
  chore: remove public kernels (#10027)
  chore: misc cleanup, docs and renaming (#9968)
  feat: IPA Accumulator in Builder (#9846)
  chore(docs): Updates to token contract (#9954)
  test(avm): minor benchmarking (#9869)
  chore(ci): run `l1-contracts` CI in parallel with `build` step (#10024)
  chore: build acir test programs in parallel to e2e build step (#9988)
  chore: pull out `array_set` pass changes (#9993)
  feat(avm): ephemeral avm tree (#9798)
  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
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fcarreiro fcarreiro fcarreiro left review comments

@LeilaWang LeilaWang LeilaWang approved these changes

Assignees
No one assigned
Labels
e2e-all CI: Enables this CI job.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dbanks12 @fcarreiro @LeilaWang