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

fix: eth: remove trace sanity check #11385

Merged
merged 1 commit into from
Nov 17, 2023
Merged

fix: eth: remove trace sanity check #11385

merged 1 commit into from
Nov 17, 2023

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Nov 7, 2023

Proposed Changes

We added this check as a self-test to make sure that the message indices in our trace matched up with those in the block. Unfortunately, this only works for blocks on the main-chain, and breaks tracing of uncle blocks (and tracing of head).

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • CI is green

We added this check as a self-test to make sure that the message indices
in our trace matched up with those in the block. Unfortunately, this
only works for blocks on the main-chain, and breaks tracing of uncle
blocks (and tracing of head).
@Stebalien
Copy link
Member Author

Hm. Apparently we're hitting this check in production. I'll have to debug this.

@Stebalien
Copy link
Member Author

Nevermind, this code was the problem. We are attempting to lookup the parent messages by looking up the "next" epoch's tipset, then looking backwards. But, if we have a null tipset, we'd look up the wrong tipset.

So, removing this code is the correct thing to do.

Copy link

@hawkaa hawkaa left a comment

Choose a reason for hiding this comment

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

LGTM!

@fridrik01
Copy link
Contributor

@Stebalien So this failed when we were tracing a block which following tipset was a null tipset? Could we have a method for fetching the next non-null tipset for a given epoch (it may be useful in other cases) and then we could have sanity checks like this

@Stebalien
Copy link
Member Author

@fridrik01 this logic doesn't work in the presence of forks either. E.g.:

Given:

A <- B <- C     <-- C is head
 \
  \- X <- Y     <-- fork

If we try to trace X, we'll end up looking up C as the next block instead of Y.

@Stebalien Stebalien merged commit 06d288e into master Nov 17, 2023
87 checks passed
@Stebalien Stebalien deleted the steb/uncle-tracing branch November 17, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants