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

Batching backward sync #3532

Merged
merged 37 commits into from
Mar 18, 2022
Merged

Batching backward sync #3532

merged 37 commits into from
Mar 18, 2022

Conversation

gezero
Copy link
Contributor

@gezero gezero commented Mar 7, 2022

PR description

This PR is making changing backward sync to use the batching mechanism.

macfarla
macfarla reviewed Mar 9, 2022
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

added some comments mostly about logging messages

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/ForwardSyncStep.java Outdated Show resolved Hide resolved
...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/ForwardSyncStep.java Outdated Show resolved Hide resolved
.../eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStep.java Outdated Show resolved Hide resolved
@gezero gezero force-pushed the batching-backward-sync branch 4 times, most recently from f3d85ad to f41df41 Compare March 11, 2022 19:27
matkt
matkt previously requested changes Mar 12, 2022
View reviewed changes
Copy link
Contributor

@matkt matkt left a comment

Choose a reason for hiding this comment

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

Added some comment

...main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncLookupService.java Show resolved Hide resolved
.../eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardSyncStep.java Outdated Show resolved Hide resolved
.../src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncContext.java Outdated Show resolved Hide resolved
...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/ForwardSyncStep.java Outdated Show resolved Hide resolved
...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/ForwardSyncStep.java Outdated Show resolved Hide resolved
...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/ForwardSyncStep.java Outdated Show resolved Hide resolved
@gezero gezero force-pushed the batching-backward-sync branch 7 times, most recently from 3034f51 to 2e8fb73 Compare March 16, 2022 15:20
@gezero
Backward sync now batches requests
224f915 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
backward step seems to be working
6cadd81 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Forward sync step seems to be batching properly now
2a8ed2e 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Batch size is now part of BackwardSyncStep
5e98360 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Cleanup finalizing of future sync
4cea531 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Reducing the backward sync message mess
fa1371e 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
do not clear the syncTarget
735786e 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
more efficient check of successors for possible long runs
7a59349 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
backwardsSync execution should be outside exceptionally
2eba6fb 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Preparing for restart of a backward sync when problem is recoverable
ec656c0 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
small change
325864f 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Fix the renaming of methods
1571c67 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Backward Sync now syncs in batches
55ae913 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Properly finishing the backward sync
51be1f4 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Waiting for TTD before Backward sync starts
8ace582 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
spotless
0f9e9c2 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Fixing tests
38adb27 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
clearing db when Backward sync finishes
005bc01 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Reducing batch size when current batch fails
3bb8ffa 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Out of bound exception fix
67dd0ae 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Out of bound exception
995aa93 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Fixing test
64a0848 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Addressing review comments
6d2565a 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Removed javadoc
f0111f2 
Signed-off-by: Jiri Peinlich <[email protected]>
@gezero
Storage mechanism of backward sync improvement
aa083e6 
Signed-off-by: Jiri Peinlich <[email protected]>
garyschulte
garyschulte requested changes Mar 18, 2022
View reviewed changes
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.

some minor feedback, will push a PR to address

...merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java Outdated Show resolved Hide resolved
.../src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardsSyncContext.java Outdated Show resolved Hide resolved
.../src/main/java/org/hyperledger/besu/ethereum/storage/keyvalue/KeyValueSegmentIdentifier.java Show resolved Hide resolved
...hyperledger/besu/ethereum/eth/manager/task/RetryingGetHeadersEndingAtFromPeerByHashTask.java Show resolved Hide resolved
...eum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BackwardChain.java Outdated Show resolved Hide resolved
...n/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/GenericKeyValueStorageFacade.java Outdated Show resolved Hide resolved
.../eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/ForwardSyncPhase.java Outdated Show resolved Hide resolved
.../eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/ForwardSyncPhase.java Outdated Show resolved Hide resolved
.../eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/ForwardSyncPhase.java
.getBlockchain()
.getBlockByHash(block.getHeader().getParentHash());
if (parent.isEmpty()) {
batchSize = batchSize / 2 + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a little opaque. Why are we changing batchSize if we encounter a missing parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that very occasionally when I request too many bodies at the same time, some peers do not give me back all the bodies back. I ended up in an endless loop.

  1. Give me 200 bodies
  2. Peer gives back bodies without one
  3. I import all blocks until the one that was not given back.
  4. Repeat 1,2 forever because the one not given back is always the same first block now.

I noticed that reducing the batch size (to eventually ask just for 1 body) resolved that issue for me. I am happy to apply a different tactic.

...m/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/backwardsync/BlocksConvertor.java Outdated Show resolved Hide resolved
garyschulte
garyschulte approved these changes Mar 18, 2022
View reviewed changes
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.

left a few comments/questions open, but ✅

This was referenced Mar 18, 2022
Closed
Closed
@gezero
Improving coverage
fec1462 
Signed-off-by: Jiri Peinlich <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Mar 18, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

80.7% 80.7% Coverage
0.0% 0.0% Duplication

@garyschulte garyschulte mentioned this pull request Mar 18, 2022
Closed
@garyschulte garyschulte dismissed matkt’s stale review March 18, 2022 17:01

to be addressed by #3614

@garyschulte garyschulte merged commit 0fce76c into hyperledger:main Mar 18, 2022
garyschulte pushed a commit to garyschulte/besu that referenced this pull request May 2, 2022
@gezero @garyschulte
Batching backward sync (hyperledger#3532)
15557be 
* Backward sync now batches requests

Signed-off-by: Jiri Peinlich <[email protected]>
@macfarla macfarla mentioned this pull request May 25, 2022
Merged
2 tasks
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@gezero @eum602
Batching backward sync (hyperledger#3532)
5f44836 
* Backward sync now batches requests

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

@macfarla macfarla macfarla left review comments

@garyschulte garyschulte garyschulte approved these changes

@matkt matkt matkt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gezero @garyschulte @macfarla @matkt