Include Heimdall Timeout

[lucca30]

Description

Implementation for #1455 . Including configurable timeout for Heimdall. You can pass it via flag or via config.toml, like in the example below:

[heimdall]
  url = "http://localhost:1317"
  timeout = "30s" # <----------here
  "bor.without" = false

Changes

• 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)
• Changes only for a subset of nodes

Checklist

• I have added at least 2 reviewer or the whole pos-v1 team
• I have added sufficient documentation in code
• I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply
• Created a task in Jira and informed the team for implementation in Erigon client (if applicable)
• Includes RPC methods changes, and the Notion documentation has been updated

Testing

• I have added unit tests
• I have added tests to CI
• I have tested this code manually on local environment
• I have tested this code manually on remote devnet using express-cli
• I have tested this code manually on mumbai/amoy
• I have created new e2e tests into express-cli

Manual tests

Please complete this section with the steps you performed if you ran manual tests for this functionality, otherwise delete it

Additional comments

Please post additional comments in this section if you have them, otherwise delete it

[manav2401]

Might be worth calling ticker.Reset() here because if heimdall api timeout is >5s, the ticker will immediately get triggered when we enter loop again defeating it's purpose.

bor/consensus/bor/heimdall/client.go

Lines 325 to 331 in 3154fc4

	if err != nil {
	if attempt%logEach == 0 {
	log.Warn("an error while trying fetching from Heimdall", "path", url.Path, "attempt", attempt, "error", err)
	}
	continue retryLoop
	}

Or maybe another idea is to set ticker retry timeout to same as heimdall timeout?

[manav2401]

Few minor nits.

[lucca30]

Might be worth calling ticker.Reset() here because if heimdall api timeout is >5s, the ticker will immediately get triggered when we enter loop again defeating it's purpose.

bor/consensus/bor/heimdall/client.go

Lines 325 to 331 in 3154fc4

	if err != nil {
	if attempt%logEach == 0 {
	log.Warn("an error while trying fetching from Heimdall", "path", url.Path, "attempt", attempt, "error", err)
	}
	continue retryLoop
	}

Or maybe another idea is to set ticker retry timeout to same as heimdall timeout?

@manav2401 , I like the idea of setting the ticker retry to the same as heimdall. Because it holds the behavior the user previously configured.

[lucca30]

@manav2401
For reference all comments were solved in this commit: 844603e

[manav2401]

One minor nit. Except that, good to go!

[lucca30]

One minor nit. Except that, good to go!

Thanks for the clarification. Fixed on 5f6e2d3