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

[Flow EVM] Run tracing hooks OnTxStart, OnTxEnd for DryRunTransaction #6491

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Sep 23, 2024

work towards: onflow/flow-evm-gateway#530

This will allow us to generate traces for calls, to be used for debug_traceCall.

@@ -306,6 +311,13 @@ func (bl *BlockView) DryRunTransaction(
txResult.GasConsumed += txResult.GasRefund
}

// call tracer on tx end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if this should be before or after adjusting the txResult.GasConsumed with the various buffers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's correct the way it is now

// call tracer on tx end
if proc.evm.Config.Tracer != nil &&
proc.evm.Config.Tracer.OnTxEnd != nil &&
txResult != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

txResult != nil this should always be true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I saw that proc.run can return nil for txResult in some cases:

if err != nil {
	return nil, err
}

Besides, this is the exact same condition for both RunTransaction & BatchRunTransaction, so I wanted to be on the safe side.

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 41.47%. Comparing base (a573d77) to head (ee3c489).

Files with missing lines Patch % Lines
fvm/evm/emulator/emulator.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6491      +/-   ##
==========================================
- Coverage   41.48%   41.47%   -0.02%     
==========================================
  Files        2027     2027              
  Lines      144828   144834       +6     
==========================================
- Hits        60084    60071      -13     
- Misses      78538    78553      +15     
- Partials     6206     6210       +4     
Flag Coverage Δ
unittests 41.47% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ramtinms ramtinms left a comment

Choose a reason for hiding this comment

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

the changes is okey to me, but I don't think we need to deploy it mainnet right away, if we are running the evm emulator offchain for this goal, then we could use a branch with these changes until post spork.

@m-Peter
Copy link
Collaborator Author

m-Peter commented Sep 24, 2024

the changes is okey to me, but I don't think we need to deploy it mainnet right away, if we are running the evm emulator offchain for this goal, then we could use a branch with these changes until post spork.

@ramtinms I have found a way to achieve this functionality with the approach in: https://github.com/onflow/flow-evm-gateway/pull/567/files#diff-34c0f187c58bb59d36ca39cc631b07ba8f74d7f6886a5d6c6a796d721a928e67R244-R253.
What I am concerned about is this line:

// call tracer
tracer.OnTxStart(&tracing.VMContext{}, tx, from)

I am not entirely sure if I can populate this exactly as GetVMContext does:

// GetVMContext provides context about the block being executed as well as state
// to the tracers.
func (evm *EVM) GetVMContext() *tracing.VMContext {
	return &tracing.VMContext{
		Coinbase:    evm.Context.Coinbase,
		BlockNumber: evm.Context.BlockNumber,
		Time:        evm.Context.Time,
		Random:      evm.Context.Random,
		GasPrice:    evm.TxContext.GasPrice,
		ChainConfig: evm.ChainConfig(),
		StateDB:     evm.StateDB,
	}
}

Other than that, the approach on the EVM GW seems to work.

@ramtinms
Copy link
Member

@m-Peter I think we should merge, my comment was for deployment given we never need tracer on the dryrun for onchain operations.

@ramtinms ramtinms added this pull request to the merge queue Sep 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 24, 2024
@j1010001 j1010001 added this pull request to the merge queue Sep 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 25, 2024
Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Maybe we need to consider using a no-op tracer instead of nil, to get rid of all these checks and make it a bit cleaner. (Just a general comment, not really for this PR)

@j1010001 j1010001 added this pull request to the merge queue Sep 25, 2024
Merged via the queue into onflow:master with commit 66c06c5 Sep 25, 2024
55 checks passed
@m-Peter m-Peter deleted the dry-run-transaction-with-tracing branch October 9, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants