feat: Properly bubble up `InsertBlockFatalError` #10276

martinezjorge · 2024-08-12T22:17:29Z

…oll, and emitting events

crates/engine/tree/src/engine.rs

crates/engine/tree/src/tree/mod.rs

martinezjorge · 2024-08-12T22:26:57Z

@Rjected Could I get some feedback/direction on this approach?

Rjected

instead of using emit_event, let's ensure that the methods which call on_insert_block_error return a Result so we can do

self.on_insert_block_error(err)?;

I like this direction as opposed to using events to bubble the error, because the events contain more "actual" information we want the engine to handle, and the result Err variant seems like a better fit for fatal errors.
does that make sense?

martinezjorge · 2024-08-17T06:17:59Z

@Rjected I switched from bubbling towards main thread using emits to bubbling up in EngineApiTreeHandler until the fn run. I also updated the tests. Please let me know you'd like me to approach the bubble up or the tests differently or if there is a test you'd like me to add.

Rjected

nice! some comments on result handling and the run signature

Rjected · 2024-08-17T16:41:30Z

crates/engine/tree/src/tree/mod.rs

+                    if let Err(fatal) = self.on_backfill_sync_finished(ctrl) {
+                        Err(fatal)
+                    } else {
+                        Ok(())
+                    }


Suggested change

if let Err(fatal) = self.on_backfill_sync_finished(ctrl) {

Err(fatal)

} else {

Ok(())

}

self.on_backfill_sync_finished(ctrl)?;

Rjected · 2024-08-17T16:42:55Z

crates/engine/tree/src/tree/mod.rs

                            }
                        }
                    }
                }
            }
