Set interpreter config to debug as well in evm.SetDebug and update tracing tests #2061
Conversation
|
Coverage from tests in coverage: 49.1% of statements across all listed packagescoverage: 61.5% of statements in consensus/istanbul coverage: 37.5% of statements in consensus/istanbul/announce coverage: 54.4% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 64.3% of statements in consensus/istanbul/core coverage: 45.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.4% of statements in consensus/istanbul/uptime coverage: 51.8% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/random |
|
|
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #2061 +/- ##
==========================================
+ Coverage 54.24% 54.27% +0.03%
==========================================
Files 692 692
Lines 115618 115628 +10
==========================================
+ Hits 62714 62759 +45
+ Misses 49056 49036 -20
+ Partials 3848 3833 -15
... and 26 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
eelanagaraj
force-pushed
the
eelanagaraj/test-debug-config-change
branch
from
94280c1 to
2212085
Compare
eelanagaraj
changed the title
eelanagaraj
changed the title
eelanagaraj
changed the title
eelanagaraj
force-pushed
the
eelanagaraj/test-debug-config-change
branch
from
864671b to
41bc603
Compare
eelanagaraj
requested review from
a team,
palango and
hbandura
and removed request for
a team
There was a problem hiding this comment.
Looks good, thanks for digging so deep into this code!
Some comments:
- What about fixing this by passing the config as a reference?
- Does
TestTraceTransactionWithRegistryDeployedfail without the prior commits fixing the flags? - I'd merge some commits, so that the history gets a bit nicer.
|
@palango 🙏
geth passes the config by value in
yes!
def, planning to squash merge this. I initially kept stuff in separate undo/redo steps to save CI results while testing (obvi overwrote those when rebasing master hehe). Happy to split things up to separate commits if squash-merging isn't an option! 😄 |
Makes a lot of sense, lets keep it like that for now. Last question: How did you generate the updates in the tracer tests? |
Sounds good!
I took most of the changes from the go-native tracer PR -- basically, that PR got these tests runnable (they have been commented out for 4 years 😅 ). The idea was to get those tests runnable with input pre-/post- the fix (no changes -- they pass either way; this was also one of the reasons for the undo/redo in the commit history since I wanted to confirm this in CI in addition to locally). The hope is that with these tests added on master, we will have a bit stronger of a signal about the go-native tracer implementation (i.e. that PR should require no changes to the input files to get those cases running). For the JSON files:
Happy to add clarification anywhere or lmk if you can think of additional steps to take regarding the native tracer changes! 🙏 |
|
@palango oop I also added one last TODO to the PR -- I won't merge this until after the blockscout results comparison for some txs & making sure that it doesn't break things. Then, blockscout outputs should be identical for the go-native tracer once this change is merged! |
Description
Debugging the failure of the test case
TestBlockTracingConcurrentMapAccessand then later in thetracers_testsuite in #1978 led to the two following observations:evm.SetDebugfunction, we currently only setevm.Config.Debugbut notevm.interpreter.cfg.Debug, but both of these are used in EVM execution code. Fixing this is necessary for the go native tracing implementation to worktracers_testsuite has been commented out for the past few years but now mostly work with the original geth testsThe debug setting issue seems to have arisen with an upstream merge where we changed EVM/interpreter initialization from passing the config by reference to passing by value. This caused the divergence in debug config values. For now, I opted by setting both debug config values in the setter function instead of reverting to passing by reference, since upstream still passes the config by value and I imagine this could lead to the exact same issue in the future (and minimizes the diff).
This PR:
tracer_testfixes from the go-native tracing PR and implements them here so we have a clearer signal of what expected values are before the native tracing implementationTested
Related Issues
Prep to get #1978 into mergeable state
Backwards compatibility
This changes the traces outputted, as this now does not trace calls when distributing fees or completing tobin transfer logic. This breaks from current tracing behavior, but seems consistent with previous (pre #1695) functionality and intended functionality (from the comments).