chore: VisitedPcs trait in CachedState
#3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Eagle941
force-pushed
the
visited_pcs_trait
branch
7 times, most recently
from
7ade621 to
111668e
Compare
Eagle941
force-pushed
the
visited_pcs_trait
branch
from
111668e to
e9711f9
Compare
iamrecursion
suggested changes
Sep 3, 2024
There was a problem hiding this comment.
Great start, but definitely needs some tweaks.
There are a lot of type annotations that good API design should make unnecessary as inference should work, and generally there are a few places that are missing docs that absolutely need them.
There are also a bunch of places where there is a type with VisitedPcsSet specified. It probably makes sense to have aliases for most of these: DefaultStatefulValidator, DefaultCachedState and so on. We don't want it to be inconvenient for the Starkware folks to use the default case in the Blockifier.
|
Also note that some of your CI is failing; not sure what "commitlint" is meant to do, but clippy should absolutely be passing. |
Eagle941
force-pushed
the
visited_pcs_trait
branch
2 times, most recently
from
df55eed to
2ea8e1c
Compare
Eagle941
changed the title
VisitedPcs trait in CachedStateVisitedPcs trait in CachedState
Eagle941
force-pushed
the
visited_pcs_trait
branch
3 times, most recently
from
7042bd1 to
12f942e
Compare
Eagle941
force-pushed
the
visited_pcs_trait
branch
2 times, most recently
from
23d8fbc to
9ba3e92
Compare
Eagle941
force-pushed
the
visited_pcs_trait
branch
from
9ba3e92 to
441544a
Compare
Eagle941
force-pushed
the
visited_pcs_trait
branch
from
441544a to
d98eacd
Compare
Eagle941
force-pushed
the
visited_pcs_trait
branch
9 times, most recently
from
2e739ea to
f41cd77
Compare
Eagle941
force-pushed
the
visited_pcs_trait
branch
from
f41cd77 to
2458cc1
Compare
Eagle941
force-pushed
the
visited_pcs_trait
branch
7 times, most recently
from
59a5f1d to
65f30bc
Compare
Co-authored-by: Ara Adkins <[email protected]>
Eagle941
force-pushed
the
visited_pcs_trait
branch
from
65f30bc to
a581585
Compare
iamrecursion
approved these changes
Sep 19, 2024
Eagle941
force-pushed
the
visited_pcs_trait
branch
from
897c5c4 to
e3141f6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change introduces the trait
VisitedPcsinCachedStatefor flexibility in the handling of visited program counters returned from an entry point call incairo_vm.The structure
CachedStatecontains the fieldvisited_pcsof typeHashSet<usize>to contain the set of visited program counters. The development of a Cairo 1 profiler requires storage of the full list of program counters. To add this flexibility in the blockifier, the type ofvisited_pcshas been replaced with the traitVisitedPcsand an implementation replicating the current behaviour, usingHashSet, has been added with the nameVisitedPcsSet.The default instantiation of
CachedStatein unit tests usesVisitedPcsSet.Native blockifier is hardcoded to use
VisitedPcsSetbecause it's only a bridge the legacy Python sequencer.