core/tracing: state journal wrapper #30441

s1na · 2024-09-16T08:46:58Z

Implements #30356

s1na · 2024-09-16T11:05:26Z

core/tracing/hooks.go

+	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)


Open question: should we add codeHash here to be consistent with OnCodeChange?

core/tracing/CHANGELOG.md

s1na · 2024-10-08T16:21:14Z

Ah seems like the journal has a crasher:

revisions: [{0 2} {1 4} {2 4} {3 4} {4 6} {5 6} {6 9} {7 11} {8 12} {9 18} {10 18} {11 20} {12 22} {13 24} {14 24} {18 27}]
panic: revision id 17 cannot be reverted

goroutine 10470 [running]:
github.com/ethereum/go-ethereum/core/tracing.(*journal).revertToSnapshot(0xc050c41c70, 0x11, 0xc0570360e0)
        github.com/ethereum/go-ethereum/core/tracing/journal.go:170 +0x185
github.com/ethereum/go-ethereum/core/tracing.(*journal).OnExit(0xc050c41c70, 0x0, {0xc13a87fe30, 0x64, 0x64}, 0x48dc9, {0x203f680, 0xc018bcc978}, 0x1)
        github.com/ethereum/go-ethereum/core/tracing/journal.go:206 +0x6f
github.com/ethereum/go-ethereum/core/vm.(*EVM).captureEnd(0xc13a9e0780?, 0x0, 0x12e208, 0xe543f, {0xc13a87fe30, 0x64, 0x64}, {0x203da40, 0x2e05070})

core/tracing/journal_test.go

karalabe · 2024-10-10T09:11:27Z

core/tracing/CHANGELOG.md

+
+### 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.


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)

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.

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 ?

I agree. I don't particularly recall switching from headers to blocks....

core/tracing/hooks.go

s1na · 2024-10-10T09:23:23Z

core/tracing/hooks.go

