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

Reorg depth counter is incorrect #5136

Closed
philknows opened this issue Feb 12, 2023 · 4 comments · Fixed by #5160
Closed

Reorg depth counter is incorrect #5136

philknows opened this issue Feb 12, 2023 · 4 comments · Fixed by #5160
Labels
meta-discussion Indicates a topic that requires input from various developers. scope-metrics All issues with regards to the exposed metrics.
Milestone

Comments

@philknows
Copy link
Member

Describe the bug

Recent reorgs are not properly being shown in our metrics. @wemeetagain investigated why our metrics cannot properly show and calculate reorgs and their depths. Reorg depth is currently calculated as newHead.slot - commonAncestor.slot

Expected behavior

The reorg depth counter should be able to determine reorgs and at what depth. We need to tweak the calculation with something else. @wemeetagain suggests maybe Math.max(newHead.slot, oldHead.slot) - commonAncestor.slot

Open for discussion/solutions.

@philknows philknows added meta-discussion Indicates a topic that requires input from various developers. scope-metrics All issues with regards to the exposed metrics. labels Feb 12, 2023
@philknows philknows added this to the v1.5.0 milestone Feb 12, 2023
@maschad
Copy link
Contributor

maschad commented Feb 15, 2023

In the case of a reorg wouldn't the newHead.slot always be greater than the oldHead.slot ? I understand the reorg depth as the difference between the new chain and the old chain so therefore:

(newHead.slot - commonAncestor.slot) - (oldHead.slot - commonAncestor.slot)

@wemeetagain
Copy link
Member

Not necessarily.

We had a recent case like so:

  • block xxx01 is processed and becomes head
  • block xxx03 thru xxx24 is processed and becomes head
  • block xxx02 is processed and becomes head

In this case our reorg depth counter registered a reorg of depth 1, even though the reorg went from 24 back to 2

@wemeetagain
Copy link
Member

Other strange case, in deep reorgs, the shufflings are different, and there can be multiple blocks at the same slot.

@maschad
Copy link
Contributor

maschad commented Feb 16, 2023

Using Prysm's calculation it seems like the find the max of the two differences between the new slot, the old slot and the common ancestor slot.

so essentially

Math.max((newHead.slot - commonAncestor.slot), (oldHead.slot - commonAncestor.slot))

maschad added a commit to maschad/lodestar that referenced this issue Feb 16, 2023
wemeetagain added a commit that referenced this issue Feb 16, 2023
* Fix reorg depth count to be the diff of the max of new & old head slots and the common ancestor slot (#5136)

* doc: Updated tsdocs in comments for getCommonAncestorDepth function (#5136)

* Remove stale comments

---------

Co-authored-by: Cayman <caymannava@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-discussion Indicates a topic that requires input from various developers. scope-metrics All issues with regards to the exposed metrics.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants