Implementation of gossipsub v1.1 - Hardening #263
Conversation
vyzo
requested review from
Stebalien,
raulk,
whyrusleeping,
Kubuxu and
yusefnapora
|
Summoning @yusefnapora @raulk @Stebalien @Kubuxu @whyrusleeping This is ready for review (modulo unit tests). |
| @@ -775,6 +784,12 @@ func (p *PubSub) handleIncomingRPC(rpc *RPC) { | |||
| } | |||
| } | |||
|
|
|||
| // ask the router to vet the peer before commiting any processing resources | |||
| if !p.rt.AcceptFrom(rpc.from) { | |||
| log.Warningf("received message from router graylisted peer %s. Dropping RPC", rpc.from) | |||
There was a problem hiding this comment.
should we do something if we get a bunch of these?
There was a problem hiding this comment.
we could blacklist i guess, but then we'd have to count stuff.
| go ps.background(gs.p.ctx) | ||
| } | ||
|
|
||
| func (ps *peerScore) Score(p peer.ID) float64 { |
There was a problem hiding this comment.
As far as I can tell, this gets called for every single message received. It seems to me like it may get rather resource intensive. Let's keep this in mind and observe CPU profiles during tests
There was a problem hiding this comment.
Yes, the Score method gets called a lot. It should be relatively lightweight to compute, but good point about looking at CPU profiles.
There was a problem hiding this comment.
We also should watch lock contention there.
score.go
Outdated
| } | ||
|
|
||
| // check that we have an app specific score; the weight can be anything (but expected positive) | ||
| if p.AppSpecificScore == nil { |
There was a problem hiding this comment.
Let's also check AppSpecificWeight here, or maybe even remove it altogether, as this is an app-controlled function that can regulate its own score values (in contrast with the other weights that modulate pubsub behavior).
There was a problem hiding this comment.
That's a good suggestion, we can consider removing the app specific weight and just keep the score function.
There was a problem hiding this comment.
+1 to removing AppSpecificWeight, I don't see it adding anything.
|
also merged in #268 as this fixes a tracing bug. |
|
Add a |
|
Some improvements of the backoff handling:
|
instead of trying to piggyback with outgoing messages
Co-Authored-By: Raúl Kripalani <[email protected]>
Co-Authored-By: Raúl Kripalani <[email protected]>
It was removing all old addresses, not just the ones in the new addres slice
Also, don't track loopback addrs.
|
Rebased on master for merging. |
|
YEaasasssss!!! Phenomenal work everyone!!! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This PR implements the Hardening extensions as defined by Gossipsub v1.1 Specification
What is left to do