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

Merge sync downloader optimisation #4044

Closed
wants to merge 6 commits into from
Closed

Merge sync downloader optimisation #4044

wants to merge 6 commits into from

Conversation

avalonche
Copy link
Contributor

Fixes | Closes | Resolves #

Anything marked as optional that you didn't need to fill in, please remove it from the PR description. Choose one of the keywords above to refer to the issue this PR solves, followed by the issue number (e.g Fixes # 666). If there is no issue, remove the line. Remove this note after reading.

Changes:

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

Comments about testing , should you have some (optional)

Further comments (optional)

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@avalonche avalonche force-pushed the merge_sync_downloader_optimisation branch 2 times, most recently from dd4f736 to 2a3dd1a Compare May 24, 2022 14:54
Base automatically changed from kiln to master May 26, 2022 13:59
@avalonche avalonche changed the base branch from master to kiln May 29, 2022 07:51
@avalonche avalonche changed the base branch from kiln to master May 29, 2022 07:52
@avalonche avalonche force-pushed the merge_sync_downloader_optimisation branch from 95bffdc to 399f5ac Compare May 30, 2022 19:50
@avalonche avalonche marked this pull request as ready for review May 31, 2022 14:24
@avalonche avalonche changed the base branch from master to kiln May 31, 2022 15:15
@avalonche avalonche changed the base branch from kiln to master May 31, 2022 15:15
@avalonche avalonche force-pushed the merge_sync_downloader_optimisation branch from 2dc263d to ae1de29 Compare May 31, 2022 15:38
@avalonche avalonche force-pushed the merge_sync_downloader_optimisation branch 2 times, most recently from 4a25ad4 to 9c7b7be Compare June 2, 2022 16:45
MarekM25
MarekM25 requested changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

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

I think we need more tests for BeaconHeaderSync.
We should discuss changes

src/Nethermind/Nethermind.Blockchain/BlockTree.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Blockchain/BlockTree.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Blockchain/BlockTree.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Blockchain/BlockTree.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Synchronization/BeaconHeadersSyncFeed.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Synchronization/BeaconHeadersSyncFeed.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Synchronization/BeaconSync.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Synchronization/ChainLevelHelper.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Synchronization/ChainLevelHelper.cs Outdated Show resolved Hide resolved
@MarekM25
Copy link
Contributor

MarekM25 commented Jun 6, 2022

@avalonche @LukaszRozmej We need to think about the scenario when we're in archive mode and we've finished beacon headers sync. If we start adding to blockchain queue blocks, we could end up with the whole blockchain in the archive node queue. I know that we have a recovery queue. However, it might be a problem. So maybe we should limit adding to the blockchain queue (based on BlockchainProcessor.Count). WDYT?

@avalonche avalonche force-pushed the merge_sync_downloader_optimisation branch from 9c7b7be to 77d16e0 Compare June 8, 2022 06:33
asdacap
asdacap reviewed Jun 15, 2022
View reviewed changes
src/Nethermind/Nethermind.Merge.Plugin/Synchronization/ChainLevelHelper.cs Outdated
@@ -100,45 +103,28 @@ public class ChainLevelHelper : IChainLevelHelper
return headers.ToArray();
}

public Block[]? GetNextBlocks(int maxCount)
public void SetNextBlocks(int maxCount, BlockDownloadContext context)
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: Recommend the term "Populate" instead of "Set" here. Or put this method in BlockDownloadContext instead i guess.

asdacap
asdacap reviewed Jun 15, 2022
View reviewed changes
src/Nethermind/Nethermind.Merge.Plugin/Synchronization/ChainLevelHelper.cs Outdated

return blocks.ToArray();
public bool IsBeaconBlock(long blockNumber)
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ: Why not put it inside block tree?

@avalonche avalonche force-pushed the merge_sync_downloader_optimisation branch from 7579288 to 5aef836 Compare June 27, 2022 15:13
MarekM25
MarekM25 reviewed Jun 27, 2022
View reviewed changes
src/Nethermind/Nethermind.Merge.Plugin/Synchronization/MergeBlockDownloader.cs Outdated
bestPeer.HeadNumber > _blockTree.BestKnownNumber;
bestPeer.HeadNumber > currentNumber;

bool foundBeaconBlocks = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we relay on chainLevelHelper instead of bool? I mean if we're calling GetNextHeaders we should get in response if we found beacon blocks. If true we should download blocks for this headers package

MarekM25
MarekM25 requested changes Jun 27, 2022
View reviewed changes
Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

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

Added a small comments

@avalonche avalonche force-pushed the merge_sync_downloader_optimisation branch 2 times, most recently from c381b14 to 1c525ac Compare June 30, 2022 06:56
@avalonche avalonche force-pushed the merge_sync_downloader_optimisation branch from 1c525ac to 718b05e Compare June 30, 2022 07:29
MarekM25
MarekM25 reviewed Jul 4, 2022
View reviewed changes
src/Nethermind/Nethermind.Merge.Plugin/Synchronization/ChainLevelHelper.cs
if (_logger.IsTrace) _logger.Trace($"ChainLevelHelper - block {startingPoint} not found");
continue;
Block? block = _blockTree.FindBlock(hashesToRequest[i], BlockTreeLookupOptions.None);
if (block == null) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we return false here?

@MarekM25 MarekM25 mentioned this pull request Jul 7, 2022
Merged
@MarekM25 MarekM25 closed this Jul 17, 2022
@rubo rubo deleted the merge_sync_downloader_optimisation branch December 20, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarekM25 MarekM25 MarekM25 requested changes

@asdacap asdacap asdacap approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@avalonche @MarekM25 @asdacap