-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Block imported below final block #2212
Comments
Hey @maoueh thanks for very detailed report, I think you found a bug in our batch header verifier. I will let you know once it's fixed |
for now, we have not used final block to limit importing block I'm sure the reorg logic is not broken by these useless imported blocks, |
Yes I don't think it's a problem for the node itself, like you mentioned, every block imported below the final block were a forked block, block which would have been discarded quickly, which I think you refer as "sidechain". However, this is a problem for our Firehose system. Indeed, once the node determine the block was final, receiving a block before causes problem later on since we cannot link back those block below the final block (because we trim any data for which We actually for now fix it in our fork by checking if the imported block is in a sidechain and kind of ignore it if not canonical.
Thanks, going to monitor this thread so let me know. |
It will be fixed in one month or three weeks |
fixed, changed the log as it tries to import the block as sidechain. |
@zzzckck @buddh0 I read the associated PR and I don't see how it fix the root problem. It appears the PR only removes the reporting that those block was added but they are still processed ultimately by the Was there other code changes prior to the PR that now prevents blocks below finalized block being imported at all? |
@maoueh The root cause is that, the previous log is a little bit misleading, it actually tries to import sidechain block, but the printed messaged did not mark the sidechain pattern.
block 35,736,658 is sidechain block, so we only change the misleading log, no other PRs |
It's "wrong" to import a side chain that hasn't any chance to become the canonical block(s). Indeed, if a side chain is imported for which My report was exactly as what I just described above. While I do agree that is not incorrect for the node to import those side chains, it's will become a problem for live tracer such what our Firehose system is doing. For a bit of context, live extended tracer is something that we contributed with the help of the Geth to the Now, those live tracer usually needs to deals with fork block. In our Firehose live tracer, we deal with forks and the handling is done by providing the finality signal to the live tracer (see https://github.com/ethereum/go-ethereum/blob/master/core/tracing/hooks.go#L112 and https://github.com/ethereum/go-ethereum/blob/master/core/tracing/hooks.go#L66). Now if the tracer receives forked block (side chain) that are actually below the last finalized block, than this is incorrect and the chain is making something that doesn't make sense. It could be argue that this would be something just for "live tracer" and I could deal with there. But I oppose the counter-argument that wasting time importing useless side chain is something is more a performance optimization that benefits anyone syncing. This is something that received some attention in the past, from some previous issue we opened last year:
This worked for a while but seems some edge cases still happen today. I hope this add the necessary details so that a correct "fix" can be implemented. Happy to jump on a call if further details is needed. |
@zzzckck @buddh0 Just checking if #2212 (comment) has been discussed internally? |
@maoueh I agree, you are right. We don't need to import side-chain blocks below finalized height. I will reopen this issue and we can fix it. |
@maoueh pls try the latest develop branch |
System information
Geth version:
geth version 1.3.8-fh2.2-f2403f09-20240131
OS & Version: Linux
Expected behaviour
Over 2 weeks, we saw a couple of occurrences where blocks were imported below the final block of the chain like if the final block was not really final or that we import known forked block(s).
This should not happen.
Actual behaviour
Here a trimmed down version of the logs highlighted to point where block get inserted below the final block:
Note
See full untrimmed logs
The
final_diff
,final_number
andfinal_hash
were added to theImported new chain segment
line with this code:Note
See https://github.com/bnb-chain/bsc/blob/v1.3.8/core/blockchain.go#L2113
And
Note
See https://github.com/bnb-chain/bsc/blob/v1.3.8/core/blockchain_insert.go#L80-L82
Steps to reproduce the behaviour
As you can see from the logs, this is not something that is happening quite often but still produces at regular interval so far. This is a 2 two weeks sample above but it's been at least 6 weeks maybe more since we started to see this problem.
We can run modified version with extra logging if you desire and extract data for you if you want.
The text was updated successfully, but these errors were encountered: