eth/tracers: add onSystemTxEnd hook for system tx gasused.

[zlacfzy]

Description

eth/tracers: add onSystemTxEnd hook for system transaction gas used.

Rationale

system transaction don't include intrinsic gas

Example

add an example CLI or API response...

Changes

Notable changes:

• eth/tracers

[NathanBSC]

add some comments to explain it's usage is better IMO

[maoueh]

@buddh0 @zlacfzy Shouldn't there be here a paired OnSystemTxStart that would go along OnSystemTxEnd?

The OnSystemCallStart/End is used in Ethereum Mainnet to record the process beacon root call. I would expect that a BSC specific tracing.Hooks would correctly wrap BSC internal transaction between OnSystemTxStart/End. For instance, I would have expected https://github.com/bnb-chain/bsc/blob/master/consensus/parlia/parlia.go#L1933 to be wrapped with the right tracing behavior for proper handling of system transaction.

For info, I'm on of the contributor that worked with Geth team to revamp the tracing API. You can see how we have currently instrumented our "old" BSC tracing code: https://github.com/streamingfast/go-ethereum/blob/release/bsc-1.x-fh2.5/consensus/parlia/parlia.go#L1970-L1973.

Right now the OnSystemTxEnd has been scattered on different places, but execution on live tracer would not work as the tracer is not passed down in Parlia engine when actually executing the transaction: https://github.com/binance-chain/bsc/blob/db357bf6e37d61642143c975707b8972fcdfac93/consensus/parlia/parlia.go#L2170

I would like to help shape the BSC tracing API to ensure it does correctly covers BSC cases. I would be happy to schedule a call with you guys and collaborate on this just like we did for Ethereum Mainnet.

[zzzckck]

@buddh0 @zlacfzy Shouldn't there be here a paired OnSystemTxStart that would go along OnSystemTxEnd?

The OnSystemCallStart/End is used in Ethereum Mainnet to record the process beacon root call. I would expect that a BSC specific tracing.Hooks would correctly wrap BSC internal transaction between OnSystemTxStart/End. For instance, I would have expected https://github.com/bnb-chain/bsc/blob/master/consensus/parlia/parlia.go#L1933 to be wrapped with the right tracing behavior for proper handling of system transaction.

For info, I'm on of the contributor that worked with Geth team to revamp the tracing API. You can see how we have currently instrumented our "old" BSC tracing code: https://github.com/streamingfast/go-ethereum/blob/release/bsc-1.x-fh2.5/consensus/parlia/parlia.go#L1970-L1973.

Right now the OnSystemTxEnd has been scattered on different places, but execution on live tracer would not work as the tracer is not passed down in Parlia engine when actually executing the transaction: https://github.com/binance-chain/bsc/blob/db357bf6e37d61642143c975707b8972fcdfac93/consensus/parlia/parlia.go#L2170

I would like to help shape the BSC tracing API to ensure it does correctly covers BSC cases. I would be happy to schedule a call with you guys and collaborate on this just like we did for Ethereum Mainnet.

@maoueh Actually, the function name: CaptureSystemTxEnd is a little bit misleading, it should be named like SystemTxFixup, which does not need to be paired with Start/End. Currently, the tracer api could not access the Parlia engine, so the SystemTx(Parlia) is executed as normal transaction, basically it could be fine, do you wanna to integrate the trace code into Parlia? Maybe you can raise a PR directly, we can discuss in that PR.

[maoueh]

@zzzckck

Actually, the function name: CaptureSystemTxEnd is a little bit misleading, it should be named like SystemTxFixup

Yeah agreed, it's the right time to change it!

I have further questions about it. I'm actually questioning this hook a lot. I've noticed it was present from some time now, so there is always a backward compatibility question, but I'll describe what I see as problematic from this hook. I'm starting to even question if my current tracing solution is affected.

I see it's only called in traceTx and seems to account for intrinsic gas when we are in a system transaction situation. But where is the intrinsic gas billed/charged when the node is syncing live and goes through the Parlia engine? I imagine I've missed it but I've look in the code back between state process and the Finalize in Parlia engine down to applyTransaction/Message in Parlia and I didn't found where it would be "billed" in this context.

I'm asking about the live syncing code path and its importance because Geth 1.14 introduced live tracers, tracers that are not invoked through the JSON-RPC but when the node syncs block which we use. Thus, I want to ensure that everything is properly traced.

Here the other griefs I have on the current OnSystemTxEnd hook and where it's called:

• It's outside the normal OnTxStart/OnTxEnd, so the tracer needs to keep the "transaction" after OnTxEnd in case maybe OnSystemTxEnd is ended. Not a problem for tracer that deals with a single transaction, but creates harder state management for tracer that trace a full block. It would be preferable if that dealing was done within the OnTxEnd/TxStart pair.
• It's at the end of the transaction, but should be at the beginning as intrinsic is usually done first.
• It's does not seems to properly report the OnGasChange (but this was added in 1.14)

Is there opening to change OnSystemTxEnd its name and how it's actually implemented? I could propose a PR here and adjust the current tracer to be retrofit correctly.

SystemTx(Parlia) is executed as normal transaction

Indeed, but it's not traced like other normal BSC transactions that are processed in state_processor leading to the tracers to miss important state changes.

do you wanna to integrate the trace code into Parlia? Maybe you can raise a PR directly, we can discuss in that PR

Yes, I want that the applyTransaction within the Parlia consensus engine to be traced like other transactions, ideally through a OnSystemTxStart/End pair.

I'll open a PR and we can discuss implementation details on it.

[maoueh]

@zzzckck

I'll open a PR and we can discuss implementation details on it.

#2772