Skip to content

Commit cb1450d

Browse files
dknopikdknopik
authored
refactor(gossipsub): Avoid indentation in handle_graft
Avoid some levels of indentation in `handle_graft` by preferring to early return/continue over having really large `if let` constructs. Pull-Request: #6526.
1 parent e8f35e1 commit cb1450d

2 files changed

Lines changed: 115 additions & 119 deletions

File tree

‎protocols/gossipsub/src/behaviour.rs‎

Lines changed: 112 additions & 116 deletions
Original file line numberDiff line numberDiff line change
@@ -1541,134 +1541,130 @@ where
15411541
// we don't GRAFT to/from explicit peers; complain loudly if this happens
15421542
if self.explicit_peers.contains(peer_id) {
15431543
tracing::warn!(peer=%peer_id, "GRAFT: ignoring request from direct peer");
1544-
// this is possibly a bug from non-reciprocal configuration; send a PRUNE for all topics
1545-
to_prune_topics = topics.into_iter().collect();
1546-
// but don't PX
1547-
do_px = false
1548-
} else {
1549-
let (below_zero, score) = self.peer_score.below_threshold(peer_id, |_| 0.0);
1550-
let now = Instant::now();
1551-
for topic_hash in topics {
1552-
if let Some(peers) = self.mesh.get_mut(&topic_hash) {
1553-
// if the peer is already in the mesh ignore the graft
1554-
if peers.contains(peer_id) {
1555-
tracing::debug!(
1556-
peer=%peer_id,
1557-
topic=%&topic_hash,
1558-
"GRAFT: Received graft for peer that is already in topic"
1559-
);
1560-
continue;
1561-
}
1544+
return;
1545+
}
15621546

1563-
// make sure we are not backing off that peer
1564-
if let Some(backoff_time) = self.backoffs.get_backoff_time(&topic_hash, peer_id)
1565-
&& backoff_time > now
1566-
{
1567-
tracing::warn!(
1568-
peer=%peer_id,
1569-
"[Penalty] Peer attempted graft within backoff time, penalizing"
1570-
);
1571-
// add behavioural penalty
1572-
if let PeerScoreState::Active(peer_score) = &mut self.peer_score {
1573-
#[cfg(feature = "metrics")]
1574-
if let Some(metrics) = self.metrics.as_mut() {
1575-
metrics.register_score_penalty(Penalty::GraftBackoff);
1576-
}
1577-
peer_score.add_penalty(peer_id, 1);
1578-
1579-
// Apply an extra graft-backoff penalty only when the peer is still
1580-
// far enough from backoff expiry.
1581-
// This compares durations only,
1582-
// avoiding Instant arithmetic and handling config edge cases
1583-
// safely: any active backoff
1584-
// qualifies for the extra penalty.
1585-
let apply_extra_penalty = match self
1586-
.config
1587-
.prune_backoff()
1588-
.checked_sub(self.config.graft_flood_threshold())
1589-
{
1590-
Some(required_remaining) => {
1591-
let remaining_backoff =
1592-
backoff_time.saturating_duration_since(now);
1593-
remaining_backoff > required_remaining
1594-
}
1595-
// graft_flood_threshold >= prune_backoff
1596-
None => true,
1597-
};
1598-
if apply_extra_penalty {
1599-
peer_score.add_penalty(peer_id, 1);
1600-
}
1601-
}
1602-
// no PX
1603-
do_px = false;
1547+
let (below_zero, score) = self.peer_score.below_threshold(peer_id, |_| 0.0);
1548+
let now = Instant::now();
1549+
for topic_hash in topics {
1550+
let Some(peers) = self.mesh.get_mut(&topic_hash) else {
1551+
// don't do PX when there is an unknown topic to avoid leaking our peers
1552+
do_px = false;
1553+
tracing::debug!(
1554+
peer=%peer_id,
1555+
topic=%topic_hash,
1556+
"GRAFT: Received graft for unknown topic from peer"
1557+
);
1558+
// spam hardening: ignore GRAFTs for unknown topics
1559+
continue;
1560+
};
16041561

1605-
to_prune_topics.insert(topic_hash.clone());
1606-
continue;
1607-
}
1562+
// if the peer is already in the mesh ignore the graft
1563+
if peers.contains(peer_id) {
1564+
tracing::debug!(
1565+
peer=%peer_id,
1566+
topic=%&topic_hash,
1567+
"GRAFT: Received graft for peer that is already in topic"
1568+
);
1569+
continue;
1570+
}
16081571

1609-
// check the score
1610-
if below_zero {
1611-
// we don't GRAFT peers with negative score
1612-
tracing::debug!(
1613-
peer=%peer_id,
1614-
%score,
1615-
topic=%topic_hash,
1616-
"GRAFT: ignoring peer with negative score"
1617-
);
1618-
// we do send them PRUNE however, because it's a matter of protocol
1619-
// correctness
1620-
to_prune_topics.insert(topic_hash.clone());
1621-
// but we won't PX to them
1622-
do_px = false;
1623-
continue;
1572+
// make sure we are not backing off that peer
1573+
if let Some(backoff_time) = self.backoffs.get_backoff_time(&topic_hash, peer_id)
1574+
&& backoff_time > now
1575+
{
1576+
tracing::warn!(
1577+
peer=%peer_id,
1578+
"[Penalty] Peer attempted graft within backoff time, penalizing"
1579+
);
1580+
// add behavioural penalty
1581+
if let PeerScoreState::Active(peer_score) = &mut self.peer_score {
1582+
#[cfg(feature = "metrics")]
1583+
if let Some(metrics) = self.metrics.as_mut() {
1584+
metrics.register_score_penalty(Penalty::GraftBackoff);
1585+
}
1586+
peer_score.add_penalty(peer_id, 1);
1587+
1588+
// Apply an extra graft-backoff penalty only when the peer is still
1589+
// far enough from backoff expiry.
1590+
// This compares durations only,
1591+
// avoiding Instant arithmetic and handling config edge cases
1592+
// safely: any active backoff
1593+
// qualifies for the extra penalty.
1594+
let apply_extra_penalty = match self
1595+
.config
1596+
.prune_backoff()
1597+
.checked_sub(self.config.graft_flood_threshold())
1598+
{
1599+
Some(required_remaining) => {
1600+
let remaining_backoff = backoff_time.saturating_duration_since(now);
1601+
remaining_backoff > required_remaining
1602+
}
1603+
// graft_flood_threshold >= prune_backoff
1604+
None => true,
1605+
};
1606+
if apply_extra_penalty {
1607+
peer_score.add_penalty(peer_id, 1);
16241608
}
1609+
}
1610+
// no PX
1611+
do_px = false;
16251612

1626-
// check mesh upper bound and only allow graft if the upper bound is not reached
1627-
let mesh_n_high = self.config.mesh_n_high_for_topic(&topic_hash);
1613+
to_prune_topics.insert(topic_hash.clone());
1614+
continue;
1615+
}
16281616

1629-
if peers.len() >= mesh_n_high {
1630-
to_prune_topics.insert(topic_hash.clone());
1631-
continue;
1632-
}
1617+
// check the score
1618+
if below_zero {
1619+
// we don't GRAFT peers with negative score
1620+
tracing::debug!(
1621+
peer=%peer_id,
1622+
%score,
1623+
topic=%topic_hash,
1624+
"GRAFT: ignoring peer with negative score"
1625+
);
1626+
// we do send them PRUNE however, because it's a matter of protocol
1627+
// correctness
1628+
to_prune_topics.insert(topic_hash.clone());
1629+
// but we won't PX to them
1630+
do_px = false;
1631+
continue;
1632+
}
16331633

1634-
// add peer to the mesh
1635-
tracing::debug!(
1636-
peer=%peer_id,
1637-
topic=%topic_hash,
1638-
"GRAFT: Mesh link added for peer in topic"
1639-
);
1634+
// check mesh upper bound and only allow graft if the upper bound is not reached
1635+
let mesh_n_high = self.config.mesh_n_high_for_topic(&topic_hash);
16401636

1641-
if peers.insert(*peer_id) {
1642-
#[cfg(feature = "metrics")]
1643-
if let Some(m) = self.metrics.as_mut() {
1644-
m.peers_included(&topic_hash, Inclusion::Subscribed, 1)
1645-
}
1646-
}
1637+
if peers.len() >= mesh_n_high {
1638+
to_prune_topics.insert(topic_hash.clone());
1639+
continue;
1640+
}
16471641

1648-
// If the peer did not previously exist in any mesh, inform the handler
1649-
peer_added_to_mesh(
1650-
*peer_id,
1651-
vec![&topic_hash],
1652-
&self.mesh,
1653-
&mut self.events,
1654-
&self.connected_peers,
1655-
);
1642+
// add peer to the mesh
1643+
tracing::debug!(
1644+
peer=%peer_id,
1645+
topic=%topic_hash,
1646+
"GRAFT: Mesh link added for peer in topic"
1647+
);
16561648

1657-
if let PeerScoreState::Active(peer_score) = &mut self.peer_score {
1658-
peer_score.graft(peer_id, topic_hash);
1659-
}
1660-
} else {
1661-
// don't do PX when there is an unknown topic to avoid leaking our peers
1662-
do_px = false;
1663-
tracing::debug!(
1664-
peer=%peer_id,
1665-
topic=%topic_hash,
1666-
"GRAFT: Received graft for unknown topic from peer"
1667-
);
1668-
// spam hardening: ignore GRAFTs for unknown topics
1669-
continue;
1649+
if peers.insert(*peer_id) {
1650+
#[cfg(feature = "metrics")]
1651+
if let Some(m) = self.metrics.as_mut() {
1652+
m.peers_included(&topic_hash, Inclusion::Subscribed, 1)
16701653
}
16711654
}
1655+
1656+
// If the peer did not previously exist in any mesh, inform the handler
1657+
peer_added_to_mesh(
1658+
*peer_id,
1659+
vec![&topic_hash],
1660+
&self.mesh,
1661+
&mut self.events,
1662+
&self.connected_peers,
1663+
);
1664+
1665+
if let PeerScoreState::Active(peer_score) = &mut self.peer_score {
1666+
peer_score.graft(peer_id, topic_hash);
1667+
}
16721668
}
16731669

16741670
if !to_prune_topics.is_empty() {

‎protocols/gossipsub/src/behaviour/tests/explicit_peers.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,9 @@ fn test_handle_graft_explicit_peer() {
144144
_ => false,
145145
}
146146
});
147-
assert!(
148-
control_msgs >= 2,
149-
"Not enough prunes sent when grafting from explicit peer"
147+
assert_eq!(
148+
control_msgs, 0,
149+
"Prunes sent when grafting from explicit peer"
150150
);
151151
}
152152

0 commit comments

Comments
 (0)