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

borheimdall: only fetch next span when in last sprint of current span #9096

Merged
merged 14 commits into from
Dec 28, 2023

Conversation

taratorio
Copy link
Member

Heimdall prepares the next span a number of sprints before the current span ends. Currently we always fetch the next span regardless of which sprint we are in during the current span. This causes a liveness issue due to how the Heimdall client works (it infinitely retries until it fetches a span - this issue will be fixed in a separate PR). This PR fixes this by matching what bor does - it fetches the next span only in the last sprint of the current span.

Changes:

  • Adds a unit test for the above
  • Adds a new function BlockInLastSprintOfSpan
  • Some code reorg and cleanup - moves the span num related functions from the bor package to the span sub package for better logical grouping

Copy link
Contributor

@mh0lt mh0lt left a comment

Choose a reason for hiding this comment

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

One style comment I think we should discuss.

In the codebase both ID and Id are used to refer to span indentifiers

My preference would be to use Id, as ID is not used across the erigon codebase in general.

@mh0lt mh0lt merged commit 1f237c0 into devel Dec 28, 2023
7 checks passed
@mh0lt mh0lt deleted the next-span-fetch-fix branch December 28, 2023 15:52
@taratorio
Copy link
Member Author

taratorio commented Dec 28, 2023

One style comment I think we should discuss.

In the codebase both ID and Id are used to refer to span indentifiers

My preference would be to use Id, as ID is not used across the erigon codebase in general.

ok, happy with any of the 3 options - we would need to chat with @battlmonstr too
in the meantime I do agree span id is used in majority of places instead of span num so ill change it to using id to avoid creating a naming mess - here's the PR #9097

mh0lt pushed a commit that referenced this pull request Dec 28, 2023
follow up on naming as suggested here
#9096 (review)
taratorio added a commit that referenced this pull request Dec 28, 2023
Corresponds to the client fix in this PR description -
#9096 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants