From d92721aab91c5ad31ddfacc3cf4f55d7becab3de Mon Sep 17 00:00:00 2001 From: behzad nouri Date: Wed, 14 Apr 2021 20:18:00 +0000 Subject: [PATCH] uses timeouts based on stake for filtering pull responses (#16549) filter_pull_responses is using default timeout when discarding pull responses (except for ContactInfo): https://github.com/solana-labs/solana/blob/f804ce63c/core/src/crds_gossip_pull.rs#L349-L350 But purging code uses timeouts based on stake: https://github.com/solana-labs/solana/blob/f804ce63c/core/src/cluster_info.rs#L1867-L1870 So the crds value will not be purged from the sender's table and will be sent again over the next pull request. --- core/src/cluster_info.rs | 3 +- core/src/crds_gossip_pull.rs | 55 +++++++++++++----------------------- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/core/src/cluster_info.rs b/core/src/cluster_info.rs index b60b5240e0..be7ed79bf1 100644 --- a/core/src/cluster_info.rs +++ b/core/src/cluster_info.rs @@ -4765,7 +4765,8 @@ mod tests { }) .take(NO_ENTRIES) .collect(); - let timeouts = cluster_info.gossip.read().unwrap().make_timeouts_test(); + let mut timeouts = HashMap::new(); + timeouts.insert(Pubkey::default(), CRDS_GOSSIP_PULL_CRDS_TIMEOUT_MS * 2); assert_eq!( (0, 0, NO_ENTRIES), cluster_info.handle_pull_response(&entrypoint_pubkey, data, &timeouts) diff --git a/core/src/crds_gossip_pull.rs b/core/src/crds_gossip_pull.rs index c9ada0d5a8..639feac106 100644 --- a/core/src/crds_gossip_pull.rs +++ b/core/src/crds_gossip_pull.rs @@ -343,42 +343,27 @@ impl CrdsGossipPull { failed_inserts.push(value.value_hash) } }; - for r in responses { - let owner = r.label().pubkey(); + let default_timeout = timeouts + .get(&Pubkey::default()) + .copied() + .unwrap_or(self.msg_timeout); + for response in responses { + let owner = response.label().pubkey(); // Check if the crds value is older than the msg_timeout - if now > r.wallclock().checked_add(self.msg_timeout).unwrap_or(0) - || now + self.msg_timeout < r.wallclock() - { - match &r.label() { - CrdsValueLabel::ContactInfo(_) => { - // Check if this ContactInfo is actually too old, it's possible that it has - // stake and so might have a longer effective timeout - let timeout = *timeouts - .get(&owner) - .unwrap_or_else(|| timeouts.get(&Pubkey::default()).unwrap()); - if now > r.wallclock().checked_add(timeout).unwrap_or(0) - || now + timeout < r.wallclock() - { - stats.timeout_count += 1; - stats.failed_timeout += 1; - continue; - } - } - _ => { - // Before discarding this value, check if a ContactInfo for the owner - // exists in the table. If it doesn't, that implies that this value can be discarded - if crds.lookup(&CrdsValueLabel::ContactInfo(owner)).is_none() { - stats.timeout_count += 1; - stats.failed_timeout += 1; - } else { - // Silently insert this old value without bumping record timestamps - maybe_push(r, &mut versioned_expired_timestamp); - } - continue; - } - } + let timeout = timeouts.get(&owner).copied().unwrap_or(default_timeout); + // Before discarding this value, check if a ContactInfo for the + // owner exists in the table. If it doesn't, that implies that this + // value can be discarded + if now <= response.wallclock().saturating_add(timeout) { + maybe_push(response, &mut versioned); + } else if crds.get_contact_info(owner).is_some() { + // Silently insert this old value without bumping record + // timestamps + maybe_push(response, &mut versioned_expired_timestamp); + } else { + stats.timeout_count += 1; + stats.failed_timeout += 1; } - maybe_push(r, &mut versioned); } (versioned, versioned_expired_timestamp, failed_inserts) } @@ -1511,7 +1496,7 @@ mod test { &peer_pubkey, &timeouts, vec![peer_vote], - node.msg_timeout + 1, + node.msg_timeout + 2, ) .0, 1