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

Add mechanism to retrieve missing blocks #4175

Conversation

Gabriel-Trintinalia
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia commented Jul 27, 2022

Signed-off-by: Gabriel Trintinalia [email protected]

PR description

Add mechanism to retrieve missing blocks when a pending block is saved

Fixed Issue(s)

#3955

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

…Manager to get stuck on pending blocks

Signed-off-by: Gabriel Trintinalia <[email protected]>
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

Good test coverage 👍

@macfarla macfarla enabled auto-merge (squash) July 27, 2022 03:50
@macfarla macfarla merged commit f05b45d into hyperledger:main Jul 27, 2022
@fab-10
Copy link
Contributor

fab-10 commented Jul 27, 2022

It is worth to know that the BlockPropagationManager runs concurrently with full sync, and full sync should be able to recover from situations where a parent block was missing, so it could be that the real cause of the issue is related to full sync, in the way the sync target is selected, rather that the BlockPropagationManager.
Anyway this could be a fallback mechanism to workaround full sync issues, and if it works well and since this block propagation is not needed after the merge, putting more effort on fixing full sync could not be worth.

@fab-10
Copy link
Contributor

fab-10 commented Jul 27, 2022

What happens in case more than one parent block is missing?

@matkt
Copy link
Contributor

matkt commented Jul 27, 2022

It is worth to know that the BlockPropagationManager runs concurrently with full sync, and full sync should be able to recover from situations where a parent block was missing, so it could be that the real cause of the issue is related to full sync, in the way the sync target is selected, rather that the BlockPropagationManager. Anyway this could be a fallback mechanism to workaround full sync issues, and if it works well and since this block propagation is not needed after the merge, putting more effort on fixing full sync could not be worth.

the fullsync only starts when we are too far from the head. so indeed it can be strange that the fullsync does not trigger after a certain time. but at the same time I also prefer this fix because we don't have to wait to be too far to fix the problem

@fab-10
Copy link
Contributor

fab-10 commented Jul 27, 2022

Double checked with @matkt, and full sync and BlockPropagationManager always run concurrently.

@Gabriel-Trintinalia
Copy link
Contributor Author

What happens in case more than one parent block is missing?

So if block +3 arrives and we do not have +1 and +2:

  1. we will request +3's parent, which is +2.
  2. Whenever +2 arrives, it would be saved for future and then request +1.
  3. When +1 arrives, it is imported, then its children (+2) and then +2's children (+3)

@Gabriel-Trintinalia Gabriel-Trintinalia deleted the 3955-fix-pending-blocks-stuckness branch July 28, 2022 00:06
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
* Add mechanism to retrieve missing blocks that caused BlockPropagationManager to get stuck on pending blocks
* Remove threshold and request the lowest block parent everytime a block is saved for future

Signed-off-by: Gabriel Trintinalia <[email protected]>

Co-authored-by: Gabriel Trintinalia <[email protected]>
Co-authored-by: garyschulte <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
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.

6 participants