-
Notifications
You must be signed in to change notification settings - Fork 29
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
Don't evict peers that have a block in-flight #1438
Conversation
609d5dc
to
5463b03
Compare
7d160f5
to
45057f3
Compare
Rebased |
/// The time since which we've been expecting a block from the peer; if None, we're not | ||
/// expecting any blocks from it. | ||
expecting_blocks_since: Option<Time>, |
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 don't think using Time
and the time getter machinery is a good match for this.
Eventually, the time returned by the time getter will be subject to correction from external sources (NTP servers and/or other peers). That will distort the delays and timeouts. In this case, I'd say just use Instant::now()
to record the time block request was sent, without involving time_getter
.
As a rule of thumb:
- For absolute time, use
time_getter
, e.g.- getting timestamps to include in blocks
- For relative time, use
Instant
+Duration
directly, e.g.- Measuring time elapsed between two points
- Waiting for a specified amount of time
The two should not be mixed up. One difficulty is to keep the two in sync during mocking. For the former, we have custom mock time getter. For the latter, there's tokio::time::advance
. Ideally, there should be an abstraction to keep the two mocks in sync during testing. However, due to external corrections mentioned above, the two are not necessarily in sync at all times.
I would argue our time abstraction API should provide both a monotonic clock/timer and wall clock time. The difference being that the former provides a way to measure relative time without distortions while the latter is a subject to time synchronisation adjustments.
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'm hoping the corrections we do will maintain the monotonicity of the clock... otherwise many things can break.
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've created an issue for this, as we've discussed - #1443
Peers from which a block was requested recently ("recently" meaning within the last 5 seconds) are now treated as if they've sent us a new tip just now; this way, peers can avoid eviction while they are sending us blocks.