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

feat(engine): compare invalid block witness against a healthy node #10844

Merged
merged 10 commits into from
Sep 16, 2024

Conversation

shekhirin
Copy link
Collaborator

@shekhirin shekhirin commented Sep 11, 2024

Closes #10549

Adds a --debug.healthy-node-rpc-url CLI argument that initializes an RPC client, checks if the chain ID matches, and passes the client to the witness invalid block hook.

Later, if the witness hook is triggered, we query the healthy node for the witness of the same block, save the output in a separate file and compare it against the original one.

@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-healthy-node-rpc-url branch from 83f3f65 to f2d12f7 Compare September 11, 2024 14:53
@shekhirin shekhirin added C-enhancement New feature or request A-cli Related to the reth CLI A-trie Related to Merkle Patricia Trie implementation A-consensus Related to the consensus engine labels Sep 12, 2024
@shekhirin shekhirin marked this pull request as ready for review September 12, 2024 14:33
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

mostly nits and style comments

crates/node/builder/src/launch/common.rs Outdated Show resolved Hide resolved
crates/node/builder/src/launch/common.rs Outdated Show resolved Hide resolved
crates/engine/invalid-block-hooks/src/witness.rs Outdated Show resolved Hide resolved
crates/engine/invalid-block-hooks/src/witness.rs Outdated Show resolved Hide resolved
Comment on lines +871 to +876
let chain_id = futures::executor::block_on(async {
EthApiClient::<
reth_rpc_types::Transaction,
reth_rpc_types::Block,
reth_rpc_types::Receipt,
>::chain_id(&client)
Copy link
Member

Choose a reason for hiding this comment

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

makes me wonder if we should just provide a blocking api in EthApiClient somehow, but not something to address in this PR

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

nice, lgtm

@Rjected
Copy link
Member

Rjected commented Sep 13, 2024

needs doctest renames

let client = jsonrpsee::http_client::HttpClientBuilder::default().build(url)?;

// Verify that the healthy node is running the same chain as the current node.
let chain_id = futures::executor::block_on(async {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we even need this check?
I mean this is a debug feature, so do we need to explicitly check this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'd prefer to have it so we don't end up in a situation when generated witness diff is incorrect and we spend time figuring out why

@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-healthy-node-rpc-url branch from 64ac568 to 8b3969d Compare September 13, 2024 16:33
@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-healthy-node-rpc-url branch from 8b3969d to b980b8c Compare September 13, 2024 16:38
@shekhirin shekhirin added this pull request to the merge queue Sep 16, 2024
Merged via the queue into main with commit 664f8b2 Sep 16, 2024
36 checks passed
@shekhirin shekhirin deleted the alexey/invalid-block-hook-healthy-node-rpc-url branch September 16, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Related to the reth CLI A-consensus Related to the consensus engine A-trie Related to Merkle Patricia Trie implementation C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create witness comparison tool
3 participants