core/tracing: state journal wrapper #30441
Conversation
s1na
requested review from
karalabe,
holiman and
rjl493456442
as code owners
core/tracing/hooks.go
Outdated
| NonceReadHook = func(addr common.Address, nonce uint64) | ||
|
|
||
| // CodeReadHook is called when EVM reads the code of an account. | ||
| CodeReadHook = func(addr common.Address, code []byte) |
There was a problem hiding this comment.
Open question: should we add codeHash here to be consistent with OnCodeChange?
|
Ah seems like the journal has a crasher: |
core/tracing/CHANGELOG.md
Outdated
|
|
||
| ### New methods | ||
|
|
||
| - `OnReorg(reverted []*types.Block)`: This hook is called when a reorg is detected. The `reverted` slice contains the blocks that are no longer part of the canonical chain. |
There was a problem hiding this comment.
Here types block is very very heavy. You should at most pass headers and allow chain access to pull the blocks on demand (chain access in someconstructor, ha)
There was a problem hiding this comment.
On second thought what is the issue? it is a slice so passed by reference and the memory can be freed as soon as OnReorg processing is done.
There was a problem hiding this comment.
Ugh, this is annoying. So reorg in the blockchain at some point in the past used to collect blocks. Turned out that sometimes it became insanely heavy and we've switched so it operates on headers. I guess later someone refactored it back to operate on blocks again. This is an issue when you do setHead or any similar operation; of even if finality fails for a while and you have blocks reorging back and forth. It's very very bad to pull all the block in from disk IMO.
CC @holiman @rjl493456442 ?
There was a problem hiding this comment.
I agree. I don't particularly recall switching from headers to blocks....
core/tracing/hooks.go
Outdated
| @@ -194,6 +221,30 @@ type Hooks struct { | |||
| OnCodeChange CodeChangeHook | |||
| OnStorageChange StorageChangeHook | |||
| OnLog LogHook | |||
| // State reads | |||
| OnBalanceRead BalanceReadHook | |||
| OnNonceRead NonceReadHook | |||
There was a problem hiding this comment.
Question from triage: how exactly is OnNonceRead used?
core/tracing/journal.go
Outdated
| validRevisions []revision | ||
| nextRevisionId int | ||
| revIds []int |
There was a problem hiding this comment.
I don't see why you need to track this. Why not just maintain a list of entries, and you hand out the id which is the current length of the entries?
core/tracing/journal.go
Outdated
| type journal struct { | ||
| entries []entry | ||
| hooks *Hooks | ||
| lastCreator *common.Address // Account that initiated the last contract creation |
There was a problem hiding this comment.
This looks like a hack. I don't see how this can be accurately updated going forward and backward along the entries. I mean, an inner scope will overwrite the outer lastCreator, and when the inner scope is reverted, the lastCreator will not be set back correctly.
There was a problem hiding this comment.
Or if we're inside a creation, and inside the constructor we call ripemd to calculate a signature: we lost lastCreator.
core/tracing/hooks.go
Outdated
| dstValue.Field(i).Set(field) | ||
| } | ||
| } | ||
| return copied |
There was a problem hiding this comment.
Is this not equivalent to
copied := *h
return &copied
| @@ -163,6 +171,9 @@ type ( | |||
| // NonceChangeHook is called when the nonce of an account changes. | |||
| NonceChangeHook = func(addr common.Address, prev, new uint64) | |||
|
|
|||
| // NonceChangeHookV2 is called when the nonce of an account changes. | |||
| NonceChangeHookV2 = func(addr common.Address, prev, new uint64, reason NonceChangeReason) | |||
There was a problem hiding this comment.
The V2 isn't helpful, we should use NonceChangeHokWithReason. Function names should be explict in what they do.
There was a problem hiding this comment.
We have chosen this naming scheme explicitly. The intent is communicating which version of the hook is the newest one. When we introduce a new hook version, the old one becomes deprecated and will eventually be unsupported. Also, if we were to introduce another revision of this hook, would it be called NonceChangeHookWithReasonAndBellsAndWhistles? The *Vx naming scheme avoids this problem.
There was a problem hiding this comment.
When the old one is removed, you can remove the WithReason part, but in the meantime, you know at a glance why the two methods differ. I don't think there's a big risk of that function being rewritten many times and, therefore, having the names getting longer and longer. But sure, just giving my opinion on what could make the code readable, not going to hold the release for that one 🤷
There was a problem hiding this comment.
We cannot rename the function because it is a stable, user-exposed API. If we could just change it, we wouldn't go through the trouble of having multiple versions of the hook.
Here we add some more changes for live tracing API v1.1: - Hook `OnSystemCallStartV2` was introduced with `VMContext` as parameter. - Hook `OnBlockHashRead` was introduced. - `GetCodeHash` was added to the state interface - The new `WrapWithJournal` construction helps with tracking EVM reverts in the tracer. --------- Co-authored-by: Felix Lange <fjl@twurst.com>
Here we add some more changes for live tracing API:
OnSystemCallStartV2was introduced withVMContextas parameter.OnBlockHashReadwas introduced.GetCodeHashwas added to the state interfaceWrapWithJournalconstruction helps with tracking EVM reverts in the tracer.