-            FromEngine::DownloadedBlocks(blocks) => {
-                if let Some(event) = self.on_downloaded(blocks) {
+            FromEngine::DownloadedBlocks(blocks) => match self.on_downloaded(blocks) {


this can be

FromEngine::DownloadedBlocks(blocks) => { if let Some(event) = self.on_downloaded(blocks)? { self.on_tree_event(event); } }

Rjected · 2024-08-17T16:44:40Z

crates/engine/tree/src/tree/mod.rs

        }

        trace!(target: "engine", block_count = %blocks.len(), "received downloaded blocks");
        let batch = self.config.max_execute_block_batch_size().min(blocks.len());
        for block in blocks.drain(..batch) {
-            if let Some(event) = self.on_downloaded_block(block) {
+            if let Ok(Some(event)) = self.on_downloaded_block(block) {


Suggested change

if let Ok(Some(event)) = self.on_downloaded_block(block) {

if let Some(event) = self.on_downloaded_block(block)? {

Rjected · 2024-08-17T16:48:01Z

crates/engine/tree/src/tree/mod.rs

@@ -500,24 +500,23 @@ where
    /// Run the engine API handler.
    ///
    /// This will block the current thread and process incoming messages.
-    pub fn run(mut self) {
+    pub fn run(mut self) -> Result<(), InsertBlockFatalError> {


in this method I would prefer that the signature remain as run(mut self), and instead we handle the on_engine_message like this:

if let Err(err) = self.on_engine_message(msg) { // some error! message return }

Okay no problem, I was not sure about this detail. Thank you for clarifying.

crates/engine/tree/src/chain.rs

…ng clone attribute macro to HandlerEvent<T>

martinezjorge · 2024-08-17T18:51:58Z

Made the requested changes but I'm having a couple unit tests that are hanging

test tree::tests::test_engine_tree_live_sync_transition_eventually_canonical has been running for over 60 seconds
test tree::tests::test_engine_tree_live_sync_transition_required_blocks_requested has been running for over 60 seconds

So investigating, trying to figure out what's going on with those

crates/engine/tree/src/tree/mod.rs

Rjected

Nice! I have some small nitpicks but this seems good otherwise

Rjected · 2024-08-26T20:43:08Z

crates/engine/tree/src/tree/mod.rs

@@ -169,7 +169,7 @@ impl TreeState {
        current_hash = self.canonical_block_hash();
        while let Some(current_block) = self.block_by_hash(current_hash) {
            if current_block.hash() == target_hash {
-                return false
+                return false;


one nit: let's remove the added semicolons

Rjected · 2024-08-26T20:43:56Z

crates/engine/tree/src/tree/mod.rs

+                        break;
+                    }
                }
                Ok(None) => {
                    debug!(target: "engine", "received no engine message for some time, while waiting for persistence task to complete");
                }
                Err(_err) => {
                    error!(target: "engine", "Engine channel disconnected");
-                    return
+                    return;
                }
            }

            if let Err(err) = self.advance_persistence() {
                error!(target: "engine", %err, "Advancing persistence failed");
-                break
+                break;


let's return here, they should be equivalent but this is just to stay consistent

Rjected · 2024-08-26T20:44:16Z

crates/engine/tree/src/tree/mod.rs


        // send fcu to the tip of main
        test_harness.fcu_to(main_chain_last_hash, ForkchoiceStatus::Syncing).await;

        let event = test_harness.from_tree_rx.recv().await.unwrap();
+        println!("event received: {:?}", event);


Suggested change

println!("event received: {:?}", event);

Rjected · 2024-08-26T20:44:24Z

crates/engine/tree/src/tree/mod.rs


        let event = test_harness.from_tree_rx.recv().await.unwrap();
+        println!("event received: {:?}", event);


Suggested change

println!("event received: {:?}", event);

Rjected

This looks good to me, thanks!

wip: added fatal error variant to EngineApiEvent, event listener in p…

50a84e9

…oll, and emitting events

martinezjorge commented Aug 12, 2024

View reviewed changes

crates/engine/tree/src/engine.rs Outdated Show resolved Hide resolved

martinezjorge commented Aug 12, 2024

View reviewed changes

crates/engine/tree/src/tree/mod.rs Outdated Show resolved Hide resolved

shekhirin added C-enhancement New feature or request A-blockchain-tree Related to sidechains, reorgs and pending blocks labels Aug 12, 2024

martinezjorge changed the title ~~WIP: enhancement: Properly bubble up InsertBlockFatalError~~ feat: Properly bubble up InsertBlockFatalError Aug 14, 2024

Merge branch 'main' into bubble-up-insertblockfatalerror

2e8a460

Rjected requested changes Aug 16, 2024

View reviewed changes

martinezjorge added 2 commits August 16, 2024 16:17

bubbling up the error from engine task to main task

98ac173

result over emit, fmt, fix tests, and revert previous changes

890a8fb

martinezjorge marked this pull request as ready for review August 17, 2024 06:10

martinezjorge requested review from rkrasiuk, mattsse and fgimenez as code owners August 17, 2024 06:10

touchup

115e9b1

Rjected requested changes Aug 17, 2024

View reviewed changes

martinezjorge added 2 commits August 17, 2024 11:48

run return signature revert, adding error message to run, and reverti…

70ec307

…ng clone attribute macro to HandlerEvent<T>

fmt

0702972

martinezjorge added 4 commits August 17, 2024 15:56

Merge branch 'main' into bubble-up-insertblockfatalerror

391a3aa

tests passing

b103ac3

Merge branch 'main' into bubble-up-insertblockfatalerror

e7f2d2b

touchups

62b3a37

martinezjorge requested a review from Rjected August 23, 2024 21:06

martinezjorge commented Aug 24, 2024

View reviewed changes

crates/engine/tree/src/tree/mod.rs Outdated Show resolved Hide resolved

martinezjorge added 3 commits August 26, 2024 13:14

break on fatalinsertblockerror

3fa91f6

fixing merge conflict

5b7058e

fmt

6cb076b

Rjected requested changes Aug 26, 2024

View reviewed changes

cleanup and touchups

5c2d919

Rjected approved these changes Aug 26, 2024

View reviewed changes

Rjected added this pull request to the merge queue Aug 26, 2024

Merged via the queue into paradigmxyz:main with commit 1c427c6 Aug 26, 2024
34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Properly bubble up `InsertBlockFatalError` #10276

feat: Properly bubble up `InsertBlockFatalError` #10276

martinezjorge commented Aug 12, 2024 •

edited

Loading

martinezjorge commented Aug 12, 2024

Rjected left a comment •

edited

Loading

martinezjorge commented Aug 17, 2024

Rjected left a comment

Rjected Aug 17, 2024

Rjected Aug 17, 2024

Rjected Aug 17, 2024

Rjected Aug 17, 2024

martinezjorge Aug 17, 2024

martinezjorge commented Aug 17, 2024

Rjected left a comment

Rjected Aug 26, 2024

martinezjorge Aug 26, 2024

Rjected Aug 26, 2024

martinezjorge Aug 26, 2024

Rjected Aug 26, 2024

martinezjorge Aug 26, 2024

Rjected Aug 26, 2024

martinezjorge Aug 26, 2024

Rjected left a comment

	if let Ok(Some(event)) = self.on_downloaded_block(block) {
	if let Some(event) = self.on_downloaded_block(block)? {


		let event = test_harness.from_tree_rx.recv().await.unwrap();
		println!("event received: {:?}", event);

feat: Properly bubble up InsertBlockFatalError #10276

feat: Properly bubble up InsertBlockFatalError #10276

Conversation

martinezjorge commented Aug 12, 2024 • edited Loading

martinezjorge commented Aug 12, 2024

Rjected left a comment • edited Loading

Choose a reason for hiding this comment

martinezjorge commented Aug 17, 2024

Rjected left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinezjorge commented Aug 17, 2024

Rjected left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rjected left a comment

Choose a reason for hiding this comment

feat: Properly bubble up `InsertBlockFatalError` #10276

feat: Properly bubble up `InsertBlockFatalError` #10276

martinezjorge commented Aug 12, 2024 •

edited

Loading

Rjected left a comment •

edited

Loading