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

Force-disconnect peers that trigger errors in the node #1250

Closed
tomaka opened this issue Sep 8, 2021 · 4 comments · Fixed by #2633
Closed

Force-disconnect peers that trigger errors in the node #1250

tomaka opened this issue Sep 8, 2021 · 4 comments · Fixed by #2633

Comments

@tomaka
Copy link
Contributor

tomaka commented Sep 8, 2021

After peer slots are implemented (#1240), it is possible for all slots to be filled with nodes that serve bad blocks or proofs.

Smoldot should instead force-disconnect these nodes in order to free slots for others.

It is unclear whether the syncing code needs to help with that, or if the sync service should do it alone.

It is also unclear whether to ban nodes for a certain duration and how to implement this ban.

@melekes
Copy link
Contributor

melekes commented Apr 29, 2022

I've contributed to a similar thing before and the design was roughly: bad behavior of a certain peer is reported to p2p service (who receives data and proxies it to other services) from other services (sync, ...). Behavior was an enum ~ { Bad, VeryBad }.

Bad -> disconnect from peer & ban it for 10min
2nd Bad -> disconnect from peer & ban it for 30min
VeryBad -> disconnect and ban forever

@tomaka
Copy link
Contributor Author

tomaka commented May 1, 2022

It is unclear whether the syncing code needs to help with that, or if the sync service should do it alone.

It is also unclear whether to ban nodes for a certain duration and how to implement this ban.

Implementing the ban on the networking part of the code should be rather easy, the problem is more about deciding which pieces of code should decide that certain nodes are to be banned.

I'm trying to not spaghettify the code with too many concepts at once.
In this context, adding the concept of "banning a node" to the syncing code might be off topic. We have this concept in the OptimisticSync already, but it is purely an implementation detail in the sense that the peer is actually ignored rather than banned.

In principle, the syncing code should simply indicate when a peer has done something wrong, and it is the higher level code that decides to ban the peer in return.

@melekes
Copy link
Contributor

melekes commented May 1, 2022

In principle, the syncing code should simply indicate when a peer has done something wrong, and it is the higher level code that decides to ban the peer in return.

Agree.

@tomaka
Copy link
Contributor Author

tomaka commented Jun 6, 2022

In the same vein, peers that refuse us (usually because they are full) should also be temporarily banned.
Right now the full node takes a long time before it starts syncing because it keeps connecting to the same nodes over and over again that keeps refusing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants