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

Adjust snap request threshold #5842

Merged
merged 3 commits into from
Jun 21, 2023
Merged

Conversation

asdacap
Copy link
Contributor

@asdacap asdacap commented Jun 20, 2023

  • Adjust some request threshold for snap request queuing.
  • Reduces the amount no request to dispatch during snap sync, but sync time too inconsistent to know for sure if it affect anything.
  • Increase storage queue size before skipping account range request.
  • Check for high code queue first before dequeing not full storage queue.

Screenshot from 2023-06-20 19-14-34

Changes

  • Slight refactor.
  • Adjust threshold.

Types of changes

What types of changes does your code introduce?

  • Optimization
  • Refactoring

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

  • Definitely can sync.

@benaadams
Copy link
Member

Might want to also change this as I think only Geth currently supports Snap; rather than "not Nethermind"?

// check if peer supports snap sync
if ((contexts & AllocationContexts.Snap) != 0)
{
// TODO: Remove when Nethermind implements snap server
return peerInfo.SyncPeer.ClientType != NodeClientType.Nethermind;
}

@benaadams
Copy link
Member

Code formatting :P

@asdacap
Copy link
Contributor Author

asdacap commented Jun 21, 2023

Might want to also change this as I think only Geth currently supports Snap; rather than "not Nethermind"?

// check if peer supports snap sync
if ((contexts & AllocationContexts.Snap) != 0)
{
// TODO: Remove when Nethermind implements snap server
return peerInfo.SyncPeer.ClientType != NodeClientType.Nethermind;
}

I think we should remove this completely actually.

@LukaszRozmej
Copy link
Member

Might want to also change this as I think only Geth currently supports Snap; rather than "not Nethermind"?

// check if peer supports snap sync
if ((contexts & AllocationContexts.Snap) != 0)
{
// TODO: Remove when Nethermind implements snap server
return peerInfo.SyncPeer.ClientType != NodeClientType.Nethermind;
}

I think we should remove this completely actually.

The idea there was that Nethermind will advertise snap sync protocol when it is syncing, but we don't want to actually sync from Nethermind as it won't serve any data. This is a temporary thing until we implement snap server. Not sure if Besu has similar issue.

@asdacap
Copy link
Contributor Author

asdacap commented Jun 21, 2023

Well, yea. But if we do try to sync from nethermind, it will disconnect us, leaving some space for geth peer.

@asdacap asdacap merged commit a0bc1b3 into master Jun 21, 2023
61 checks passed
@asdacap asdacap deleted the perf/adjust-snap-request-threshold branch June 21, 2023 11:50
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.

None yet

3 participants