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

Add ITraceDebugPlugin infrastructure #1724

Closed
wants to merge 14 commits into from
Closed

Add ITraceDebugPlugin infrastructure #1724

wants to merge 14 commits into from

Conversation

devhawk
Copy link
Contributor

@devhawk devhawk commented Jun 22, 2020

This PR adds infrastructure to capture trace information during execution in order to enable Time Travel Debugging

@devhawk
Copy link
Contributor Author

devhawk commented Jun 23, 2020

Note, build failure after 9c73550 is unrelated to code change. Github workflow was unable to download dotnet-format

@devhawk devhawk mentioned this pull request Jun 24, 2020
16 tasks
@devhawk
Copy link
Contributor Author

devhawk commented Jun 24, 2020

Note, TestReVerifyTopUnverifiedTransactionsIfNeeded passes locally and is not related to my code change as far as I can tell

{
public interface ITraceDebugPlugin
{
bool ShouldTrace(Header header, Transaction tx);
Copy link
Member

Choose a reason for hiding this comment

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

Is not enough with GetSink ? it's needed ShouldTrace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is that you don't likely want to trace every tx. For example, if I deploy a contract to mainnet, I can also deploy a mainnet node with the debugger plugin to capture every run of my contract for later analysis if need be. But I don't want to capture every tx - just mine. This API allows me to cleanly separate the logic that decides if a given tx should be traced vs. the logic that does the tracing.

Copy link
Member

Choose a reason for hiding this comment

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

But this logic can be done in the plugin, if you don't want to trace this TX, just ignore it, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it could be done other ways. I feel this approach provides more flexibility in case we want to support multiple tracers for a single transaction.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think @erikzhang ?

@erikzhang
Copy link
Member

I think the only method that ITraceDebugPlugin should have is GetApplicationEngine(). It returns an object inherited from ApplicationEngine class. If no ITraceDebugPlugin is found, then the default ApplicationEngine is used.

And the ITraceDebugPlugin should be renamed to IApplicationEngineProvider.

@devhawk
Copy link
Contributor Author

devhawk commented Jul 7, 2020

I think the only method that ITraceDebugPlugin should have is GetApplicationEngine(). It returns an object inherited from ApplicationEngine class. If no ITraceDebugPlugin is found, then the default ApplicationEngine is used.

And the ITraceDebugPlugin should be renamed to IApplicationEngineProvider.

@erikzhang Could you please provide more context around what your goal is with this design? I would like to understand your thinking better.

@erikzhang
Copy link
Member

In this way, we can extend the ApplicationEngine in the plug-in at will. The solution in this PR feels that most of the functions of the plug-in are already in the core, and there is no scalability.

@devhawk
Copy link
Contributor Author

devhawk commented Jul 8, 2020

FYI, I'm leaving this PR open for now, but #1758 is likely going to make this PR obsolete

@devhawk
Copy link
Contributor Author

devhawk commented Jul 11, 2020

Closing in favor of #1758

@devhawk devhawk closed this Jul 11, 2020
@devhawk devhawk deleted the devhawk/ttd3 branch July 23, 2020 20:20
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.

None yet

3 participants