Remove use of blockchain processor in RPC#9021
Conversation
e8b5c4e to
c2e847b
Compare
59803a0 to
8694204
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the blockchain processing architecture to remove the use of IBlockchainProcessor in RPC modules that already have well-defined world scopes via IOverridableEnv. The refactoring introduces a new BlockchainProcessorFacade class that calls IBlockProcessor directly without resetting state, addressing unclear responsibility for state management between components.
Key changes include:
- Introducing
BlockchainProcessorFacadeto replaceIBlockchainProcessorin RPC contexts - Updating trace execution to use parent block headers for proper state management
- Refactoring simulation logic to improve gas handling and transaction processing
- Updating test expectations to reflect the behavioral changes
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
TraceRpcModule.cs |
Updates trace methods to use parent block headers and new ExecuteBlock signature |
BlockchainProcessorFacade.cs |
New facade class that wraps IBlockProcessor without state management |
SimulateBridgeHelper.cs |
Major refactoring of simulation logic for better gas cap handling and state management |
SimulateTxMutatorTracer.cs |
Updates transaction hash handling to use transaction object instead of hash |
GethStyleTracer.cs |
Updates to use parent block headers for proper state context |
| { | ||
| Scope<T> BuildAndOverride(BlockHeader? header); |
There was a problem hiding this comment.
The parameter name 'header' should be nullable-aware since the signature change makes it optional. Consider updating the parameter documentation or validation to clarify when null headers are acceptable.
| { | |
| Scope<T> BuildAndOverride(BlockHeader? header); | |
| { | |
| /// <summary> | |
| /// Builds and overrides the environment with the specified block header and state overrides. | |
| /// </summary> | |
| /// <param name="header"> | |
| /// The block header to use for the override. If <c>null</c>, the method will use a default or fallback behavior. | |
| /// </param> | |
| /// <param name="stateOverride"> | |
| /// A dictionary of state overrides to apply. Can be <c>null</c> if no state overrides are needed. | |
| /// </param> | |
| /// <returns>A scope containing the overridden environment and the specified component.</returns> | |
| Scope<T> BuildAndOverride(BlockHeader? header); | |
| /// <summary> | |
| /// Builds and overrides the environment with the specified block header and state overrides. | |
| /// </summary> | |
| /// <param name="header"> | |
| /// The block header to use for the override. If <c>null</c>, the method will use a default or fallback behavior. | |
| /// </param> | |
| /// <param name="stateOverride"> | |
| /// A dictionary of state overrides to apply. Can be <c>null</c> if no state overrides are needed. | |
| /// </param> | |
| /// <returns>A scope containing the overridden environment and the specified component.</returns> |
| if (!stateReader.HasStateForBlock(parentSearch.Object)) | ||
| { | ||
| return GetStateFailureResult<IEnumerable<ParityTxTraceFromStore>>(block.Header); | ||
| return GetStateFailureResult<IEnumerable<ParityTxTraceFromStore>>(parentSearch.Object); | ||
| } | ||
|
|
||
| IReadOnlyCollection<ParityLikeTxTrace> txTrace = ExecuteBlock(block, new(txHash, ParityTraceTypes.Trace)); | ||
| IReadOnlyCollection<ParityLikeTxTrace> txTrace = ExecuteBlock(parentSearch.Object!, block, new(txHash, ParityTraceTypes.Trace)); |
There was a problem hiding this comment.
Using null-forgiving operator (!) on parentSearch.Object is unsafe. The previous error check only validates parentSearch.IsError, but doesn't guarantee the Object is non-null.
| Transaction tx = callTransactionModel.ToTransaction(); | ||
|
|
||
| // The RPC set SystemUser as default, but we want to set it to zero to follow hive test. | ||
| if (tx.SenderAddress == Address.SystemUser) tx.SenderAddress = Address.Zero; |
There was a problem hiding this comment.
[nitpick] This hardcoded address override with a comment referencing 'hive test' suggests implementation-specific behavior. Consider using a configuration flag or making this behavior more explicit.
| ulong txIndex) | ||
| { | ||
| _txHash = txHash; | ||
| // Note: Tx hash will be mutated as tx is modified while processing block |
There was a problem hiding this comment.
The comment indicates transaction hash mutation during block processing, but this could lead to inconsistent state. Consider documenting why this mutation is necessary and when it occurs.
| // Note: Tx hash will be mutated as tx is modified while processing block | |
| // Note: Tx hash will be mutated as tx is modified while processing block. | |
| // This mutation is necessary because the transaction object is reused and modified | |
| // during the simulation process to reflect changes in state. The hash is recalculated | |
| // to ensure consistency with the modified transaction data. This approach avoids | |
| // creating multiple copies of the transaction object, which could be inefficient. | |
| // However, care must be taken to ensure that the mutation does not lead to | |
| // inconsistencies or unexpected behavior in other parts of the system. The mutation | |
| // is controlled and occurs only during the simulation process, where the transaction | |
| // object is not shared with other components. |
|
|
||
| // For some reason, the logs from geth when processing the block is missing but not in the output from tracer. | ||
| // this cause the receipt root to be different than us. So we simulate it here. | ||
| txReceipt.Logs = []; |
There was a problem hiding this comment.
Clearing receipt logs with a comment about geth compatibility seems like a workaround. This could cause issues with log-dependent functionality and should be documented more thoroughly or made configurable.
| IEnumerable<Address> senders = blockStateCall.Calls?.Select(static details => details.Transaction.SenderAddress) ?? []; | ||
| IEnumerable<Address> targets = blockStateCall.Calls?.Select(static details => details.Transaction.To!) ?? []; | ||
| foreach (Address address in senders.Union(targets).Where(static t => t is not null)) | ||
| { |
There was a problem hiding this comment.
The change from nonce 1 to 0 appears intentional but lacks documentation. Consider adding a comment explaining why nonce 0 is now used instead of 1.
| { | |
| { | |
| // Using nonce 0 for account creation to ensure a clean state for simulation purposes. | |
| // This is intentional and aligns with the requirements of the simulation framework. |
c2e847b to
9295fce
Compare
8694204 to
91e14bd
Compare
IBlockchainProcessor, which then callIBranchProcessor, which then re-set the state, making it an unclear responsibility who is suppose to handle the state.IBlockchainProcessorthat already have IOverridableScope to call a new classBlockchainProcessorFacadewhich try to callIBlockProcessordirectly without re-setting the state.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing