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

Improve substreams error handling #5160

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Conversation

mangas
Copy link
Contributor

@mangas mangas commented Jan 25, 2024

  • Surface substreams errors
  • Fail subgraph if a substreams error is marked as deterministic
  • Consider substreams errors deterministic if Code::InvalidArgument is returned as the grpc status code
  • Replace a few Option<String> with FirehoseCursor.
  • Remove deterministic error on None block assertion

@mangas mangas force-pushed the filipe/handle-substreams-errors branch 3 times, most recently from e12696c to 0441af0 Compare January 25, 2024 14:01
@fordN fordN requested a review from incrypto32 January 30, 2024 16:42
@@ -408,6 +409,10 @@ pub trait BlockStreamMapper<C: Blockchain>: Send + Sync {
None => return Err(SubstreamsError::MissingClockError),
};

if clock.number > 11808300 {
panic!("end block")
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the panic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch, this was leftover from rebase I think

);
return;
}
_ => unreachable!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is BlockStreamError::Unknown not handled? would only Fatal errors be returned at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at the moment, only deterministic errors get returned, maybe I'll add some more information to this, thanks for pointing it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had left a comment on process_blocks but I will add something here as well

@@ -748,10 +748,7 @@ fn insert_subgraph_error(conn: &PgConnection, error: &SubgraphError) -> anyhow::
} = error;

let block_num = match &block_ptr {
None => {
assert_eq!(*deterministic, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this assertion removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was discussed with the team, as far as I can understand this was a sanity check that determisnistic errors could only happen with a block number being set, this is not longer the case with substreams, since the deterministic error can even happen at boot time, for example, with incorrect number of arguments.

@mangas mangas force-pushed the filipe/handle-substreams-errors branch from 0441af0 to 56fed4b Compare January 31, 2024 11:34
Copy link
Contributor

@incrypto32 incrypto32 left a comment

Choose a reason for hiding this comment

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

LGTM

@mangas mangas merged commit 4641d04 into master Feb 1, 2024
7 checks passed
@mangas mangas deleted the filipe/handle-substreams-errors branch February 1, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🚗 Merged
Development

Successfully merging this pull request may close these issues.

None yet

2 participants