Skip to content

Conversation

@zouguangxian
Copy link
Contributor

synchronisation failure when local header is ahead of local full block.

synchronisation failure when local header is ahead of local full block.
@holiman
Copy link
Contributor

holiman commented Oct 12, 2020

Could you give some more context here? In what scenarios would this happen, and why is the fix improving the situation?

@zouguangxian
Copy link
Contributor Author

Could you give some more context here? In what scenarios would this happen, and why is the fix improving the situation?

I integrate a BFT-based consensus into go-ethereum, and launch a 3 nodes network via docker-compose.yml. After shutdown and restart the cluster, one node(A) still have the latest header and block, but the other two node(B,C) just have latest header and it's parent block. In such a situation, B and C can't sync to the latest block.

@holiman
Copy link
Contributor

holiman commented Oct 21, 2020

In your changes, did you change the rollback mechanism at sync failure? I don't understand how they can have the latest head but not have filled the header with a full block?

@holiman
Copy link
Contributor

holiman commented Oct 21, 2020

The thing about this change, is that it may possibly fix some cornercase you're experiencing with your modified version of the code, but we need to ensure that there's exactly no way that this change screws up with regular go-ethereum mainnet clients. And that's very hard to do, unless we spend a lot of time analyzing what the possible consequences are, and ensure that we have sufficient test coverage of every potential case.

So it's very unlikely (imo) that we will accept this PR, unless it actually improves something or solves a problem relevant for ethereum mainnet clients.

@zouguangxian
Copy link
Contributor Author

In your changes, did you change the rollback mechanism at sync failure? I don't understand how they can have the latest head but not have filled the header with a full block?

No, I don't change rollback mechanism at sync failure.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

lgtm

@fjl fjl changed the title fix synchronisation failure eth: fix corner case in sync head determination Dec 8, 2020
@fjl fjl removed the status:triage label Dec 15, 2020
@fjl fjl self-assigned this Dec 15, 2020
@fjl fjl added this to the 1.10.2 milestone Mar 25, 2021
@fjl fjl merged commit 9557271 into ethereum:master Mar 26, 2021
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
This avoids synchronisation failures when the local header is ahead of
the local full block.
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.

4 participants