fix: detect invalid buffered block when insert fails #10217
Conversation
Rjected
added
C-bug
crates/engine/tree/src/tree/mod.rs
Outdated
| @@ -1391,11 +1391,14 @@ where | |||
| } | |||
| Err(err) => { | |||
| debug!(target: "engine", ?err, "failed to connect buffered block to tree"); | |||
| if let Err(fatal) = self.on_insert_block_error(err) { | |||
| warn!(target: "engine", ?fatal, "fatal error occurred while connecting buffered blocks"); | |||
There was a problem hiding this comment.
fatal is mislead here imo because fatal means that this block is invalid not that a fatal error happened?
could we rephrase this?
There was a problem hiding this comment.
Validation errors should be filtered out and handled in insert_block_error, the type in the Err is an InsertBlockFatalError
There was a problem hiding this comment.
yeah, my point is how should "fatal" be interpreted if a user sees this on the terminal
fatal error occurred while connecting buffered blocks
this lacks context that this means only the insert routine was fatal, but I'd like to avoid logging "fatal" here
There was a problem hiding this comment.
because we don't terminate the task here, right? so it's not fatal, but maybe it should?
There was a problem hiding this comment.
because we don't terminate the task here, right? so it's not fatal, but maybe it should?
Yeah, I guess we should bubble these up, since it would be an error that we can't otherwise do anything with
There was a problem hiding this comment.
okay, no I fully understood it, this is indeed a fatal internal error but can don't have an easy way to terminate the service, since we don't really expect this case under normal ops, this is appropriate until we find a way to either terminate the task or emit an error message
crates/engine/tree/src/tree/mod.rs
Outdated
| @@ -1391,11 +1391,14 @@ where | |||
| } | |||
| Err(err) => { | |||
| debug!(target: "engine", ?err, "failed to connect buffered block to tree"); | |||
| if let Err(fatal) = self.on_insert_block_error(err) { | |||
| warn!(target: "engine", ?fatal, "fatal error occurred while connecting buffered blocks"); | |||
There was a problem hiding this comment.
this impls display I believe?
| warn!(target: "engine", ?fatal, "fatal error occurred while connecting buffered blocks"); | |
| warn!(target: "engine", %fatal, "fatal error occurred while connecting buffered blocks"); |
crates/engine/tree/src/tree/mod.rs
Outdated
| if let Err(fatal) = self.on_insert_block_error(err) { | ||
| warn!(target: "engine", ?fatal, "fatal error occurred while inserting downloaded block"); | ||
| } |
This progresses us further in the
Invalid Missing Ancestor Syncing ReOrgtest, and allows us to actually detect the invalid ancestor block. Instead of justdebuglogging on each insert block error, we now perform theon_insert_block_errorcheck whenever we get an error. This will inspect the error and insert it into the invalid headers cache if it was an actual invalid block.