Add common.RetryForever() and use for concurrent sync operations

[sergerad]

Description

Add common.RetryForever() and use for concurrent block/event synchronisation operations that can take a long time and fail at any point.

Background

The Edge application approaches some errors by logging and running the rest of the program without retrying the failed operation. This is undesirable when those operations are critical (such as relayer startup and sync, which can mean that deposits no longer work). Critical operations should be retried because:

1. It allows the application to self heal, rather than run without critical capabilities; and
2. It allows for more reliable alerting and monitoring based on repeating critical errors.

This change allows critical operations to be retried every N seconds. Some such errors are transient and will fix themselves on retry. Other errors will have to be manually fixed by Edge operators.

Note that in a previous PR, we implemented an approach which propagated concurrent errors back to the server shutdown logic. However, this was much more complicated and introduced a paradigm to error handling that is not consistent with how Edge is implemented today.

Changes include

• Bugfix (non-breaking change that solves an issue)
• Hotfix (change that solves an urgent issue, and requires immediate attention)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)

Breaking changes

None

Checklist

• I have assigned this PR to myself
• I have added at least 1 reviewer
• I have added the relevant labels
• I have updated the official documentation
• I have added sufficient documentation in code

Testing

• I have tested this code with the official test suite
• I have tested this code manually

[sergerad]

Note that the failing TestE2E_Bridge_DepositAndWithdrawERC1155 passes on my machine. I assume its flakey due to the heuristic it uses for waiting for blocks after the deposit txs

[vcastellm]

In some cases retrying an error could make sense but in the majority of cases, the approach should be to error as soon as possible and stop the execution from progressing.

[sergerad]

In some cases retrying an error could make sense but in the majority of cases, the approach should be to error as soon as possible and stop the execution from progressing.

I initially misinterpreted this to mean stop the program - but I understand that you mean stop the goroutine that is erroring.

This approach makes it very hard to operate and debug Edge instances. We could have an instance that failed to sync rootchain events many hours ago and we would have to search for that singular ERROR log in an unknown range of time. If the sync was retried, we would see the ERROR log regularly. If we alert on every ERROR log, then we are inundated with alerts - I believe because dispatcher can ERROR log if RPC requests from users fail.

If we keep the status quo, and don't retry (or exit the program) for failures like gRPC server startup, event tracker sync, and prometheus server, what is your advice for cutting through the logs and being able to alert on ERRORs? Am I incorrect about the dispatcher logging ERROR on user requests? Thanks

Example of user-initiated RPC requests that results in ERROR logs (using eth_sendRawTransaction):

message:"2023-05-15T20:58:25.180Z [ERROR] polygon.server.dispatcher: failed to dispatch: method=eth_sendRawTransaction err=\"cannot parse RLP: cannot parse short bytes: cannot parse array value: cannot parse short bytes: cannot parse array value: cannot parse short bytes: length is not enough\""

[sergerad]

Looking at tt.Sync in ethgo https://github.com/umbracle/ethgo/blob/main/tracker/tracker.go#L578.
The RetryForever here only helps for batchsync, because the longrunning for loop after it is handled concurrently by the Sync func itself. That means the following change is useless apart from retrying the batchsync before the for loop

	go common.RetryForever(ctx, time.Second, func(context.Context) error {
		if err := tt.Sync(ctx); err != nil {
			e.logger.Error("failed to sync", "error", err)
			return err
		}
		return nil
	})

IDK why the sync func in ethgo completely ignores those errors in the for loop

[Stefan-Ethernal]

I feel that we should keep retrying logic only in event_tracker.go and in polybft.go (synchronization protocol).

As a side note, please set golangci-lint as a linting tool and run make lint command. Seems like there are a couple of linter errors which should be fixed.

[sergerad]

@Stefan-Ethernal Yep I can see the golangci-lint warnings in my IDE about line cuddling etc. I was ignoring them as I wasn't sure whether you guys really wanted to enforce those spacing requirements given the github workflow does not enforce them.

I am happy to add a workflow step that enforces the linter if you would like. Or perhaps there is a reason you do not want this. Thanks

[Stefan-Ethernal]

@Stefan-Ethernal Yep I can see the golangci-lint warnings in my IDE about line cuddilng etc. I was ignoring them as I wasn't sure whether you guys really wanted to enforce those spacing requirements given the github workflow does not enforce them.

I am happy to add a workflow step that enforces the linter if you would like. Or perhaps there is a reason you do not want this. Thanks

@sergerad In fact we do have GH workflow for linters as well, although for some reason it hasn't been triggering for external collaborators. I have created a #1512 which got merged into the develop, so if you can rebase to the latest commit on the develop branch and let's see if is it any better now (I expect it to trigger this time). 🙂 🤞

EDIT: It didn't help, so now I'm suspecting that it may be related to repository settings.

[sergerad]

Have removed the retry logic apart from polybft sync / event tracker. If we merge this PR, I will need to do a follow up PR to ethgo to address the fact that this error is not handled: https://github.com/umbracle/ethgo/blob/main/tracker/tracker.go#LL582C7-L582C22

@vcastellm are you happy doing the retries on the sync logic? Currently in this PR we are also concurrently retrying initialization logic for the trackers because the tests rely on the fact that those errors are not handled yet. If we want initialization logic out of the concurrent retries, I can update the tests in this PR.

If you are not happy doing retries on sync logic at all, I can close this PR

[Stefan-Ethernal]

LGTM.

As a side note: please rebase again to the most recent commit on the develop branch, since we have finally fixed lint workflow and then just resolve pending linting issues.

[sergerad]

As a side note: please rebase again to the most recent commit on the develop branch, since we have finally fixed lint workflow and then just resolve pending linting issues.

I did rebase when you mentioned the linting. I can see this commit in my logs

commit c3490646a75df27a36831e1a56873a5c46d01d61
Author: Stefan Negovanović <93934272+Stefan-Ethernal@users.noreply.github.com>
Date:   Tue May 16 20:43:07 2023 +0200

    Fix Lint GH workflow (#1512)
    
    * Remove ignored branches and increase timeout
    
    * Broader triggers
    
    * Add verbose output

Will rebase again now in any case

[Stefan-Ethernal]

I did rebase when you mentioned the linting. I can see this commit in my logs

Yep, but that one didn't work, so we have opened and merged another PR today (#1526) and now it's working properly 🙂

[goran-ethernal]

LGTM. Thanks for taking into consideration comments from the guys.

[vcastellm]

@vcastellm are you happy doing the retries on the sync logic?

LGTM now, thanks for the contribution and sorry for the late reply