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.

Sign up for GitHub

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

Jump to bottom

Adding Tracing Options #1644

Merged
merged 2 commits into from
Mar 18, 2022
Merged

Adding Tracing Options #1644

merged 2 commits into from
Mar 18, 2022

Conversation

nagarev
Copy link
Contributor

@nagarev nagarev commented Oct 29, 2021

Adding tracing options to debug_traceTransaction method

Description

As described in linked issue, there are some missing options for debug_traceTransaction method. This PR adds the ones regarding storage, memory and stack.

Motivation and Context

It helps with ecosystem compatibility.

How Has This Been Tested?

By doing unit and manual tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • [] Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)
  • Other information:

Fixes #1553

@nagarev nagarev force-pushed the debug-trace-options branch 2 times, most recently from 0552134 to 83495b6 Compare December 20, 2021 13:24
@nagarev nagarev marked this pull request as ready for review December 20, 2021 13:24
Vovchyk
Vovchyk requested changes Jan 5, 2022
View reviewed changes
rskj-core/src/main/java/org/ethereum/vm/trace/ProgramTraceProcessor.java Outdated
@@ -39,6 +39,17 @@

private final Map<Keccak256, ProgramTrace> traces = new HashMap<>();

private static TraceOptions traceOptions = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

does it need to be static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I've refactored this class, also.

rskj-core/src/main/java/org/ethereum/vm/trace/ProgramTraceProcessor.java Outdated
filterProvider.addFilter("opFilter",
SimpleBeanPropertyFilter.serializeAllExcept(traceOptions.getDisabledFields()));

return OBJECT_MAPPER.setFilterProvider(filterProvider).valueToTree(txTraces);
Copy link
Contributor

Choose a reason for hiding this comment

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

multiple threads may collide here. OBJECT_MAPPER is static one, so setting a filter here may not be a thread-safe operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed that for THIS method only. 💪🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(So the rest of them can perform as always)

@nagarev nagarev requested a review from Vovchyk January 13, 2022 21:28
illuque-iov
illuque-iov reviewed Jan 14, 2022
View reviewed changes
rskj-core/src/main/java/co/rsk/rpc/modules/debug/TraceOptions.java Outdated
disabledFields.add("storage");
}
traceOptions.remove("disableStorage");
}
Copy link

@illuque-iov illuque-iov Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would rather extract the branch if (traceOptions.containsKey("disableStorage")) { to its own method, ie. setDisableStorage, to ease readability: indentation is too high here IMO.

Same for the other branches alike this one.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Let's do that!

illuque-iov
illuque-iov reviewed Jan 14, 2022
View reviewed changes
rskj-core/src/main/java/co/rsk/rpc/modules/debug/TraceOptions.java Outdated
if (Boolean.parseBoolean(traceOptions.get("disableMemory"))) {
disabledFields.add("memory");
}
traceOptions.remove("disableMemory");
Copy link

Choose a reason for hiding this comment

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

Also, as we discussed before, it would be better to not modify the received traceOptions.

@nagarev nagarev force-pushed the debug-trace-options branch 2 times, most recently from 663e3c9 to 822a41d Compare January 21, 2022 14:18
Vovchyk
Vovchyk previously approved these changes Jan 21, 2022
View reviewed changes
@nagarev
Copy link
Contributor Author

nagarev commented Jan 21, 2022

pipeline:run

@illuque-iov illuque-iov self-requested a review January 21, 2022 17:00
illuque-iov
illuque-iov previously approved these changes Jan 21, 2022
View reviewed changes
VaWheel
VaWheel reviewed Jan 26, 2022
View reviewed changes
rskj-core/src/main/java/org/ethereum/vm/trace/ProgramTraceProcessor.java Outdated
}

public ProgramTraceProcessor(TraceOptions options) {
this();
Copy link

Choose a reason for hiding this comment

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

do we need this() call since we will overwrite traceOptions in the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're completely right! It remained like that after a refactoring was made. Solved!

@nagarev nagarev dismissed stale reviews from illuque-iov and Vovchyk via 61bd55c January 26, 2022 17:30
@nagarev
Copy link
Contributor Author

nagarev commented Jan 27, 2022

pipeline:run

@sonarcloud
Copy link

sonarcloud bot commented Feb 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

90.3% 90.3% Coverage
0.0% 0.0% Duplication

@Vovchyk
Copy link
Contributor

Vovchyk commented Feb 9, 2022

pipeline:run

3 similar comments
@Vovchyk
Copy link
Contributor

Vovchyk commented Feb 9, 2022

pipeline:run

@Vovchyk
Copy link
Contributor

Vovchyk commented Feb 9, 2022

pipeline:run

@Vovchyk
Copy link
Contributor

Vovchyk commented Feb 9, 2022

pipeline:run

@Vovchyk Vovchyk changed the base branch from master to IRIS-3.3.0-rc February 10, 2022 08:41
@sonarcloud
Copy link

sonarcloud bot commented Mar 18, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

88.2% 88.2% Coverage
0.0% 0.0% Duplication

@Vovchyk Vovchyk merged commit 315755a into IRIS-3.3.0-rc Mar 18, 2022
@Vovchyk Vovchyk deleted the debug-trace-options branch March 18, 2022 17:04
@aeidelman aeidelman added this to the Iris v3.3.0 milestone Mar 23, 2022
@nagarev nagarev mentioned this pull request May 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VaWheel VaWheel VaWheel left review comments

@Vovchyk Vovchyk Vovchyk left review comments

@illuque-iov illuque-iov illuque-iov left review comments

@heitorfm heitorfm Awaiting requested review from heitorfm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Iris 3.3.0
Development

Successfully merging this pull request may close these issues.

Support geth-like options in debug_traceTransaction
5 participants
@nagarev @Vovchyk @VaWheel @illuque-iov @aeidelman