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

fix(portal-bridge): re-initialize census if no peers are available #1481

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Sep 24, 2024

What was wrong?

One of the state bridges is stuck, and only displays a large amount of No known peers in state census, unable to offer. errors in the logs.

How was it fixed?

I had trouble re-producing this locally, but I think these changes will help. There are 2 cases where we might want to re-initialize the census

  1. Initial initialization failed, in which case we just exit the bridge process and the docker restart policy will restart the container
  2. Peers drop offline during gossip, in which case we just re-initialize the census during gossip so we don't have to restart the whole process

To-Do

@njgheorghita njgheorghita marked this pull request as ready for review September 24, 2024 18:08
@njgheorghita njgheorghita marked this pull request as draft September 24, 2024 18:13
@njgheorghita njgheorghita force-pushed the census-re-init branch 3 times, most recently from 166d327 to aea1b93 Compare September 24, 2024 18:46
@njgheorghita njgheorghita marked this pull request as ready for review September 24, 2024 18:53
Copy link
Collaborator

@morph-dev morph-dev left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -52,11 +59,15 @@ impl Census {
}

impl Census {
pub async fn init(&mut self) {
pub async fn init(&mut self) -> anyhow::Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should this also be Result<(), CensusError>?

@@ -56,7 +56,9 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
let (census_tx, census_rx) = mpsc::unbounded_channel();
let mut census = Census::new(portal_client.clone(), census_rx);
// initialize the census to acquire critical threshold view of network before gossiping
census.init().await;
if census.init().await.is_err() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: In my opinion, main can just return anyhow::Result<()> and you can use anyhow!(..) here. Or just use census.init().await?; and propagate error.

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit:

@njgheorghita njgheorghita merged commit 0914534 into ethereum:master Sep 25, 2024
9 checks passed
@njgheorghita njgheorghita deleted the census-re-init branch September 25, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants