-
Notifications
You must be signed in to change notification settings - Fork 640
refactor(tracing): centralize gas cost guard #10121
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the tracing infrastructure to centralize the gas cost guard mechanism in the GethLikeTxTracer base class, eliminating code duplication and improving maintainability. The refactoring introduces a template method pattern where ReportOperationRemainingGas is sealed and derived classes override OnOperationRemainingGas instead.
Key changes:
- Moved
_gasCostAlreadySetForCurrentOpguard from derived classes to the baseGethLikeTxTracerclass - Introduced sealed
ReportOperationRemainingGaswith protected virtualOnOperationRemainingGascallback pattern - Added
RegisterExternalTracermethod to support plugin-based tracer registration
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Nethermind.Evm.Test/Tracing/GethLikeTxTracerTests.cs |
New test file validating gas cost guard behavior with multiple calls per operation |
Nethermind.Evm.Test/Tracing/GethLikeNativeTracerFactoryTests.cs |
Added tests for external tracer registration including idempotency and overwrite protection |
Nethermind.Blockchain/Tracing/GethStyle/GethLikeTxTracer.cs |
Centralized gas guard in base class with sealed ReportOperationRemainingGas and virtual OnOperationRemainingGas callback |
Nethermind.Blockchain/Tracing/GethStyle/Custom/Native/GethLikeNativeTracerFactory.cs |
Added RegisterExternalTracer method using TryAdd for thread-safe registration |
Nethermind.Blockchain/Tracing/GethStyle/Custom/Native/FourByte/Native4ByteTracer.cs |
Added base call to StartOperation to support centralized guard reset |
Nethermind.Blockchain/Tracing/GethStyle/Custom/Native/Call/NativeCallTracer.cs |
Updated to override OnOperationRemainingGas instead of ReportOperationRemainingGas |
Nethermind.Blockchain/Tracing/GethStyle/Custom/JavaScript/GethLikeJavaScriptTxTracer.cs |
Updated to override OnOperationRemainingGas and added base call to StartOperation |
This is an excellent refactoring that follows best practices for code reuse and the template method pattern. The changes are well-tested, maintain backward compatibility, and improve code maintainability. I found no issues with the implementation.
|
In what case is this path triggered? Can we avoid it at all? |
| public static void RegisterExternalTracer(string tracerName, GethLikeNativeTracerFactoryDelegate tracerDelegate) | ||
| { | ||
| _tracers.TryAdd(tracerName, tracerDelegate); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either should be Add and it throws, or at least return if it fails
| public static void RegisterExternalTracer(string tracerName, GethLikeNativeTracerFactoryDelegate tracerDelegate) | |
| { | |
| _tracers.TryAdd(tracerName, tracerDelegate); | |
| } | |
| public static bool RegisterExternalTracer(string tracerName, GethLikeNativeTracerFactoryDelegate tracerDelegate) => | |
| _tracers.TryAdd(tracerName, tracerDelegate); |
|
Replaced by #10228 |
Changes
_gasCostAlreadySetForCurrentOpguard inGethLikeTxTracerbase classReportOperationRemainingGassealed, add protected virtualOnOperationRemainingGascallbackGethLikeTxTracer<TEntry>,GethLikeJavaScriptTxTracer, andNativeCallTracerto useOnOperationRemainingGasRegisterExternalTracermethod toGethLikeNativeTracerFactoryfor plugin tracer registrationTypes of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes