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

Fixing Monitor crashing regularly, small additional fixes #1669

Open
wants to merge 3 commits into
base: staging
Choose a base branch
from

Conversation

kuzdogan
Copy link
Member

@kuzdogan kuzdogan commented Sep 27, 2024

The Monitors were crashing because we were throwing here

throw new Error(
"Failed to send contract to Sourcify server after multiple attempts.",
);
}

  • Removes the throw
  • Fixes the Avalanche RPC URLs
  • Removes the launch.json built and run Monitor

@manuelwedler
Copy link
Collaborator

Why shouldn't we throw in that place?

@kuzdogan
Copy link
Member Author

@manuelwedler Why should we? The monitor was able to successfully assemble the contract and send a request to the Sourcify server. It's a problem either on the network (probably this in our case) or an error of the Sourcify server. I don't think this should stop the Monitor from running.

We already log an error and actually we need to pay attention to that if there's an increase in errors. This wouldn't work now as we have plenty of these errors and need to fix it first.

@marcocastignoli
Copy link
Member

I agree with @kuzdogan that the monitor should not be stopped for that error. When I implemented it I was confused by the try-catch at the top of the callstack

start = async (): Promise<void> => {
this.chainLogger.info("Starting ChainMonitor");
try {
if (this.running) {
this.chainLogger.warn("ChainMonitor already running");
return;
}
this.running = true;
if (this.startBlock === undefined) {
this.chainLogger.info(
"No start block provided. Starting from last block",
);
this.startBlock = await this.sourcifyChain.getBlockNumber();
}
this.chainLogger.info("Starting polling", { block: this.startBlock });
// Start polling
this.pollBlocks(this.startBlock);
// Listen to new blocks
this.on(NEW_BLOCK_EVENT, this.processBlockListener);
// Start logging block pause periodically
this.startBlockPauseLogging();
} catch (error: any) {
this.chainLogger.error("Error starting ChainMonitor", { error });
}
};

Since this is an event being triggered, the error it's not catched. Instead of not throwing, I think it makes more sense to add another try-catch inside processBlockListener, so we have a central place in which all errors are catched inside the event.

@manuelwedler
Copy link
Collaborator

@manuelwedler Why should we? The monitor was able to successfully assemble the contract and send a request to the Sourcify server. It's a problem either on the network (probably this in our case) or an error of the Sourcify server. I don't think this should stop the Monitor from running.

We already log an error and actually we need to pay attention to that if there's an increase in errors. This wouldn't work now as we have plenty of these errors and need to fix it first.

Okay makes sense to me. I didn't understand that the monitor was crashing for it. Thanks for explaining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Sprint - Up Next
Development

Successfully merging this pull request may close these issues.

3 participants