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

Respond to chunk requests once (per some time interval) #3830

Open
birchmd opened this issue Jan 21, 2021 · 8 comments
Open

Respond to chunk requests once (per some time interval) #3830

birchmd opened this issue Jan 21, 2021 · 8 comments
Labels
A-chain Area: Chain, client & related A-network Area: Network T-core Team: issues relevant to the core team

Comments

@birchmd
Copy link
Contributor

birchmd commented Jan 21, 2021

Nodes will request chunk parts they are missing when they get a block which includes a chunk they do not yet have (either have their part for or have reconstructed if it is for a shard they track). These requests are re-sent relatively frequently to ensure they are delivered and responded to. However, presently nodes will always respond to these requests, even if they have responded in the past. This can lead to redundant messages being sent over the network and could potentially contribute to congestion which is slowing down the network.

In this issue we want to add logic to node such they they will only respond to a request once per some time period (which can then be tuned for performance).

@birchmd birchmd added the A-chain Area: Chain, client & related label Jan 21, 2021
@birchmd birchmd self-assigned this Jan 21, 2021
@bowenwang1996
Copy link
Collaborator

I think it may be better to do this on the network level, for all requests that require responses.

@birchmd
Copy link
Contributor Author

birchmd commented Jan 26, 2021

Update on this issue:

We tried not accepting duplicate messages at the network level, remembering which we had seen before using the sha256 hash of the message (commit). But the hashing of messages introduced more overhead than ignoring duplicates saved.

We tried storing the raw message bytes as well (commit). This seemed to improve performance a little bit, though made the memory use much higher, probably too high to be an acceptable solution for production.

We tried using xxHash instead of sha256 since it is a faster hash function, but this also didn't seem to help performance very much. However, this might still be worth moving to production as a simple DOS prevention measure.

@birchmd
Copy link
Contributor Author

birchmd commented Feb 3, 2021

Update: upgrading to the latest actix/tokio versions (see #3868 and #3869) seems to have a much greater impact on performance (may even allow sustaining 4k tps on 8 shards). The possibility of DOS attacks via flooding nodes with messages (especially since the p2p network is fully open) still exists, but that can be addressed as a general stability/security fix, rather than a sharding performance improvement. We also have an issue which is likely to reduce the amount of network traffic generated by (honest) non-validating nodes (see #3890).

Given the above, I am closing this issue.

@birchmd birchmd closed this as completed Feb 3, 2021
@bowenwang1996
Copy link
Collaborator

I don't think we should close this issue given that it is not done and we still need to do it.

@stale
Copy link

stale bot commented Jul 1, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Jul 1, 2021
@bowenwang1996 bowenwang1996 added T-core Team: issues relevant to the core team and removed S-stale labels Jul 1, 2021
@stale
Copy link

stale bot commented Sep 29, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Sep 29, 2021
@stale
Copy link

stale bot commented Dec 29, 2021

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale
Copy link

stale bot commented Mar 31, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Mar 31, 2022
@akhi3030 akhi3030 removed the S-stale label Jul 8, 2022
@walnut-the-cat walnut-the-cat added the A-network Area: Network label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-chain Area: Chain, client & related A-network Area: Network T-core Team: issues relevant to the core team
Projects
None yet
Development

No branches or pull requests

4 participants