refactor: fix state unavailability handling in DebugRpcModule #8432
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the DebugRpcModule to improve state unavailability handling by adding state checks and a dedicated error method, and updates related factory and extension methods.
- Added IStateReader parameter and state availability checks in DebugRpcModule
- Introduced GetStateFailureResult for uniform error responses when state is missing
- Updated DebugModuleFactory to pass the IStateReader instance
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Nethermind/Nethermind.State/StateReaderExtensions.cs | Added a new GetAccount extension method to retrieve account details without verifying the success of TryGetAccount |
| src/Nethermind/Nethermind.JsonRpc/Modules/DebugModule/DebugRpcModule.cs | Updated various debug_trace* methods to include state availability checks and modified error handling in debug_resetHead and others |
| src/Nethermind/Nethermind.JsonRpc/Modules/DebugModule/DebugModuleFactory.cs | Updated the factory to pass IStateReader from the world state manager |
Comments suppressed due to low confidence (2)
src/Nethermind/Nethermind.JsonRpc/Modules/DebugModule/DebugRpcModule.cs:399
- In debug_resetHead, returning a success with a null result when the transaction is not found deviates from the previous error handling pattern. Confirm that this change in behavior is intentional and, if not, consider reverting to a failure response with an appropriate error message.
return ResultWrapper<string?>.Success(null);
src/Nethermind/Nethermind.JsonRpc/Modules/DebugModule/DebugRpcModule.cs:414
- The updated debug_getRawReceipts method no longer applies the specific RlpBehaviors (e.g., Eip658Receipts) previously used for encoding. Verify that using the default Rlp.Encode is acceptable for all cases or reintroduce the necessary encoding behavior.
TxReceipt[]? receipts = _debugBridge.GetReceiptsForBlock(blockParameter);
| @@ -58,5 +58,15 @@ public static TrieStats CollectStats(this IStateReader stateProvider, Hash256 ro | |||
| }); | |||
| return collector.Stats; | |||
| } | |||
|
|
|||
| public static Account GetAccount(this IStateReader stateReader, Hash256 stateRoot, Address address) | |||
There was a problem hiding this comment.
The GetAccount extension method does not verify the return value of TryGetAccount. Consider checking its success before constructing a new Account to avoid propagating uninitialized or default values.
Copilot uses AI. Check for mistakes.
|
@benaadams hi, I'm not sure if I have to mention you(sorry if not) but as you launched copilot I thought maybe you can review my changes |
| @@ -65,6 +68,33 @@ public ResultWrapper<int> debug_deleteChainSlice(in long startNumber, bool force | |||
|
|
|||
| public ResultWrapper<GethLikeTxTrace> debug_traceTransaction(Hash256 transactionHash, GethTraceOptions? options = null) | |||
| { | |||
| // First, find the block by transaction hash | |||
There was a problem hiding this comment.
Please avoid adding unnecessary comments.
| } | ||
|
|
||
| return ResultWrapper<byte[]>.Success(rlp); | ||
| byte[] data = _debugBridge.GetBlockRlp(blockNumber); |
There was a problem hiding this comment.
This check for null is relevant.
| } | ||
|
|
||
| return ResultWrapper<byte[]>.Success(rlp); | ||
| byte[] data = _debugBridge.GetBlockRlp(hash); |
There was a problem hiding this comment.
This check for null is relevant.
| @@ -58,5 +58,21 @@ public static TrieStats CollectStats(this IStateReader stateProvider, Hash256 ro | |||
| }); | |||
| return collector.Stats; | |||
| } | |||
|
|
|||
| public static Account GetAccount(this IStateReader stateReader, Hash256 stateRoot, Address address) | |||
There was a problem hiding this comment.
Could you explain why this method was added?
There was a problem hiding this comment.
This method was added to simplify direct account access. It's basically a cleaner alternative to TryGetAccount when you just need to grab an account at a specific address and state root without dealing with all the success/failure return values. Makes the calling code much more readable.
| { | ||
| return ResultWrapper<byte[]>.Fail($"Block {blockParameter} was not found", ErrorCodes.ResourceNotFound); | ||
| return ResultWrapper<byte[]>.Success(Array.Empty<byte>()); |
There was a problem hiding this comment.
Why was the error changed to an empty response?
| if (block is null) | ||
| { | ||
| return ResultWrapper<byte[]>.Fail($"Block {blockParameter} was not found", ErrorCodes.ResourceNotFound); | ||
| return ResultWrapper<byte[]>.Success(Array.Empty<byte>()); |
There was a problem hiding this comment.
Why was the error changed to an empty response?
| using CancellationTokenSource timeout = BuildTimeoutCancellationTokenSource(); | ||
| CancellationToken cancellationToken = timeout.Token; | ||
| var txTraces = _debugBridge.TraceBadBlockToFile(blockHash, cancellationToken, options); | ||
| return ResultWrapper<IEnumerable<string>>.Success(txTraces); |
There was a problem hiding this comment.
Please avoid removing _logger.Trace() calls.
| { | ||
| return ResultWrapper<string?>.Success(Rlp.Encode(transaction, RlpBehaviors.SkipTypedWrapping).Bytes.ToHexString(true)); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Please use a specific exception type to avoid returning ErrorCodes.InternalError when it's not relevant.
Fixes #8174
Changes
Types 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