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

core/tracing: add system call callback when performing ProcessBeaconBlockRoot #29355

Merged
merged 2 commits into from May 6, 2024

Conversation

maoueh
Copy link
Contributor

@maoueh maoueh commented Mar 26, 2024

Added a start/end system where tracer can be notified that processing of some Ethereum system calls is starting processing and also notifies it when the processing has completed.

Doing a start/end for system call will enable tracers to "route" incoming next tracing events to go to a separate bucket than other EVM calls. Those not interested by this fact can simply avoid registering the hooks.

The EVM call is going to be traced normally afterward between the signals provided by those 2 new hooks but outside of a transaction context OnTxStart/End. That something implementors of live tracers will need to be aware of (since only "trx tracers" are not concerned by ProcessBeaconRoot).

I've looked into providing some kind of unit tests for that but it prove harder than anticipated. Since this has a minimal implementation surface, it think it's fine but I can take a second look if deemed required.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@holiman
Copy link
Contributor

holiman commented Mar 27, 2024

What would a tracer do with this information?

EDIT: why not just use the fact that the tx sender is a magic system sender, and use that to distinguish?

@s1na
Copy link
Contributor

s1na commented Mar 27, 2024

EDIT: why not just use the fact that the tx sender is a magic system sender, and use that to distinguish?

The reason we added this is the tracer will be surprised to see a call frame enter outside of a tx. It's true that OnEnter we can check system address and figure out that we are in a system call, hence no OnTxStart was emitted. @maoueh what do you think?

@lightclient
Copy link
Member

I'm a little skeptical of the need for this. @maoueh could you elaborate on your intended use case? And if you're using EVM tracing to build an external DB of data, how do you deal with withdrawals which are directly applied to the state?

I could imagine that other downstream chains using go-ethereum might appreciate these modifications the tracer API though if they have developed more complex system calls.

@maoueh
Copy link
Contributor Author

maoueh commented Mar 27, 2024

@lightclient That is totally correct. Live tracer has an interest to whatever make state changes to the chain which is the case here with beacon root.

The EVM call that is being made is going to trace a StorageChange out when executing the contract because AFAIK, that's that the contract does.

We have a real use case today for this like you said which is a virtual archive node fed by state changes (storage,balance,nonce,etc.) generated by our Firehose tracer. We are able to then implement eth_call (and a few others) on top of that using distributed kvdb. You see how missing those thing can be problematic, I wouldn't be able to serve eth_call anymore that touchs this state because I would never have seen it.

Completely omitting those tracing is contrary to the live tracer spirit IMO.

It's true that I could infer from the block header and generate a StorageChange "manually" but having proper marker enable me to right now to support any future "system call" doing any kind of state changes.

About withdrawals, they generate a BalanceChange today with the now merged new tracing hooks, which also happens outside any transaction and "could" be seen as system level but here too, I wouldn't want to miss a balance change just because it's "system" based.

@maoueh
Copy link
Contributor Author

maoueh commented Mar 27, 2024

Now, how this can be done, there is different possibilities of course. I think the marker OnSystemCallStart/End is the easiest. You can them simply prepare yourself to treat upcoming events as part of a system call and you get the full model.

Quite open to check other possibilities, a specific OnProcessBeaconRootStart/End would be quite fine too and more specific. With the way hooks are made, shouldn't be a problem to add more stuff later.

Just a OnProcessBeaconRoot would prove more brittle for me as when receiving the signal, I would "trap" the next OnEnter/OnExit sequence. Less ideal but I've did worst :)

@maoueh
Copy link
Contributor Author

maoueh commented Mar 27, 2024

@lightclient Just saw your other PR, going to take a look.

@maoueh
Copy link
Contributor Author

maoueh commented Mar 27, 2024

For extra clarity, actually right now I get all that data I need from the live tracer even without this PR because the EVM and StateDB are still correctly traced so storage changes are properly sent. I'm just unable to group them into a "special" bucket "system_call" which is more contextual.

