Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: add mux tracer for native call tracer and native prestate tracers #615

Merged
merged 29 commits into from
Jan 18, 2024

Conversation

0xmountaintop
Copy link

@0xmountaintop 0xmountaintop commented Jan 11, 2024

1. Purpose or design rationale of this PR

  1. continued work based on [DO NOT MERGE] hack to use native tracers #557. background see Add call and prestate tracer to l2trace code #594
  2. have to upgrade go version because eth/tracers/native/prestate.go uses some new go feature
  3. then need to upgrade golangci-lint because the previous version isn't compatible with go1.20
  4. use interface and move core/trace.go to avoid import cycle
  5. suggest ignoring lint CI now, because most of the hints make little sense, and the formatting introduces a lot of diff

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

@0xmountaintop 0xmountaintop force-pushed the add-tracers-2 branch 2 times, most recently from bc03745 to 8ba7ebb Compare January 11, 2024 09:49
@0xmountaintop 0xmountaintop force-pushed the add-tracers-2 branch 2 times, most recently from 5d50d9e to 7859e54 Compare January 11, 2024 10:05
@0xmountaintop 0xmountaintop marked this pull request as ready for review January 11, 2024 11:59
@0xmountaintop 0xmountaintop force-pushed the add-tracers-2 branch 2 times, most recently from 3a2d08c to 4a8f9fb Compare January 12, 2024 03:11
@0xmountaintop 0xmountaintop changed the title Add tracers 2 feat: add native prestate tracers Jan 15, 2024
lightsing
lightsing previously approved these changes Jan 16, 2024
georgehao
georgehao previously approved these changes Jan 17, 2024
@0xmountaintop 0xmountaintop changed the title feat: add native prestate tracers feat: add mux tracer for native call tracer and native prestate tracers Jan 18, 2024
@0xmountaintop 0xmountaintop dismissed stale reviews from georgehao and lightsing via acf23b7 January 18, 2024 05:50
@0xmountaintop 0xmountaintop merged commit 88349c6 into develop Jan 18, 2024
4 of 5 checks passed
@0xmountaintop 0xmountaintop deleted the add-tracers-2 branch January 18, 2024 06:58
@lispc
Copy link

lispc commented Feb 1, 2024

i think after this PR, traces only add more field but not remove existed fields. So in thoery it should be compatible with existed prover? @HAOYUatHZ

@0xmountaintop
Copy link
Author

i think after this PR, traces only add more field but not remove existed fields. So in thoery it should be compatible with existed prover? @HAOYUatHZ

yes this PR is compatible (see https://scrollco.slack.com/archives/C05UJL77FAR/p1705908908525729?thread_ts=1704082609.850259&cid=C05UJL77FAR)

but after we "disable stack", it will be incompatible and circuits need to be upgraded

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants