-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: add support for debug_getBadBlock #20177
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
base: main
Are you sure you want to change the base?
Conversation
klkvr
left a comment
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.
would prefer us to avoid doing breaking changes for this
| // keep track of invalid blocks for `debug_getBadBlocks` only if debug RPC is enabled | ||
| if debug_enabled { | ||
| let bad_block_store = registry.bad_block_store().clone(); | ||
| let mut engine_events_stream = engine_events.new_listener(); | ||
| node.task_executor().spawn(Box::pin(async move { | ||
| while let Some(event) = engine_events_stream.next().await { | ||
| if let ConsensusEngineEvent::InvalidBlock(block) = event && | ||
| let Ok(recovered) = | ||
| RecoveredBlock::try_recover_sealed(block.as_ref().clone()) | ||
| { | ||
| bad_block_store.insert(recovered); | ||
| } | ||
| } | ||
| })); | ||
| } |
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.
hmm can we not have this being handled by DebugApi itself?
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.
Have moved this part of logic to inside DebugApi
crates/rpc/rpc-eth-api/src/types.rs
Outdated
| pub trait EthApiTypes: RpcNodeCore + Send + Sync + Clone { | ||
| /// Extension of [`FromEthApiError`], with network specific errors. | ||
| type Error: Into<jsonrpsee_types::error::ErrorObject<'static>> | ||
| + FromEthApiError | ||
| + AsEthApiError | ||
| + From<<Self::RpcConvert as RpcConvert>::Error> | ||
| + Error | ||
| + Send | ||
| + Sync; | ||
| /// Blockchain primitive types, specific to network, e.g. block and transaction. | ||
| type NetworkTypes: RpcTypes; | ||
| /// Conversion methods for transaction RPC type. | ||
| type RpcConvert: RpcConvert<Network = Self::NetworkTypes>; | ||
| /// Provider block type used by RPC. | ||
| type ProviderBlock: BlockTrait; |
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.
would prefer us to avoid changing this trait
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.
Have revert changes to this trait , it remains the same as before
| + reth_ethereum::rpc::eth::RpcNodeCore< | ||
| Provider = Node::Provider, | ||
| Primitives = <Node::Types as NodeTypes>::Primitives, | ||
| >, |
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.
hmm unclear why this is needed. I know that those bounds are sometimes problematic but we need to figure out solutions for this that don't break such user-facing APIs
This targets on issue #20123 , as below :
Add a shared BadBlockStore (bounded, deduped, newest-first) and wire it into DebugApi
Implement debug_getBadBlocks to expose recent invalid blocks with full transactions, hash, and RLP
Listen for ConsensusEngineEvent::InvalidBlock, recover the sealed block with senders, and cache it for the debug API
cc @mattsse @klkvr @Rjected