@lightclient
Copy link
Member

Thanks @maoueh, I think I understand now. This PR seems reasonable.

@maoueh
Copy link
Contributor Author

maoueh commented Mar 27, 2024

why not just use the fact that the tx sender is a magic system sender, and use that to distinguish?

I would be fine with this too, but it seems more and more "system call" are considered from that I saw in @lightclient PR. I think it would make sense to have something that works well with all those. Of course not alway easy to design future proof API from the start.

It would be fine waiting a bit too to ensure we correctly cover other currently planned system calls.

@maoueh maoueh closed this Apr 17, 2024
@maoueh maoueh deleted the feature/system-call-tracing branch April 17, 2024 17:14
@maoueh maoueh restored the feature/system-call-tracing branch April 24, 2024 13:22
@maoueh
Copy link
Contributor Author

maoueh commented Apr 24, 2024

Closed by mistake

@maoueh maoueh reopened this Apr 24, 2024
@holiman
Copy link
Contributor

holiman commented Apr 30, 2024

why not just use the fact that the tx sender is a magic system sender, and use that to distinguish?

I would be fine with this too, but it seems more and more "system call" are considered from that I saw in @lightclient PR. I think it would make sense to have something that works well with all those. Of course not alway easy to design future proof API from the start.

It would work for all of them -- they all use the same magic system sender, don't they?

@holiman holiman changed the title Added system call tracing when performing ProcessBeaconBlockRoot core: add system call callback when performing ProcessBeaconBlockRoot Apr 30, 2024
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Yeah ok

@fjl fjl added this to the 1.14.1 milestone Apr 30, 2024
@holiman
Copy link
Contributor

holiman commented Apr 30, 2024

core/tracing/hooks.go:184: File is not `goimports`-ed (goimports)
  OnClose          CloseHook
util.go:47: exit status 1

@maoueh
Copy link
Contributor Author

maoueh commented Apr 30, 2024

core/tracing/hooks.go:184: File is not goimports-ed (goimports)

Hmm, I'll check that

Added a `start/end` system where tracer can be notified that processing of some Ethereum system calls is starting processing and also notifies it when the processing has completed.

Doing a `start/end` for system call will enable tracers to "route" incoming next tracing events to go to a separate bucket than other EVM calls. Those not interested by this fact can simply avoid registering the hooks.

The EVM call is going to be traced normally afterward between the signals provided by those 2 new hooks but outside of a transaction context `OnTxStart/End`. That something implementors of live tracers will need to be aware of (since only "trx tracers" are not concerned by `ProcessBeaconRoot`).

One thing of importance for tracers
@maoueh maoueh force-pushed the feature/system-call-tracing branch from c02c1d9 to 8bec7ef Compare April 30, 2024 21:08
@maoueh
Copy link
Contributor Author

maoueh commented Apr 30, 2024

@holiman Can you make the CI re-run? Locally I cannot see any lints error using go run build/ci.go lint, I've rebased the work now to check if it's gonna help.

@fjl
Copy link
Contributor

fjl commented May 2, 2024

The CI is ok now. Please add the new hooks to the changelog in core/tracing/changelog.md

@s1na
Copy link
Contributor

s1na commented May 2, 2024

Updated the changelog here s1na@827f13c but can't push it to the PR directly as the organization permissions don't allow it.

@maoueh
Copy link
Contributor Author

maoueh commented May 3, 2024

@fjl @s1na Commit added

@fjl fjl changed the title core: add system call callback when performing ProcessBeaconBlockRoot core/tracing: add system call callback when performing ProcessBeaconBlockRoot May 6, 2024
@fjl fjl merged commit a09a610 into ethereum:master May 6, 2024
1 of 2 checks passed
@maoueh maoueh deleted the feature/system-call-tracing branch May 6, 2024 14:37
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.

None yet

5 participants