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

feat: tracing hooks added to stateDB #10915

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 42 additions & 21 deletions core/state/intra_block_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ type IntraBlockState struct {
validRevisions []revision
nextRevisionID int
trace bool
logger *tracing.Hooks
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
logger *tracing.Hooks
tracingHooks *tracing.Hooks

balanceInc map[libcommon.Address]*BalanceIncrease // Map of balance increases (without first reading the account)
}

Expand All @@ -110,6 +111,10 @@ func New(stateReader StateReader) *IntraBlockState {
}
}

func (sdb *IntraBlockState) SetLogger(l *tracing.Hooks) {
sdb.logger = l
}

func (sdb *IntraBlockState) SetTrace(trace bool) {
sdb.trace = trace
}
Expand Down Expand Up @@ -164,6 +169,9 @@ func (sdb *IntraBlockState) AddLog(log2 *types.Log) {
log2.BlockHash = sdb.bhash
log2.TxIndex = uint(sdb.txIndex)
log2.Index = sdb.logSize
if sdb.logger != nil && sdb.logger.OnLog != nil {
sdb.logger.OnLog(log2)
}
sdb.logs[sdb.thash] = append(sdb.logs[sdb.thash], log2)
sdb.logSize++
}
Expand Down Expand Up @@ -321,24 +329,27 @@ 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 sdb.logger == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@dhyaniarun1993 dhyaniarun1993 Jul 3, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@dhyaniarun1993 dhyaniarun1993 Jul 18, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @AskAlexSharov
Can you help me with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// 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)
Expand Down Expand Up @@ -426,11 +437,18 @@ func (sdb *IntraBlockState) Selfdestruct(addr libcommon.Address) bool {
if stateObject == nil || stateObject.deleted {
return false
}

prevBalance := *stateObject.Balance()
sdb.journal.append(selfdestructChange{
account: &addr,
prev: stateObject.selfdestructed,
prevbalance: *stateObject.Balance(),
prevbalance: prevBalance,
})

if sdb.logger != nil && sdb.logger.OnBalanceChange != nil && !prevBalance.IsZero() {
sdb.logger.OnBalanceChange(addr, &prevBalance, uint256.NewInt(0), tracing.BalanceDecreaseSelfdestruct)
}

stateObject.markSelfdestructed()
stateObject.createdContract = false
stateObject.data.Balance.Clear()
Expand Down Expand Up @@ -621,9 +639,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 {
Copy link
Member

Choose a reason for hiding this comment

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

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 {

emptyRemoval := EIP161Enabled && stateObject.empty() && (!isAura || addr != SystemAddress)
if stateObject.selfdestructed || (isDirty && emptyRemoval) {
if logger != nil && logger.OnBalanceChange != nil && !stateObject.Balance().IsZero() && stateObject.selfdestructed {
logger.OnBalanceChange(stateObject.address, stateObject.Balance(), uint256.NewInt(0), tracing.BalanceDecreaseSelfdestructBurn)
}
if err := stateWriter.DeleteAccount(addr, &stateObject.original); err != nil {
return err
}
Expand Down Expand Up @@ -695,7 +716,7 @@ func (sdb *IntraBlockState) FinalizeTx(chainRules *chain.Rules, stateWriter Stat
}

//fmt.Printf("FinalizeTx: %x, balance=%d %T\n", addr, so.data.Balance.Uint64(), stateWriter)
if err := updateAccount(chainRules.IsSpuriousDragon, chainRules.IsAura, stateWriter, addr, so, true); err != nil {
if err := updateAccount(chainRules.IsSpuriousDragon, chainRules.IsAura, stateWriter, addr, so, true, sdb.logger); err != nil {
return err
}
so.newlyCreated = false
Expand Down Expand Up @@ -751,7 +772,7 @@ func (sdb *IntraBlockState) MakeWriteSet(chainRules *chain.Rules, stateWriter St
}
for addr, stateObject := range sdb.stateObjects {
_, isDirty := sdb.stateObjectsDirty[addr]
if err := updateAccount(chainRules.IsSpuriousDragon, chainRules.IsAura, stateWriter, addr, stateObject, isDirty); err != nil {
if err := updateAccount(chainRules.IsSpuriousDragon, chainRules.IsAura, stateWriter, addr, stateObject, isDirty, sdb.logger); err != nil {
return err
}
}
Expand Down
12 changes: 12 additions & 0 deletions core/state/state_object.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,9 @@ func (so *stateObject) SetState(key *libcommon.Hash, value uint256.Int) {
key: *key,
prevalue: prev,
})
if so.db.logger != nil && so.db.logger.OnStorageChange != nil {
so.db.logger.OnStorageChange(so.address, key, prev, value)
}
so.setState(key, value)
}

Expand Down Expand Up @@ -299,6 +302,9 @@ func (so *stateObject) SetBalance(amount *uint256.Int, reason tracing.BalanceCha
account: &so.address,
prev: so.data.Balance,
})
if so.db.logger != nil && so.db.logger.OnBalanceChange != nil {
so.db.logger.OnBalanceChange(so.address, so.Balance(), amount, reason)
}
so.setBalance(amount)
}

Expand Down Expand Up @@ -346,6 +352,9 @@ func (so *stateObject) SetCode(codeHash libcommon.Hash, code []byte) {
prevhash: so.data.CodeHash,
prevcode: prevcode,
})
if so.db.logger != nil && so.db.logger.OnCodeChange != nil {
so.db.logger.OnCodeChange(so.address, so.data.CodeHash, prevcode, codeHash, code)
}
so.setCode(codeHash, code)
}

Expand All @@ -360,6 +369,9 @@ func (so *stateObject) SetNonce(nonce uint64) {
account: &so.address,
prev: so.data.Nonce,
})
if so.db.logger != nil && so.db.logger.OnNonceChange != nil {
so.db.logger.OnNonceChange(so.address, so.data.Nonce, nonce)
}
so.setNonce(nonce)
}

Expand Down
2 changes: 2 additions & 0 deletions core/vm/evmtypes/evmtypes.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,4 +155,6 @@ type IntraBlockState interface {
Snapshot() int

AddLog(*types.Log)

SetLogger(hooks *tracing.Hooks)
Copy link
Member

Choose a reason for hiding this comment

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

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

}
Loading