-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from 2 commits
a813dc0
f1eb36d
6004a57
5f99ba3
25e6dd1
0504bcd
ba6d1c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -91,6 +91,7 @@ type IntraBlockState struct { | |||||
validRevisions []revision | ||||||
nextRevisionID int | ||||||
trace bool | ||||||
logger *tracing.Hooks | ||||||
balanceInc map[libcommon.Address]*BalanceIncrease // Map of balance increases (without first reading the account) | ||||||
} | ||||||
|
||||||
|
@@ -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 | ||||||
} | ||||||
|
@@ -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++ | ||||||
} | ||||||
|
@@ -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 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think having or not-having logger must not change writing to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. I will trigger the tracer event at this level then. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see that we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Ok let me know, I think that a double transfer to the same address in the same transaction will yield 2 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you mean in the tracer(
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @AskAlexSharov There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Both actually, but yeah I was more referring to |
||||||
// 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) | ||||||
|
@@ -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() | ||||||
|
@@ -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 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 | ||||||
} | ||||||
|
@@ -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 | ||||||
|
@@ -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 | ||||||
} | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -155,4 +155,6 @@ type IntraBlockState interface { | |||||
Snapshot() int | ||||||
|
||||||
AddLog(*types.Log) | ||||||
|
||||||
SetLogger(hooks *tracing.Hooks) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.