Skip to content

timeout when trying to get net_identifiers at startup #5568

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 26, 2024
Merged

timeout when trying to get net_identifiers at startup #5568

merged 2 commits into from
Jul 26, 2024

Conversation

mangas
Copy link
Contributor

@mangas mangas commented Jul 25, 2024 •
edited
Loading

  1. If an error occurs during startup net_identifier calls, just ignore an use a default value
  2. If validation is enabled, this will cause the adapter to eventually fail because whatever genesis the adapter returns won't match the default (0x00) genesis value.

This change prevents the graph-node from failing to start due to broken adapters but if genesis validation is enabled it should eventually update the store

@mangas mangas force-pushed the net-ident-timeout branch from 4b6287b to b3030ac Compare July 25, 2024 17:34
@mangas mangas force-pushed the net-ident-timeout branch from b3030ac to d872893 Compare July 25, 2024 17:39
@mangas mangas requested a review from incrypto32 July 26, 2024 08:54
zorancv
zorancv approved these changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@zorancv zorancv left a comment

Choose a reason for hiding this comment

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

So we don't try to enforce specific genesis block, right? In that case it LGTM.

@mangas
Copy link
Contributor Author

mangas commented Jul 26, 2024

So we don't try to enforce specific genesis block, right? In that case it LGTM.

Yeah unfortunately the erroring part can cause an outage easily so I think the best course is to try to keep running and then if something fails it should happen for the specific chain

@zorancv
Copy link
Contributor

zorancv commented Jul 26, 2024

So we don't try to enforce specific genesis block, right? In that case it LGTM.

Yeah unfortunately the erroring part can cause an outage easily so I think the best course is to try to keep running and then if something fails it should happen for the specific chain

I guess more conservative approach would be to allow failures for the 'sloppy' chains and be strict for the 'good' ones. However not sure if even that is possible given that firehose can't provide it, for instance... At any rate merging this seems very reasonable in order to solve the pressing issue. and it's already strictly better than the original behaviour.

@mangas mangas force-pushed the net-ident-timeout branch from 775d38d to 9997d48 Compare July 26, 2024 10:29
zorancv
zorancv reviewed Jul 26, 2024
View reviewed changes
node/src/manager/commands/config.rs
@@ -177,6 +177,7 @@ pub async fn provider(
registry,
metrics,
Arc::new(NoopIdentValidator),
false,
Copy link
Contributor

@zorancv zorancv Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Don't you want it dependant on the config in graphman too? Also above in the manager.rs...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only for run, the others don't depend on the ProviderManager component

@mangas mangas merged commit f99d68c into master Jul 26, 2024
7 checks passed
@mangas mangas deleted the net-ident-timeout branch July 26, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zorancv zorancv zorancv approved these changes

@incrypto32 incrypto32 Awaiting requested review from incrypto32

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mangas @zorancv