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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion book/cli/reth/node.md
Original file line number Diff line number Diff line change
Expand Up @@ -541,12 +541,15 @@ Debug:
The path to store engine API messages at. If specified, all of the intercepted engine API messages will be written to specified location

--debug.invalid-block-hook <INVALID_BLOCK_HOOK>
Determines which type of bad block hook to install
Determines which type of invalid block hook to install

Example: `witness,prestate`

[possible values: witness, pre-state, opcode]

--debug.healthy-node-rpc-url <URL>
The RPC URL of a healthy node to use for comparing invalid block hook results against.

Database:
--db.log-level <LOG_LEVEL>
Database logging level. Levels higher than "notice" require a debug build
Expand Down
5 changes: 5 additions & 0 deletions crates/engine/invalid-block-hooks/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,19 @@ reth-evm.workspace = true
reth-primitives.workspace = true
reth-provider.workspace = true
reth-revm.workspace = true
reth-rpc-api = { workspace = true, features = ["client"] }
reth-tracing.workspace = true
reth-trie = { workspace = true, features = ["serde"] }

# alloy
alloy-rlp.workspace = true
alloy-rpc-types-debug.workspace = true

# async
futures.workspace = true

# misc
eyre.workspace = true
jsonrpsee.workspace = true
pretty_assertions = "1.4"
serde_json.workspace = true
62 changes: 47 additions & 15 deletions crates/engine/invalid-block-hooks/src/witness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,33 @@ use reth_revm::{
primitives::{BlockEnv, CfgEnvWithHandlerCfg, EnvWithHandlerCfg},
DatabaseCommit, StateBuilder,
};
use reth_rpc_api::DebugApiClient;
use reth_tracing::tracing::warn;
use reth_trie::{updates::TrieUpdates, HashedPostState, HashedStorage};

/// Generates a witness for the given block and saves it to a file.
#[derive(Debug)]
pub struct InvalidBlockWitnessHook<P, EvmConfig> {
/// The directory to write the witness to. Additionally, diff files will be written to this
/// directory in case of failed sanity checks.
output_directory: PathBuf,
/// The provider to read the historical state and do the EVM execution.
provider: P,
/// The EVM configuration to use for the execution.
evm_config: EvmConfig,
/// The directory to write the witness to. Additionally, diff files will be written to this
/// directory in case of failed sanity checks.
output_directory: PathBuf,
/// The healthy node client to compare the witness against.
healthy_node_client: Option<jsonrpsee::http_client::HttpClient>,
}

impl<P, EvmConfig> InvalidBlockWitnessHook<P, EvmConfig> {
/// Creates a new witness hook.
pub const fn new(output_directory: PathBuf, provider: P, evm_config: EvmConfig) -> Self {
Self { output_directory, provider, evm_config }
pub const fn new(
provider: P,
evm_config: EvmConfig,
output_directory: PathBuf,
healthy_node_client: Option<jsonrpsee::http_client::HttpClient>,
) -> Self {
Self { provider, evm_config, output_directory, healthy_node_client }
}
}

Expand Down Expand Up @@ -148,12 +156,13 @@ where
let witness = state_provider.witness(HashedPostState::default(), hashed_state.clone())?;

// Write the witness to the output directory.
let mut file = File::options()
.write(true)
.create_new(true)
.open(self.output_directory.join(format!("{}_{}.json", block.number, block.hash())))?;
let response = ExecutionWitness { witness, state_preimages: Some(state_preimages) };
file.write_all(serde_json::to_string(&response)?.as_bytes())?;
File::create_new(self.output_directory.join(format!(
"{}_{}.json",
block.number,
block.hash()
)))?
.write_all(serde_json::to_string(&response)?.as_bytes())?;

// The bundle state after re-execution should match the original one.
if bundle_state != output.state {
Expand All @@ -179,6 +188,33 @@ where
}
}

if let Some(healthy_node_client) = &self.healthy_node_client {
// Compare the witness against the healthy node.
let healthy_node_witness = futures::executor::block_on(async move {
DebugApiClient::debug_execution_witness(
healthy_node_client,
block.number.into(),
true,
)
.await
})?;

// Write the healthy node witness to the output directory.
File::create_new(self.output_directory.join(format!(
"{}_{}.healthy_witness.json",
block.number,
block.hash()
)))?
.write_all(serde_json::to_string(&healthy_node_witness)?.as_bytes())?;

// If the witnesses are different, write the diff to the output directory.
if response != healthy_node_witness {
let filename = format!("{}_{}.healthy_witness.diff", block.number, block.hash());
let path = self.save_diff(filename, &response, &healthy_node_witness)?;
warn!(target: "engine::invalid_block_hooks::witness", path = %path.display(), "Witness mismatch against healthy node");
}
}

Ok(())
}

Expand All @@ -191,11 +227,7 @@ where
) -> eyre::Result<PathBuf> {
let path = self.output_directory.join(filename);
let diff = Comparison::new(original, new);
File::options()
.write(true)
.create_new(true)
.open(&path)?
.write_all(diff.to_string().as_bytes())?;
File::create_new(&path)?.write_all(diff.to_string().as_bytes())?;

Ok(path)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/engine/tree/src/tree/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ where
}
}

