Push tracing of Parlia system transactions so that live tracers can properly traces those state changes

[maoueh]

Description

See #2768 for context and details.

This introduced OnSystemTxStart/OnSystemTxEnd and renamed OnSystemTxEnd to OnSystemTxFixIntrinsicGas (so OnSystemTxEnd can be re-used for other purposes).

The Parlia engine is now tracing it's system transaction properly which was not the case before.

Rationale

This PR ensure that system transactions are properly tested when using live tracers.

Changes

Consensus engine is now receiving a tracer *tracing.Hooks that Parlia engine then uses when needing to apply system transactions.

[zlacfzy]

Seem don't need receipt in context, maybe can just use applyTransaction?

[maoueh]

It's for the defer to pass it down to OnTxEnd(receipt) at https://github.com/bnb-chain/bsc/pull/2772/files/a69186357e1be7043932f26dd621cadb22342c13#diff-7c199a2e5208a5ff8460e89bc19a9f9734312a8a4b15052d9d3e7e65c74aaa06R2022.

I initially modified applyTransaction but felt it was bad to change all call site to right away ignore so I created a second version.

But I could implemented differently maybe that would be less confusing:

func applyTransaction(...) error {
  // Used by OnTxEnd via a defer to properly pass the final receipt.
  var tracingReceipt *types.Receipt
...

I usually prefer to use Go native return capabilities which works even if receipt is not set explicitly but only return.

Let me know the team preference here, we can also keep the current code and I can document the function to explain receipt usage.

[zzzckck]

@maoueh I would prefer the second one: create the receipt variable at the beginning

func applyTransaction(...) error {
  // Used by OnTxEnd via a defer to properly pass the final receipt.
  var tracingReceipt *types.Receipt
...

[maoueh]

@zlacfzy @zzzckck I would like to have BSC's team input on those balance change reasons.

For SetBalance, I'm not sure BalanceIncreaseRewardTransactionFee would be the best choice. There is BalanceChangeTransfer that would be possible, however I find this would breaks the symmetry for transfers where usually there is 2 in a row, one for the recipient and the other.

Maybe we should introduce BalanceChangeBSCDistributeReward or two, one for each leg:

• BalanceDecreaseBSCDistributeReward
• BalanceIncreaseBSCDistributeReward

[zzzckck]

@maoueh I think BalanceIncreaseRewardTransactionFee is not accurate. I would prefer to introduce the two new reason for BSC, but I am afraid of forward compatible issue, how about:

BalanceDecreaseBSCDistributeReward BalanceChangeReason = 1000
BalanceIncreaseBSCDistributeReward BalanceChangeReason = 1001

Another solution would be just leave it to unspecified: BalanceChangeUnspecified

[maoueh]

I prefer indeed to add 2 new reasons, makes tracing system aware of important balance changes. Don't think Unspecified is good here.

but I am afraid of forward compatible issue

Indeed, that's always a possibility, even more that the BalanceChangeReason is a byte today leading to just 255 values.

But having managed multiple Geth forks over the last 3 years, I can tell you that new balance change reasons is very rare. I think it will be just fine. I've put BSC starting at 210 for now, let me know how it feels.

[zzzckck]

@maoueh basically, I am ok with this PR, we can merge it and create a new PR to address my 2 comments:

1.create the receipt variable at the beginning.
2.BalanceIncreaseRewardTransactionFee to BalanceChangeUnspecified

[Joj501]

Approve change

[maoueh]

@zzzckck @Joj501 I adjusted the PR

[galaio]

The OnTxStart may need to be called first, then it can use the right env.

[maoueh]

Sorry not sure I fully understand the comment.

[galaio]

I mean the onTxStart need be called before OnSystemTxStart.

if tracer := vmConfig.Tracer; tracer != nil {
		if tracer.OnTxStart != nil {
			tracer.OnTxStart(vmenv.GetVMContext(), expectedTx, msg.From())
		}
		if vmConfig.Tracer.OnSystemTxStart != nil {
			vmConfig.Tracer.OnSystemTxStart()
		}
...
}

Because the EVM env only transfers in OnTxStart, so it is better to call first. Do I understand correctly?

func (l *jsonLogger) OnTxStart(env *tracing.VMContext, tx *types.Transaction, from common.Address) {
	l.env = env
}
func (l *jsonLogger) OnSystemTxStart() {
	l.encoder.Encode(l.env.Coinbase, "trigger system tx")
}

[maoueh]

I see the OnSystemTxStart acting as a marker. Usually you track the overall state of execution, so I would see an implementation like this:

type Tracer struct {
    (... implement Hooks)
    
    inSystemTrx bool
}

func (t *Tracer) OnSystemTrxStart() { t.inSystemTrx = true }
func (t *Tracer) OnTrxStart() { <track trx normally> }
func (t *Tracer) OnTrxEnd() { if (t.inSystemTrx) { // In sytsem } else { // In normal }  }
func (t *Tracer) OnSystemTrxEnd() { t.inSystemTrx = false }

In that setup, the system trx start must be called first.

[galaio]

Ok, but the defer order is wrong, may

if vmConfig.Tracer.OnSystemTxEnd != nil {
	defer func() {
		vmConfig.Tracer.OnSystemTxEnd()
	}()
}
if tracer.OnTxEnd != nil {
	defer func() {
		tracer.OnTxEnd(tracingReceipt, applyErr)
	}()
}

It will run as you expect:

func (t *Tracer) OnSystemTrxStart() { t.inSystemTrx = true }
func (t *Tracer) OnTrStart() { <track trx normally> }
func (t *Tracer) OnTxEnd() { if (t.inSystemTrx) { // In sytsem } else { // In normal }  }
func (t *Tracer) OnSystemTrxEnd() { t.inSystemTrx = false }

[maoueh]

You are so right, good catch, thanks!

Is there Go tests in the project that exercices the parlia consensus engine and test system transactions that I could tweak to ensure the system tracing is fully covered?

[galaio]

you can refer newTestParliaHandlerAfterCancun in eth/handler_test.go:256, there are some integrated testings with parlia. It may help you.

[zzzckck]

this one change the code logic, the vmConfig was empty before, but now it will use the vmConfig passed from uplayer.

vmenv := vm.NewEVM(context, vm.TxContext{Origin: msg.From(), GasPrice: big.NewInt(0)}, state, chainConfig, vm.Config{})

Should be careful.

[maoueh]

That's true.

I can pass the tracer around instead of a pre-built vm.Config instance and I could change back to vm.Config{Tracer: tracer}. Would you prefer that?

[maoueh]

@zzzckck

[zzzckck]

will hold this PR for the moment, as there would be 2nd round of code sync, which could have conflict.

[maoueh]

Ok I rebased on latest develop and added unit tests, I would have liked to actually test at a higher level, probably using Blockchain directly of StateProcessor.

But at least the current tests shows overall correct behavior.