feat: tracing hooks added to stateDB #10915

dhyaniarun1993 · 2024-06-26T10:09:13Z

This PR is created to from the tracing hook implementation PR to simplify it's review and merge process.
In this PR, we have plugged tracing hook framework to the statedb.

AskAlexSharov · 2024-06-26T12:18:00Z

core/state/intra_block_state.go

-			bi = &BalanceIncrease{}
-			sdb.balanceInc[addr] = bi
+
+	if sdb.logger == nil {


i think having or not-having logger must not change writing to sdb.journal. sdb.journal it's our way to implement revert of txn execution.

Got it. I will trigger the tracer event at this level then.

I looked into this and as per tracer implementation, we are required to report the OnBalanceChange that takes prev and new balance. In order to report this correctly and timely, we have to get the stateObject and log it. Kindly provide your guidance.
cc: @maoueh

That's an interesting problem.

I've read the implementation of the various usage of balanceInc and it seems an optimization by Erigon to avoid/delay reading state object from DB on balance increase. As long as the state object is not read, the balance increase is deferred until the very end of the transaction (or block depending on execution context).

The tracing however wants to record balance change at the moment they are actually performed so that proper balance tracking can be done within a transaction and most importantly, the the overall "ordering" of what is happening within the transaction is kept relatively against other tracked state changes (nonce, code, etc).

Now, this is side-stepped by this optimization, a different proposition could be:

diff --git a/core/state/intra_block_state.go b/core/state/intra_block_state.go index ff0574b2c5..9339b0d688 100644 --- a/core/state/intra_block_state.go +++ b/core/state/intra_block_state.go @@ -326,30 +326,36 @@ func (sdb *IntraBlockState) AddBalance(addr libcommon.Address, amount *uint256.I if !needAccount && addr == ripemd && amount.IsZero() { needAccount = true } if !needAccount { sdb.journal.append(balanceIncrease{ account: &addr, increase: *amount, }) bi, ok := sdb.balanceInc[addr] if !ok { bi = &BalanceIncrease{} sdb.balanceInc[addr] = bi } bi.increase.Add(&bi.increase, amount) bi.count++ + + if sdb.logger != nil && sdb.logger.OnBalanceChange != nil { + balance := sdb.GetOrNewStateObject(addr).Balance() + sdb.logger.OnBalanceChange(addr, balance, new(uint256.Int).Add(balance, amount), reason) + } + return } stateObject := sdb.GetOrNewStateObject(addr) stateObject.AddBalance(amount, reason) }

Ultimately, that has the same net effect as the current diff but written differently. Indeed, now that we call GetOrNewStateObject to get the current balance, we are going to populate stateObjects[addr] entry as well as transferring the balance that exist now. And since now the addr is populated in stateObjects, the next balance increase for this same account will not go in the if !needAccount statement anymore.

This diff above the same net effect as the current PR both skips the optimization altogether by the fact that we need to read the account and get the actual balance.

However, I think the proposed diff above is less obvious about the consequences of calling GetOrNewStateObject which modifies the actual state.

I personally feel that the first version with an appropriate comment would best reflect the intent of the change:

diff --git a/core/state/intra_block_state.go b/core/state/intra_block_state.go index ff0574b2c5..2802bc6642 100644 --- a/core/state/intra_block_state.go +++ b/core/state/intra_block_state.go @@ -321,24 +321,32 @@ func (sdb *IntraBlockState) AddBalance(addr libcommon.Address, amount *uint256.I if sdb.trace { fmt.Printf("AddBalance %x, %d\n", addr, amount) } - // If this account has not been read, add to the balance increment map - _, needAccount := sdb.stateObjects[addr] - if !needAccount && addr == ripemd && amount.IsZero() { - needAccount = true - } - if !needAccount { - sdb.journal.append(balanceIncrease{ - account: &addr, - increase: *amount, - }) - bi, ok := sdb.balanceInc[addr] - if !ok { - bi = &BalanceIncrease{} - sdb.balanceInc[addr] = bi + + // If tracing of balance changes is enabled, we cannot apply optimization + // of not reading the account before increasing the balance. Indeed, the tracing + // requires knowledge of the balance prior the change and most importantly + // must record the change at the time it's applied. + tracingBalanceChange := sdb.logger != nil && sdb.logger.OnBalanceChange != nil + if !tracingBalanceChange { + // If this account has not been read, add to the balance increment map + _, needAccount := sdb.stateObjects[addr] + if !needAccount && addr == ripemd && amount.IsZero() { + needAccount = true + } + if !needAccount { + sdb.journal.append(balanceIncrease{ + account: &addr, + increase: *amount, + }) + bi, ok := sdb.balanceInc[addr] + if !ok { + bi = &BalanceIncrease{} + sdb.balanceInc[addr] = bi + } + bi.increase.Add(&bi.increase, amount) + bi.count++ + return } - bi.increase.Add(&bi.increase, amount) - bi.count++ - return } stateObject := sdb.GetOrNewStateObject(addr)

@AskAlexSharov Let us know your opinion here.

balanceInc - it’s not optimisation of execution/state_reading. It’s reduction of “conflict rate in parallel-execution”.

let me explain: if run txs in-parallel - if 2 txs update same key X - it’s conflict. But if 2 txs IncreaseBalance of same account (without reading it) - it’s actually conflict-free operation.

So: balanceInc it’s “conflict-free updates” - this is reason why it’s stored separately from other updates.

we don’t have working prototype of parallel execution now, but we had 1 year ago and we plan to have it in future. (FYI: conflict-resolution is done by ReadsValid func).

I can see that we have readList only for StateReaderV3. What about other readers?

from my perspective it's not just optimization - Hook's API forcing app to read different keys (which was not read before)

Agreed, but only special node will pay this price, and there is no way around, someone will need to read at once to get the actual value before the increase.

But I'm all with you that if possible, we should avoid that, like @dhyaniarun1993 mentioned, we are going to explore the path you suggested, I'm confident we can make it work properly.

I will work on the proposition and see where it leads us.

Ok let me know, I think that a double transfer to the same address in the same transaction will yield 2 balanceInc being generated. Let me know your findings, I can take a quick look at adding it in battlefield too, shouldn't be too hard and I could add a quick println call to ensure we correctly exercise all code paths.

The best would be to have an actual unit test that proves the correct tracing instrumentation. Have we ported over the "integration test" we had in Firehose Geth? (https://github.com/streamingfast/go-ethereum/blob/firehose-fh3.0/eth/tracers/internal/tracetest/firehose/firehose_test.go#L72)

I think that a double transfer to the same address in the same transaction will yield 2 balanceInc being generated

Do you mean in the tracer(OnBalanceChange) or the journal(balanceIncrease)?

Have we ported over the "integration test" we had in Firehose Geth?

Yes. but that PR is not upto date with the current changes.

@maoueh Can you add this to the battlefield? I have test it out.

Hey @AskAlexSharov
Can you help me with this?

@dhyaniarun1993

Do you mean in the tracer(OnBalanceChange) or the journal(balanceIncrease)?

Both actually, but yeah I was more referring to journal to ensure we do have a test case that covers that code path.

shohamc1 · 2024-11-21T06:17:40Z

Hi @dhyaniarun1993, we are interested in getting this and the larger refactor PR merged - would you mind rebasing on main? It seems like the changes to add ReadAccountDataForDebug are no longer required.

shohamc1

Some minor naming suggestions.

shohamc1 · 2024-11-21T06:18:32Z

core/vm/evmtypes/evmtypes.go

@@ -155,4 +155,6 @@ type IntraBlockState interface {
 	Snapshot() int

 	AddLog(*types.Log)
+
+	SetLogger(hooks *tracing.Hooks)


Suggested change

SetLogger(hooks *tracing.Hooks)

SetHooks(hooks *tracing.Hooks)

shohamc1 · 2024-11-21T06:18:52Z

core/state/intra_block_state.go

@@ -91,6 +91,7 @@ type IntraBlockState struct {
 	validRevisions []revision
 	nextRevisionID int
 	trace          bool
+	logger         *tracing.Hooks


Suggested change

logger *tracing.Hooks

tracingHooks *tracing.Hooks

shohamc1 · 2024-11-21T06:19:20Z

core/state/intra_block_state.go

@@ -621,9 +652,12 @@ func (sdb *IntraBlockState) GetRefund() uint64 {
 	return sdb.refund
 }

-func updateAccount(EIP161Enabled bool, isAura bool, stateWriter StateWriter, addr libcommon.Address, stateObject *stateObject, isDirty bool) error {
+func updateAccount(EIP161Enabled bool, isAura bool, stateWriter StateWriter, addr libcommon.Address, stateObject *stateObject, isDirty bool, logger *tracing.Hooks) error {


Suggested change

func updateAccount(EIP161Enabled bool, isAura bool, stateWriter StateWriter, addr libcommon.Address, stateObject *stateObject, isDirty bool, logger *tracing.Hooks) error {

func updateAccount(EIP161Enabled bool, isAura bool, stateWriter StateWriter, addr libcommon.Address, stateObject *stateObject, isDirty bool, tracingHooks *tracing.Hooks) error {

core/state/intra_block_state_logger_test.go

dhyaniarun1993 · 2024-11-21T07:06:02Z

Hi @dhyaniarun1993, we are interested in getting this and the larger refactor PR merged - would you mind rebasing on main? It seems like the changes to add ReadAccountDataForDebug are no longer required.

Okay. I will pull the changes and check.

dhyaniarun1993 · 2024-11-21T12:26:38Z

Hi @dhyaniarun1993, we are interested in getting this and the larger refactor PR merged - would you mind rebasing on main? It seems like the changes to add ReadAccountDataForDebug are no longer required.

I think ReadAccountDataForDebug is still required. @AskAlexSharov what do you think?

shohamc1 · 2024-11-21T14:39:47Z

I think ReadAccountDataForDebug is still required. @AskAlexSharov what do you think?

The only difference in implementations of the two functions is in ReaderParallelV3, where there is the discardReadList flag to toggle creation of readlists. Parallel execution is not used during tracing so I think it is fine.

AskAlexSharov · 2024-11-22T04:26:12Z

agree with argument about parallel exec (it's low-prio).
But i still think using ReadAccountDataForDebug method is conceptually useful: because it clearly shows that "enabling Hooks" changing behavior of EVM - with Hooks it reading more accounts from DB than without. If Hooks are tool for state diffs production - it's ok, but if Hooks is debugging tool (like tracers) - then changing behavior of EVM is a bug.

Anyway - what is the problem with ReadAccountDataForDebug? It exists - just call it in 1 place.

It's ok for me to test/merge this PR.

@dhyaniarun1993 @shohamc1

dhyaniarun1993 · 2024-11-22T08:48:46Z

agree with argument about parallel exec (it's low-prio). But i still think using ReadAccountDataForDebug method is conceptually useful: because it clearly shows that "enabling Hooks" changing behavior of EVM - with Hooks it reading more accounts from DB than without. If Hooks are tool for state diffs production - it's ok, but if Hooks is debugging tool (like tracers) - then changing behavior of EVM is a bug.

Anyway - what is the problem with ReadAccountDataForDebug? It exists - just call it in 1 place.

It's ok for me to test/merge this PR.

@dhyaniarun1993 @shohamc1

I am also good with the current implementation. I have started working on the bigger tracing refactor PR and will be testing end to end in that.

shohamc1 · 2024-11-22T12:13:50Z

Merging this for now - I still think we don’t need the function. But let’s reevaluate on the other PR as you say.

Submodule `execution-spec-tests` was inadvertently deleted by PR #10915 and then PR #12925 broke the tests, which went unnoticed because of the submodule deletion.

dhyaniarun1993 added 2 commits June 26, 2024 15:35

feat: tracing hooks added to stateDB

a813dc0

IntraBlockState interface updated

f1eb36d

AskAlexSharov requested changes Jun 26, 2024

View reviewed changes

dhyaniarun1993 added 3 commits July 16, 2024 21:49

added ReadAccountDataForDebug for tracing hooks

6004a57

test cases added for logger

5f99ba3

added test for logger

25e6dd1

shohamc1 reviewed Nov 21, 2024

View reviewed changes

dhyaniarun1993 added 2 commits November 21, 2024 14:34

origin/main merged

0504bcd

naming fixes

ba6d1c2

shohamc1 approved these changes Nov 22, 2024

View reviewed changes

shohamc1 enabled auto-merge (squash) November 22, 2024 12:13

shohamc1 disabled auto-merge November 25, 2024 09:49

AskAlexSharov approved these changes Nov 25, 2024

View reviewed changes

shohamc1 merged commit 4b09d8a into erigontech:main Nov 25, 2024
13 of 14 checks passed

yperbasis mentioned this pull request Dec 3, 2024

Restore execution-spec-tests submodule and revert PR #12925 #12976

Merged

maoueh deleted the feat/erigon-live-tracer-statedb-hook branch January 16, 2025 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: tracing hooks added to stateDB #10915

feat: tracing hooks added to stateDB #10915

dhyaniarun1993 commented Jun 26, 2024 •

edited

Loading

AskAlexSharov Jun 26, 2024

dhyaniarun1993 Jun 26, 2024

dhyaniarun1993 Jul 3, 2024 •

edited

Loading

maoueh Jul 8, 2024

AskAlexSharov Jul 9, 2024

dhyaniarun1993 Jul 16, 2024

maoueh Jul 16, 2024

dhyaniarun1993 Jul 18, 2024 •

edited

Loading

dhyaniarun1993 Aug 7, 2024

maoueh Aug 7, 2024

shohamc1 commented Nov 21, 2024

shohamc1 left a comment

shohamc1 Nov 21, 2024

shohamc1 Nov 21, 2024

shohamc1 Nov 21, 2024

dhyaniarun1993 commented Nov 21, 2024

dhyaniarun1993 commented Nov 21, 2024

shohamc1 commented Nov 21, 2024

AskAlexSharov commented Nov 22, 2024

dhyaniarun1993 commented Nov 22, 2024

shohamc1 commented Nov 22, 2024

	SetLogger(hooks *tracing.Hooks)
	SetHooks(hooks *tracing.Hooks)

	func updateAccount(EIP161Enabled bool, isAura bool, stateWriter StateWriter, addr libcommon.Address, stateObject stateObject, isDirty bool, logger tracing.Hooks) error {
	func updateAccount(EIP161Enabled bool, isAura bool, stateWriter StateWriter, addr libcommon.Address, stateObject stateObject, isDirty bool, tracingHooks tracing.Hooks) error {

feat: tracing hooks added to stateDB #10915

feat: tracing hooks added to stateDB #10915

Conversation

dhyaniarun1993 commented Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhyaniarun1993 Jul 3, 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

dhyaniarun1993 Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shohamc1 commented Nov 21, 2024

shohamc1 left a 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

dhyaniarun1993 commented Nov 21, 2024

dhyaniarun1993 commented Nov 21, 2024

shohamc1 commented Nov 21, 2024

AskAlexSharov commented Nov 22, 2024

dhyaniarun1993 commented Nov 22, 2024

shohamc1 commented Nov 22, 2024

dhyaniarun1993 commented Jun 26, 2024 •

edited

Loading

dhyaniarun1993 Jul 3, 2024 •

edited

Loading

dhyaniarun1993 Jul 18, 2024 •

edited

Loading