-
Notifications
You must be signed in to change notification settings - Fork 950
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
protocols/kad: Fix right shift overflow panic in record_received #1492
Changes from 5 commits
e6a57ba
aaaec36
a3271c2
b08f509
9e27e25
36f6b93
ccdf994
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -946,8 +946,6 @@ where | |||||
return | ||||||
} | ||||||
|
||||||
let now = Instant::now(); | ||||||
|
||||||
// Calculate the expiration exponentially inversely proportional to the | ||||||
// number of nodes between the local node and the closest node to the key | ||||||
// (beyond the replication factor). This ensures avoiding over-caching | ||||||
|
@@ -956,9 +954,7 @@ where | |||||
let num_between = self.kbuckets.count_nodes_between(&target); | ||||||
let k = self.queries.config().replication_factor.get(); | ||||||
let num_beyond_k = (usize::max(k, num_between) - k) as u32; | ||||||
let expiration = self.record_ttl.map(|ttl| | ||||||
now + Duration::from_secs(ttl.as_secs() >> num_beyond_k) | ||||||
); | ||||||
let expiration = exp_decr_expiration(self.record_ttl, num_beyond_k); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// The smaller TTL prevails. Only if neither TTL is set is the record | ||||||
// stored "forever". | ||||||
record.expires = record.expires.or(expiration).min(expiration); | ||||||
|
@@ -1030,6 +1026,15 @@ where | |||||
} | ||||||
} | ||||||
|
||||||
/// Calculate exponentially decreasing expiration from a default time-to-live by a factor. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
fn exp_decr_expiration(default_ttl: Option<Duration>, factor: u32) -> Option<Instant> { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
default_ttl.map(|ttl| Instant::now() + Duration::from_secs( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
ttl.as_secs() | ||||||
.checked_shr(factor) | ||||||
.unwrap_or(0), | ||||||
)) | ||||||
} | ||||||
|
||||||
impl<TStore> NetworkBehaviour for Kademlia<TStore> | ||||||
where | ||||||
for<'a> TStore: RecordStore<'a>, | ||||||
|
@@ -1911,4 +1916,3 @@ impl QueryInfo { | |||||
} | ||||||
} | ||||||
} | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is preferable to leave this here as it can be reused multiple times (cf. #1496). And since
Instant::now()
is a side-effecting function for obtaining some notion of system time, usually both testability and performance is improved by using it mostly in top-level functions and passing the instant down to others where needed (see also the other comments).