switch to upstream gossipsub #7057
Conversation
jxs
force-pushed
the
sync-gossipsub-upstream
branch
4 times, most recently
from
7e2e2cd to
600cfc5
Compare
|
Thanks for this PR! I'm checking if there are any improvements from our fork that might have been missed during upstreaming. |
| match self.gossipsub_mut().unsubscribe(&libp2p_topic) { | ||
| Err(_) => { | ||
| warn!(self.log, "Failed to unsubscribe from topic"; "topic" => %libp2p_topic); | ||
| false | ||
| } | ||
| Ok(v) => { | ||
| // Inform the network | ||
| debug!(self.log, "Unsubscribed to topic"; "topic" => %topic); | ||
| v | ||
| } | ||
| } |
There was a problem hiding this comment.
Why did we remove these logs?
There was a problem hiding this comment.
cause we no longer err on unsubscribe() it always succeeds at the Behavior level
| if let Err(e) = self.gossipsub_mut().report_message_validation_result( | ||
| self.gossipsub_mut().report_message_validation_result( | ||
| &message_id, | ||
| propagation_source, | ||
| validation_result, | ||
| ) { | ||
| warn!(self.log, "Failed to report message validation"; "message_id" => %message_id, "peer_id" => %propagation_source, "error" => ?e); | ||
| } |
There was a problem hiding this comment.
same reason, we no longer fail at this level, message validation report always succeeds
| if let Err(e) = self.gossipsub_mut().report_message_validation_result( | ||
| self.gossipsub_mut().report_message_validation_result( | ||
| &id, | ||
| &propagation_source, | ||
| MessageAcceptance::Reject, | ||
| ) { | ||
| warn!(self.log, "Failed to report message validation"; "message_id" => %id, "peer_id" => %propagation_source, "error" => ?e); | ||
| } |
jxs
force-pushed
the
sync-gossipsub-upstream
branch
from
600cfc5 to
ce2b347
Compare
|
I noticed that #6873 has not been upstreamed, but I'm not sure if we still need these metrics. cc: @pawanjay176 |
|
Thanks @ackintosh . I think one of the metrics in the PR is buggy, been meaning to fix it but haven't gotten around to it so its good that it doesn't makes it upstream. |
|
@jxs @pawanjay176 lighthouse/beacon_node/lighthouse_network/gossipsub/src/metrics.rs Lines 188 to 195 in e7ea696 |
|
@pawanjay176 @ackintosh wdyt of adding them temporarily on sigp/rust-libp2p#570 until we confirm IDONTWANTs are saving us bandwidth? |
|
That makes sense to me. |
|
This pull request has merge conflicts. Could you please resolve them @jxs? 🙏 |
Issue Addressed
We forked
gossipsubinto the lighthouse repo sometime ago so that we could iterate quicker on implementing back pressure and IDONTWANT.Meanwhile we have pushed all our changes upstream and we are now the main maintainers of
rust-libp2pthis allows us to use upstreamgossipsubagain.Nonetheless we still use our forked repo to give us freedom to experiment with features before submitting them upstream