NethermindEth / NethermindEth/pluto

Unify time mocking strategy across the codebase

Open
#306 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

testing
Dominant language
Rust
Stars
8
Forks
5
Avg merge
4d 16h
Merged PRs (30d)
37

Description

Context

Time mocking has been flagged as a recurring pain point across multiple PRs:

  • PR #18 (QBFT): Tests were flaky due to reliance on real wall-clock time. cargo nextest was introduced specifically for timeout/retry control. A custom FakeClock was built for QBFT to work around the problem. Several tests remain #[ignore]d with timing-related failures ("wrong decide round", "wrong prepared value").
  • PR #276 (Deadliner): Utc::now() is called directly throughout deadline.rs, making the module untestable with tokio::time::pause()/advance(). Tests rely on real wall-clock time with short millisecond sleeps — fragile under CI load. The PR was merged with the agreement to revisit this as a separate issue.

Current state

The codebase has three unrelated time approaches coexisting:

Approach Where Mockable?
chrono::Utc::now() / chrono::Local::now() deadline.rs, retry.rs, peerinfo, cluster/definition.rs, p2p/k1.rs No (not controlled by tokio test-util)
std::time::Instant / tokio::time::Instant peerinfo/protocol.rs, p2p/relay.rs, QBFT tests Partially (tokio variant responds to pause()/advance())
FakeClock (custom, crossbeam-based) core/qbft/fake_clock.rs Yes, but ad-hoc and only for QBFT

Additionally, some modules use injectable time functions as a workaround:

  • retry.rs has with_time() accepting a Fn() -> DateTime<Utc> provider
  • deadline.rs has DeadlineFunc for injectable deadline calculation

These are local solutions — there is no unified abstraction.

Problem

  1. chrono::Utc::now() is invisible to tokio's test-util. tokio::time::pause() only controls tokio::time::Instant and tokio::time::sleep(). Any code using chrono::Utc::now() will see real wall-clock time in tests, leading to flaky or non-deterministic behavior.
  2. FakeClock is QBFT-specific. It uses crossbeam::channel and std::time::Instant — a completely different mechanism from tokio's async timers. It can't be reused for async code that uses tokio::select!.
  3. No consistent pattern for new code to follow. Each module has invented its own approach.

Proposed direction

Explore a unified time abstraction, potentially:

  • Option A: Lean into tokio::time throughout. Replace chrono::Utc::now() with tokio::time::Instant-based calculations where possible. Use tokio::time::pause()/advance() in tests. Keep chrono only for formatting/parsing, not as a clock source.
  • Option B: Create a small Clock trait (or crate-level abstraction as suggested in #276) that wraps the time source. Production uses real time; tests inject a controllable clock. This is more flexible but adds a layer of indirection.
  • Option C: Hybrid — use tokio's built-in mocking for async timer code, and a simple injectable Fn() -> DateTime<Utc> for code that needs wall-clock timestamps.

Affected areas

Key files using chrono::Utc::now() or direct time that would benefit from unification:

  • crates/core/src/deadline.rs
  • crates/app/src/retry.rs
  • crates/peerinfo/src/protocol.rs, config.rs, behaviour.rs
  • crates/cluster/src/definition.rs
  • crates/p2p/src/k1.rs
  • crates/core/src/qbft/fake_clock.rs (custom mock)
  • crates/app/src/privkeylock.rs

References

  • #18 — QBFT implementation (flaky tests, FakeClock introduction)
  • #276 — Deadliner (time mocking discussion)
  • tokio test-util docs

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading crates/core/src/deadline.rs, crates/app/src/retry.rs, crates/core/src/qbft/fake_clock.rs, and the other affected files listed in the issue, then review PRs #18 and #276. Map the existing clock and injection patterns, evaluate the proposed alternatives, and define completion as a consistent, testable time strategy across the affected modules without the current timing flakiness.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
distributed-systems, testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.