Skip to content
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

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

ImplOfAnImpl
Copy link
Contributor

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.

@ImplOfAnImpl
Copy link
Contributor Author

Rebased

Comment on lines +67 to +69
/// 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>,
Copy link
Contributor

@iljakuklic iljakuklic Jan 11, 2024

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Base automatically changed from p2p_apply_some_todos to master January 12, 2024 09:25
@ImplOfAnImpl ImplOfAnImpl merged commit 734379e into master Jan 12, 2024
11 checks passed
@ImplOfAnImpl ImplOfAnImpl deleted the p2p_dont_evict_when_blocks_in_flight branch January 12, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants