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, tree): witness invalid block hook #10685

Merged
merged 23 commits into from
Sep 6, 2024

Conversation

shekhirin
Copy link
Collaborator

@shekhirin shekhirin commented Sep 4, 2024

Closes #10547

Implements the Witness invalid block hook that saves the witness and preimages to <DIR>/<CHAIN_ID>/invalid_block_hooks/witness/<BLOCK_NUMBER>_<BLOCK_HASH>.json.

Tested by running with a following diff

diff --git a/crates/engine/tree/src/tree/mod.rs b/crates/engine/tree/src/tree/mod.rs
index dc6d0eb2f..b70b8e748 100644
--- a/crates/engine/tree/src/tree/mod.rs
+++ b/crates/engine/tree/src/tree/mod.rs
@@ -2077,7 +2077,7 @@ where
         let root_time = Instant::now();
         let (state_root, trie_output) =
             state_provider.state_root_with_updates(hashed_state.clone())?;
-        if state_root != block.state_root {
+        if true {
             // call post-block hook
             if let Err(hook_err) = self.invalid_block_hook.on_invalid_block(
                 &parent_block,

and comparing the hook output against a remotely running healthy node using cast rpc localhost:8543 debug_executionWitness $(cast to-hex BLOCK_NUMBER) true. Output JSONs were exactly matching when keys are sorted using jq -S.

@shekhirin shekhirin added C-enhancement New feature or request A-consensus Related to the consensus engine A-observability Related to tracing, metrics, logs and other observability tools A-trie Related to Merkle Patricia Trie implementation labels Sep 4, 2024
@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-witness branch from fdb47fb to 8a557f4 Compare September 4, 2024 10:52
@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-witness branch from 8dc3efc to f5bcce1 Compare September 4, 2024 13:13
@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-witness branch from f5bcce1 to fc92bb6 Compare September 4, 2024 13:15
@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-witness branch 2 times, most recently from 96e6827 to c0295d2 Compare September 5, 2024 11:56
@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-witness branch from c0295d2 to 6f32102 Compare September 5, 2024 11:57
@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-witness branch from 4573c80 to 1b91e73 Compare September 5, 2024 14:49
@shekhirin shekhirin marked this pull request as ready for review September 5, 2024 16:09
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.

lgtm, have one nit about field docs

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
@shekhirin shekhirin force-pushed the alexey/invalid-block-hook-witness branch from a6f5e3b to e1981d8 Compare September 6, 2024 09:18
_output: &BlockExecutionOutput<Receipt>,
_trie_updates: Option<(&TrieUpdates, B256)>,
) {
) -> eyre::Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, not a fan of returning a result here because this is a bit meaningless and results in more code in engine which just logs the error, but is fine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agreed, also by handling errors inside the hook, we will have better logging with the origin of error

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
crates/engine/invalid-block-hooks/src/witness.rs Outdated Show resolved Hide resolved
@shekhirin shekhirin added this pull request to the merge queue Sep 6, 2024
Merged via the queue into main with commit 3ec5d37 Sep 6, 2024
35 checks passed
@shekhirin shekhirin deleted the alexey/invalid-block-hook-witness branch September 6, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Related to the consensus engine A-observability Related to tracing, metrics, logs and other observability tools 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.

Add witness bad block hook implementation
4 participants