/// Sets the bad block hook.
/// Sets the invalid block hook.
fn set_invalid_block_hook(&mut self, invalid_block_hook: Box<dyn InvalidBlockHook>) {
self.invalid_block_hook = invalid_block_hook;
}
Expand Down
2 changes: 2 additions & 0 deletions crates/node/builder/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ reth-payload-validator.workspace = true
reth-primitives.workspace = true
reth-provider.workspace = true
reth-prune.workspace = true
reth-rpc-api.workspace = true
reth-rpc-builder.workspace = true
reth-rpc-engine-api.workspace = true
reth-rpc-eth-types.workspace = true
Expand Down Expand Up @@ -82,6 +83,7 @@ secp256k1 = { workspace = true, features = [
aquamarine.workspace = true
eyre.workspace = true
fdlimit.workspace = true
jsonrpsee.workspace = true
rayon.workspace = true

# tracing
Expand Down
89 changes: 59 additions & 30 deletions crates/node/builder/src/launch/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use std::{sync::Arc, thread::available_parallelism};

use alloy_primitives::{BlockNumber, B256};
use eyre::Context;
use eyre::{Context, OptionExt};
use rayon::ThreadPoolBuilder;
use reth_auto_seal_consensus::MiningMode;
use reth_beacon_consensus::EthBeaconConsensus;
Expand All @@ -23,6 +23,7 @@ use reth_invalid_block_hooks::InvalidBlockWitnessHook;
use reth_network_p2p::headers::client::HeadersClient;
use reth_node_api::{FullNodeTypes, NodeTypes, NodeTypesWithDB};
use reth_node_core::{
args::InvalidBlockHookType,
dirs::{ChainPath, DataDirPath},
node_config::NodeConfig,
version::{
Expand All @@ -44,6 +45,7 @@ use reth_provider::{
TreeViewer,
};
use reth_prune::{PruneModes, PrunerBuilder};
use reth_rpc_api::clients::EthApiClient;
use reth_rpc_builder::config::RethRpcServerConfig;
use reth_rpc_layer::JwtSecret;
use reth_stages::{sets::DefaultStages, MetricEvent, Pipeline, PipelineTarget, StageId};
Expand Down Expand Up @@ -855,35 +857,62 @@ where
{
/// Returns the [`InvalidBlockHook`] to use for the node.
pub fn invalid_block_hook(&self) -> eyre::Result<Box<dyn InvalidBlockHook>> {
Ok(if let Some(ref hook) = self.node_config().debug.invalid_block_hook {
let output_directory = self.data_dir().invalid_block_hooks();
let hooks = hook
.iter()
.copied()
.map(|hook| {
let output_directory = output_directory.join(hook.to_string());
fs::create_dir_all(&output_directory)?;

Ok(match hook {
reth_node_core::args::InvalidBlockHook::Witness => {
Box::new(InvalidBlockWitnessHook::new(
output_directory,
self.blockchain_db().clone(),
self.components().evm_config().clone(),
)) as Box<dyn InvalidBlockHook>
}
reth_node_core::args::InvalidBlockHook::PreState |
reth_node_core::args::InvalidBlockHook::Opcode => {
eyre::bail!("invalid block hook {hook:?} is not implemented yet")
}
})
})
.collect::<Result<_, _>>()?;

Box::new(InvalidBlockHooks(hooks))
} else {
Box::new(NoopInvalidBlockHook::default())
})
let Some(ref hook) = self.node_config().debug.invalid_block_hook else {
return Ok(Box::new(NoopInvalidBlockHook::default()))
};
let healthy_node_rpc_client = self.get_healthy_node_client()?;

let output_directory = self.data_dir().invalid_block_hooks();
let hooks = hook
.iter()
.copied()
.map(|hook| {
let output_directory = output_directory.join(hook.to_string());
fs::create_dir_all(&output_directory)?;

Ok(match hook {
InvalidBlockHookType::Witness => Box::new(InvalidBlockWitnessHook::new(
self.blockchain_db().clone(),
self.components().evm_config().clone(),
output_directory,
healthy_node_rpc_client.clone(),
)),
InvalidBlockHookType::PreState | InvalidBlockHookType::Opcode => {
eyre::bail!("invalid block hook {hook:?} is not implemented yet")
}
} as Box<dyn InvalidBlockHook>)
})
.collect::<Result<_, _>>()?;

Ok(Box::new(InvalidBlockHooks(hooks)))
}

/// Returns an RPC client for the healthy node, if configured in the node config.
fn get_healthy_node_client(&self) -> eyre::Result<Option<jsonrpsee::http_client::HttpClient>> {
self.node_config()
.debug
.healthy_node_rpc_url
.as_ref()
.map(|url| {
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

EthApiClient::<
reth_rpc_types::Transaction,
reth_rpc_types::Block,
reth_rpc_types::Receipt,
>::chain_id(&client)
Comment on lines +900 to +905
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

.await
})?
.ok_or_eyre("healthy node rpc client didn't return a chain id")?;
if chain_id.to::<u64>() != self.chain_id().id() {
eyre::bail!("invalid chain id for healthy node: {chain_id}")
}

Ok(client)
})
.transpose()
}
}

Expand Down
Loading
Loading