@@ -194,6 +221,30 @@ type Hooks struct {
 	OnCodeChange    CodeChangeHook
 	OnStorageChange StorageChangeHook
 	OnLog           LogHook
+	// State reads
+	OnBalanceRead  BalanceReadHook
+	OnNonceRead    NonceReadHook


Question from triage: how exactly is OnNonceRead used?

s1na · 2024-10-24T04:29:42Z

I have pulled in the latest master changes and moved the state read hooks over to the hooked statedb.

One thing to know about the state read hooks: They will not give you the full prestate by themselves. You will need also the previous values emitted as part of state change hooks. This is because e.g. statedb.AddBalance does not do statedb.GetBalance internally.

s1na · 2024-11-26T15:19:59Z

Copying from the chat with @nebojsa94:

Also, regarding jorunaling logic on your branch tracing V1.1, there’s an edge case with failed contract creation where the nonce is reverted, but it shouldn’t be. This happens because CaptureEnter is triggered before the nonce is incremented for contract creation prior to state snapshotting.

holiman · 2024-12-10T11:52:41Z

core/tracing/hooks.go

+}
+
+// Copy creates a new Hooks instance with all implemented hooks copied from the original.
+func (h *Hooks) Copy() *Hooks {


Why is this method needed? It's not obvious to me what the side-effects are. Typically, the hooks might be closures, and the closures are still referenced as if they were not copied.

func TestHooks(t *testing.T) { counter := 0 a := &Hooks{ OnClose: func() { counter++ }, } a.OnClose() t.Logf("counter is %d", counter) a.Copy().OnClose() t.Logf("counter is %d", counter) }

outputs:

hooks_test.go:13: counter is 1 hooks_test.go:15: counter is 2

I'm curious why you'd ever need to use this Copy method.

Is it because you want to copy all, but not have to specify all manually?

Typically, the hooks might be closures, and the closures are still referenced as if they were not copied.

Right you are correct I had not foreseen that. After thinking a bit, it feels like for my use-case that is fine. Essentially the clone will replace some of the methods to add some pre-processing logic. The rest are supposed to execute as in the original tracer.

I have un-exported the Copy method to avoid people to shoot themselves in the foot and added a comment to clarify this point.

Edit: right what I want is to add the journal in front of the tracer and process some of the hooks first before proxying back to tracer. And this without having to iterate the list of all hooks which I find very error-prone. I have already had to fix bugs because of missing some hook in there.

holiman · 2024-12-10T11:52:50Z

core/tracing/hooks.go

@@ -172,6 +173,9 @@ type (

 	// LogHook is called when a log is emitted.
 	LogHook = func(log *types.Log)
+
+	// BlockHashReadHook is called when EVM reads the blockhash of a block.
+	BlockHashReadHook = func(blockNumber uint64, hash common.Hash)


Why is this needed?

The use-case is to have access to the headers of hashes that are accessed by the EVM. Alternative would be if we added a GetHeaderByHash method somewhere. But getting the hash from OnOpcode is also tricky since the hash will be put on the stack after OnOpcode is invoked.

holiman · 2024-12-10T11:57:46Z

core/tracing/journal.go

+type journal struct {
+	entries     []entry
+	hooks       *Hooks
+	lastCreator *common.Address // Account that initiated the last contract creation
+
+	validRevisions []revision
+	nextRevisionId int
+	revIds         []int
+}


the linearJournal in my PR #30660 is IMO a better base to start from. It does away with validrevisions and revIds, instead it just as a list of indexes, revisions, which point to an entry.

type linearJournal struct { entries []journalEntry // Current changes tracked by the linearJournal dirties map[common.Address]int // Dirty accounts and the number of changes revisions []int // sequence of indexes to points in time designating snapshots }

I have looked at #30660 and agree it is a better way to do journaling for tracers. The key point for me there is that there will be only 1 revert hook emitted as opposed to one for each change to a state element.

Given that #30660 seems to be still in flux I like to wait on it to be merged and implement it for tracers in a future PR as an improvement.

These points were discussed at standup:

It looks like we will need to change the model a bit with the set-based journal as it operates on accounts, requiring also a new hook like OnAccountReverted and the inconsistencies there around emitting state changes on the field level and the reverts being on the account level.

The point was raised by @holiman that we are exposing a behaviour to tracers that will be hard to revert. The behaviour in question is the reverse of every state change (i.e. if Balance: A->B->C, we emit Balance: C->B->A on revert instead of just Balance: C->A).

On the second point I'd like to add my perspective: This journal is simply a wrapper around the tracers. We are not changing tracing interface semantics at all. Users can copy this file and run it themselves right now. And we are exposing every state modification, and I believe the reverse of it is the same.

It looks like we will need to change the model a bit with the set-based journal

I don't get it. The two PRs, the two journals are unrelated. You have opted to copy-paste the legacy linear journal-implementation. I think the new linear journal-implementation is better/simpler.

You could also have chosen to copy-paste the set-based journal-implementation. I don't care which you choose, really, but I don't see any point in picking one now and switching later. If you want another, pick that one from the get-go ?

Users can copy this file and run it themselves right now.

They can't perform WrapWithJournal from "user-space," can they? Isn't that what makes this a big "blessed" ?

They can't perform WrapWithJournal from "user-space," can they? Isn't that what makes this a big "blessed" ?

Right exact copy wouldn't work. They'd have to implement WrapWithJournal locally, but it's totally possible.

I don't care which you choose, really, but I don't see any point in picking one now and switching later.

I think the current journal is good, it is consistent with the existing interface, and it has been running in geth for quite a while.

holiman · 2024-12-19T18:30:11Z

core/tracing/journal.go

+	validRevisions []revision
+	nextRevisionId int
+	revIds         []int


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?

holiman · 2024-12-19T18:34:22Z

core/tracing/journal.go

+
+func (j *journal) OnNonceChange(addr common.Address, prev, new uint64) {
+	// When a contract is created, the nonce of the creator is incremented.
+	// This change is not reverted when the creation fails.


Hm, what? Doesn't that depend on ... things.. ? Or is it always the case?

For evm-creates (as opposed to create-tx where the flow is different), the new scope begins here: https://github.com/ethereum/go-ethereum/blob/master/core/vm/evm.go#L425 , and the nonce-bump happens a few lines later: https://github.com/ethereum/go-ethereum/blob/master/core/vm/evm.go#L442

However, the actual snapshot is taken even further below: https://github.com/ethereum/go-ethereum/blob/master/core/vm/evm.go#L479 .

So that's the mismatch you see. What the tracing percieves is

SCOPE_START{ NONCE ++ OTHER_STUFF }

But the reality is

NONCE++ SCOPE_START{ OTHER_STUFF }

So when the scope reverts, you see this weird inconsistency that somehow nonce is a special snowflake.

The proper fix would be to move the captureBegin-invocation down, so it happens close to where we take the snapshot. The nonce-bump belongs to the parent scope.

holiman · 2024-12-19T18:34:34Z

core/tracing/journal.go

+type journal struct {
+	entries     []entry
+	hooks       *Hooks
+	lastCreator *common.Address // Account that initiated the last contract creation


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.

Or if we're inside a creation, and inside the constructor we call ripemd to calculate a signature: we lost lastCreator.

s1na added 7 commits August 26, 2024 15:45

core/tracing: add vm context to system call hook

8659e68

core/tracing: add GetCodeHash to statedb interface

b4e0174

core/tracing: emit state change events for journal reverts

f670a7f

core/tracing: add hook for reverted out blocks

cf873c3

log selfdestructs balance revert

365b715

Add state read hooks

aac4024

add tracing journal

dbe5f83

s1na requested review from karalabe, holiman and rjl493456442 as code owners September 16, 2024 08:46

s1na commented Sep 16, 2024

View reviewed changes

s1na added 8 commits September 16, 2024 13:26

update changelog

b87c4fe

fix indent

702a42f

add block hash read hook

c915bed

resolve merge conflict

838fc25

fix code and nonce param order

1cc58cf

update test

3c58155

pass-through non-journaled hooks

501f302

missed two hooks

1a64297

maoueh reviewed Oct 5, 2024

View reviewed changes

core/tracing/CHANGELOG.md Show resolved Hide resolved

s1na added 2 commits October 8, 2024 20:09

fix journal cur rev Id

1862333

add note on balanceChangeRevert reason

6650000

s1na added the status:triage label Oct 9, 2024

refactor WrapWithJournal to use reflection

d9de74e

karalabe reviewed Oct 10, 2024

View reviewed changes

core/tracing/journal_test.go Show resolved Hide resolved

karalabe reviewed Oct 10, 2024

View reviewed changes

holiman reviewed Oct 10, 2024

View reviewed changes

core/tracing/hooks.go Show resolved Hide resolved

s1na commented Oct 10, 2024

View reviewed changes

core/tracing/hooks.go Show resolved Hide resolved

s1na commented Oct 10, 2024

View reviewed changes

update changelog

4d2fb0e

s1na added 2 commits October 25, 2024 06:26

Merge branch 'master' into tracing/v1.1

b37f2ac

Add test for all underlying hooks being called

a0f7cd6

s1na mentioned this pull request Oct 29, 2024

core/state: invoke OnCodeChange-hook on selfdestruct #30686

Merged

Merge branch 'master' into tracing/v1.1

87582a4

s1na mentioned this pull request Nov 25, 2024

core/state: add code to state reader #30808

Closed

resolve merge conflict

6e4d14c

handle creation nonce in journal

553f023

fjl added this to the 1.14.13 milestone Nov 28, 2024

s1na added 11 commits December 2, 2024 10:45

Merge branch 'master' into tracing/v1.1

be93d72

Merge branch 'master' into tracing/v1.1

1dda30d

rm OnCodeSizeRead

4acea3b

rm onreorg type

018df6b

wrapper func for OnSystemCallStart

60b2222

update changelog

6c56ea5

Merge branch 'master' into tracing/v1.1

f4cf2a5

run go generate

7fb2688

rm read hooks

3228063

lint issue

95b82cf

fix changelog

de48d55

s1na changed the title ~~core/tracing: v1.1~~ core/tracing: state journal wrapper Dec 10, 2024

holiman reviewed Dec 10, 2024

View reviewed changes

s1na added 2 commits December 10, 2024 18:12

un-expose hooks copy

9cae376

Merge branch 'master' into tracing/v1.1

bf51dde

holiman reviewed Dec 19, 2024

View reviewed changes

s1na added the status:marinating PR hasn't been open long enough to get merged label Dec 19, 2024

fjl removed the status:marinating PR hasn't been open long enough to get merged label Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/tracing: state journal wrapper #30441

core/tracing: state journal wrapper #30441

s1na commented Sep 16, 2024

s1na Sep 16, 2024

s1na commented Oct 8, 2024

karalabe Oct 10, 2024

s1na Oct 14, 2024

karalabe Oct 14, 2024

holiman Oct 14, 2024

s1na Oct 10, 2024

s1na commented Oct 24, 2024

s1na commented Nov 26, 2024

holiman Dec 10, 2024

holiman Dec 10, 2024

s1na Dec 10, 2024 •

edited

Loading

holiman Dec 10, 2024

s1na Dec 10, 2024

holiman Dec 10, 2024

s1na Dec 16, 2024

s1na Dec 19, 2024

holiman Dec 19, 2024

holiman Dec 19, 2024

s1na Dec 19, 2024

holiman Dec 19, 2024

holiman Dec 19, 2024

holiman Dec 20, 2024

holiman Dec 19, 2024

holiman Dec 19, 2024


		### 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.

core/tracing: state journal wrapper #30441

Are you sure you want to change the base?

core/tracing: state journal wrapper #30441

Conversation

s1na commented Sep 16, 2024

Choose a reason for hiding this comment

s1na commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1na commented Oct 24, 2024

s1na commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1na Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1na Dec 10, 2024 •

edited

Loading