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.

Sign up for GitHub

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

Jump to bottom

Improve pending blocks retrieval mechanism #4227

Merged
merged 28 commits into from
Aug 12, 2022
Merged

Improve pending blocks retrieval mechanism #4227

merged 28 commits into from
Aug 12, 2022

Conversation

Gabriel-Trintinalia
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia commented Aug 8, 2022 •
edited
Loading

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

PR description

When a block is saved for future import, requesting the lowest announced block parent does not seem to be always the best option. It can be that we cannot retrieve this block and then we keep selecting the lowest block over and over and eventually it does not fix the gap. A better approach for trying to unstick could be to request the parent of the lowest pending ancestor

Changes:

  • When saving a block for future import, try to retrieve the parent of the lowest pending block connected to this block.
  • When saving a block for future import, do not try to retrieve the parent the lowest announced block.
  • Avoid requesting the same parent block several times when saving pending blocks
  • Improve logs when saving/importing a block

Fixed Issue(s)

potential aid for #3955
#4197

Documentation

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

Changelog

@Gabriel-Trintinalia
Add more log to retrieve parent method
97f21b7 
Signed-off-by: Gabriel Trintinalia <[email protected]>
@Gabriel-Trintinalia Gabriel-Trintinalia marked this pull request as draft August 8, 2022 03:22
@Gabriel-Trintinalia
Fix spotless
149ff69 
Signed-off-by: Gabriel Trintinalia <[email protected]>
@non-fungible-nelson non-fungible-nelson added the TeamRevenant GH issues worked on by Revenant Team label Aug 8, 2022
@Gabriel-Trintinalia Gabriel-Trintinalia mentioned this pull request Aug 9, 2022
Closed
@Gabriel-Trintinalia Gabriel-Trintinalia changed the title WIP: Add more log to retrieve parent method WIP: Request the parent of the lowest pending block instead of lowest announced block Aug 9, 2022
@Gabriel-Trintinalia Gabriel-Trintinalia changed the title WIP: Request the parent of the lowest pending block instead of lowest announced block Improve pending blocks retrieval mechanism Aug 9, 2022
@Gabriel-Trintinalia Gabriel-Trintinalia marked this pull request as ready for review August 9, 2022 23:42
ajsutton
ajsutton reviewed Aug 10, 2022
View reviewed changes
ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/BlockHeader.java Outdated Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java Outdated Show resolved Hide resolved
...eum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/state/PendingBlocksManager.java Outdated Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java Outdated Show resolved Hide resolved
...rc/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java Outdated Show resolved Hide resolved
fab-10
fab-10 requested changes Aug 10, 2022
View reviewed changes
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

This goes in the right directions, implementing a basic backward sync to fill gaps in the block propagation, I have reported just a couple of minor things.

During this code review, I have also reviewed the role of maybeProcessNonAnnouncedBlocks that looks forward, so in the future to try to proactively fetch new blocks, that usually should be notified by peers soon after, so not sure how much it is useful, instead this class was missing something that looks backward, that is probably much needed, not sure if the author relied on the fact that full sync eventally fills the gaps, but when full sync stalls (for yet unknown reasons) we are in throuble, and this PR should fix that!

...eum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/state/PendingBlocksManager.java Outdated Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java Outdated Show resolved Hide resolved
@Gabriel-Trintinalia
Refine tests
202a079 
Signed-off-by: Gabriel Trintinalia <[email protected]>
@Gabriel-Trintinalia
Refine tests
e5c1403 
Signed-off-by: Gabriel Trintinalia <[email protected]>
ajsutton
ajsutton approved these changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

I'm leaning on @fab-10 's review here a fair bit for the finer details of how this works but looks good to my understanding.

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java Show resolved Hide resolved
...eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/state/PendingBlocksManagerTest.java Outdated Show resolved Hide resolved
pinges
pinges approved these changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@pinges pinges left a comment

Choose a reason for hiding this comment

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

LGTM, you might want to think about getting rid of the second parameter of the pendingAncestorBlockOf() method.
Some finals missing :-)

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java
LOG.info(
"Imported {} pending blocks: {}",
r.size(),
r.stream().map(b -> b.getHeader().getNumber()).collect(Collectors.toList()));
}
if (t != null) {
LOG.error("Error importing pending blocks", t);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if there was an error? Do we need to remove a block from the pending blocks? Depending on what error was thrown?

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java Outdated Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java Outdated Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java Outdated Show resolved Hide resolved
@macfarla macfarla enabled auto-merge (squash) August 12, 2022 03:54
@Gabriel-Trintinalia
Change CHANGELOG.md
cdc6dd1 
Signed-off-by: Gabriel Trintinalia <[email protected]>
auto-merge was automatically disabled August 12, 2022 03:59

Head branch was pushed to by a user without write access

@macfarla macfarla merged commit f847ead into hyperledger:main Aug 12, 2022
@Gabriel-Trintinalia Gabriel-Trintinalia deleted the 3955-besu-stop-importing branch October 5, 2022 01:30
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@Gabriel-Trintinalia @eum602
Improve pending blocks retrieval mechanism (hyperledger#4227)
0b38563 
* Add more log to retrieve parent method
* Request the lowest pending ancestor when saving a block
* Replace recursive implementation with iterative when getting pending ancestors of Block
* Decrease scope of synchronized block to reflect only the event of adding pending block to the list
* Add fork to the chain so test is more representative

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

Signed-off-by: Gabriel Trintinalia <[email protected]>
Co-authored-by: Gabriel Trintinalia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajsutton ajsutton ajsutton approved these changes

@pinges pinges pinges approved these changes

@fab-10 fab-10 fab-10 approved these changes

Assignees
No one assigned
Labels
mainnet TeamRevenant GH issues worked on by Revenant Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Gabriel-Trintinalia @ajsutton @pinges @fab-10 @macfarla @non-fungible-nelson