#35820 refactor: keep duration calculations typed

ReviewabilityReviewsAgreementSize
#35820 l0rinc refactor: keep duration calculations typedReady0Strong77 + 33 tests
  • Prevent unit mismatches and mixing unrelated numbers by using typed durations across core subsystems
  • Helps developers and reviewers catch duration calculation errors at compile time
  • Ready to review
  • Concept approval for better clarity and readability in duration calculations (janb84)
  • Objection about churn and lack of type safety resolved by rework (stickies-v)
  • Objection about time guideline violations and noisy conversions resolved (maflcko)
  • 77 lines added or modified outside tests
  • 33 lines added or modified in tests
  • 101 lines removed in total
  • 27 files, 13 commits
Uncategorized15 PRs, unranked
6 rejected
  • Kernel (libbitcoinkernel): not a member. Touches src/kernel/chainparams.cpp only for duration literal refactoring without altering the kernel C/C++ API or library boundary.
  • Mempool and policy: not a member. Touches src/txmempool.h and src/txmempool.cpp only to use typed duration arithmetic for decay calculation.
  • P2P: not a member. Touches net_processing.cpp, net.cpp, banman.h, and headerssync.cpp only to use std::chrono durations.
  • RPC / REST / ZMQ: not a member. Touches src/rpc/node.cpp and src/rpc/blockchain.cpp to convert internal time arithmetic to std::chrono.
  • Validation: not a member. Touches src/validation.cpp, src/chain.h, and src/node/chainstate.cpp without changing consensus or validation rules.
  • Wallet: not a member. Touches src/wallet/spend.cpp and src/wallet/wallet.cpp for minor duration arithmetic adjustments.
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Replaces raw integer time constants and arithmetic with typed `std::chrono` durations and literals across chain parameters, block validation, mempool decay, P2P relay limits, RPCs, and GUI code. Subsystem interfaces that still require raw integers convert durations at the boundary using explicit division (e.g. `duration / 1s`). Unit and functional characterization tests are added to verify proof-of-work parameters and the assumevalid threshold remain unchanged.

Problem: Multiple subsystems represent time intervals and durations as raw integers (usually seconds), leaving units implicit and allowing differing time units or unrelated integer values to be mixed without compiler type checks.

Evidence of importance:

  • PR description: 'Several fixed-duration calculations use raw integer time counts, leaving their units implicit and allowing unrelated integer values to be mixed.'
  • Author-only refactoring; characterization tests in `pow_tests.cpp`, `chain_tests.cpp`, `mempool_tests.cpp`, and `feature_assumevalid.py` corroborate that values remain identical.

Scope: Touches 27 files spanning consensus parameters, validation, mempool, P2P networking, RPC, GUI, and wallet code in a mechanical refactoring.

Risk of a mistake: medium (consensus, validation, p2p, mempool_policy, rpc_api, gui). A typo or unit mismatch in block header timestamp validation or consensus parameters could cause nodes to reject valid blocks or split from consensus.

Changed since the previous assessment: None apparent; the patch contents, scope, and characterization test approach match the previous description.

Reviewability

Ready

Nothing known that would invalidate a review now.

Agreement

Strong

substantive support, no open objection (janb84)

Thread notes: Initial read. Both stickies-v and maflcko raised concerns about code noise and guideline violations with TicksSeconds, which the author resolved by reworking the commits to keep types native where supported and reverting noisier conversions.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
stickies-v (member)objection, usefulnesschurns readable raw duration expressions without adding real type safety or clarityfixed · fix pushed · v1, not yet re-checkedansweredyes2026-07-27: 'for the most part I don't see the point as this doesn't add any actual type safety or clarity'
Status from: 2026-07-28: l0rinc: 'The branch is now split by risk, with exact-value coverage for the consensus-sensitive changes.'
maflcko (member)objection, approachviolates util/time.h guidelines against using TicksSeconds in application codefixed · fix pushed · v1, not yet re-checkedchangedyes2026-07-28: 'This seems like a step in the wrong direction. The docstring of TicksSeconds also says that it should be avoided in code.'
Status from: 2026-07-29: l0rinc: 'I reworked the branch so durations remain typed where the surrounding APIs already support chrono, and I like it a lot better this way.'
maflcko (member)objection, stylereplaces simple raw integer literals with noisy chrono conversions and casts without adding compile-time checksfixed · fix pushed · v1, not yet re-checkedansweredyes2026-07-29: 'A plain 300 /* ms */ seems more readable and direct than a count_milliseconds(300ms) or the alternative used in other parts of this pull: 300ms / 1ms.'
Status from: 2026-07-29: l0rinc: 'Sure, I’ve reverted the ones that were arguably noisier than the original code. Thanks for the pushback!'

Support:

  • janb84 (member) (Concept ACK, 2026-07-27): I like the clarity / readability this changes gives. It will even provide less proficient in C++ users to read consensus code.

Participants:

  • janb84 (member, support, 4 statements 2026-07-27 to 2026-07-27): Concept ACK; appreciates readability and clarity benefits of typed durations
  • stickies-v (member, objection, 1 statements 2026-07-27 to 2026-07-27): questioned whether wrapping raw durations in chrono literals and TicksSeconds added genuine type safety or clarity
  • maflcko (member, objection, 4 statements 2026-07-28 to 2026-07-29): pointed out TicksSeconds violates util/time.h guidelines and objected to noisy conversions in Qt code

Files

49 lines under test/bench/ci.

  • src/qt/guiutil.cpp +15/-30
  • test/functional/feature_assumevalid.py +19/-9
  • src/kernel/chainparams.cpp +11/-10
  • src/net_processing.cpp +9/-9
  • src/test/pow_tests.cpp +10/-6
  • src/qt/rpcconsole.cpp +6/-6
  • src/rpc/node.cpp +6/-6
  • src/qt/bitcoingui.cpp +3/-2
  • src/qt/intro.cpp +3/-2
  • src/validation.cpp +2/-3
  • src/chain.h +2/-2
  • src/headerssync.cpp +2/-2
  • src/net.cpp +2/-2
  • src/qt/rpcconsole.h +3/-1
  • src/banman.h +2/-1
  • src/txmempool.h +2/-1
  • src/wallet/spend.cpp +2/-1
  • src/node/chainstate.cpp +1/-1
  • src/qt/guiutil.h +1/-1
  • src/qt/modaloverlay.cpp +1/-1
  • src/qt/sendcoinsdialog.cpp +1/-1
  • src/rpc/blockchain.cpp +1/-1
  • src/test/chain_tests.cpp +2/-0
  • src/test/headers_sync_chainwork_tests.cpp +1/-1
  • src/txmempool.cpp +1/-1
  • src/wallet/wallet.cpp +1/-1
  • src/test/mempool_tests.cpp +1/-0

Card

The short text the ranking pass compares across PRs.

Replaces raw integer duration constants and calculations across consensus parameters, block validation, mempool decay, P2P limits, RPCs, and the GUI with typed std::chrono durations and literals. The change eliminates implicit time units to prevent unit confusion bugs while preserving underlying values with explicit unit division where integer interfaces remain. Characterization tests are added to verify consensus PoW timing and the assumevalid work threshold remain exact. This is an internal refactoring without stated user-facing behavior changes or linked upstream issues.

Data

dossier JSON · extract JSON · model ledger, generated 2026-09-25T01:22, confidence high, input hash 274b5e62a3b548bf
thread read through 2026-08-18 (14 statements), head 261fff951c
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: Refactoring