refactor: keep duration calculations typed #35820

l0rinc commented at 5:06 AM on July 27, 2026: contributor

Problem: Several fixed-duration calculations use raw integer time counts, leaving their units implicit and allowing unrelated integer values to be mixed.

Fix: Use std::chrono types through calculations where the surrounding APIs already support them, including GUI formatting, peer bans, scheduler forwarding, block timestamps, and target-spacing arithmetic. Where carrying chrono through an integer interface would broaden the change or make the result harder to read, this PR leaves the expression unchanged or keeps the conversion simply duration / 1s. The remaining integer interfaces receive the same values, and characterization tests pin the PoW timing and inclusive assumevalid threshold.

DrahtBot added the label Refactoring on Jul 27, 2026

DrahtBot commented at 5:07 AM on July 27, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35820.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	janb84

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35351 (net: Disallow invalid HeadersSyncState due to lagging clock by hodlinator)
#26022 (Add util::ResultPtr class by ryanofsky)
#25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/qt/guiutil.cpp:788 in 7bb44a4b75

 787 | -    const int WEEK_IN_SECONDS = 7*24*60*60;
 788 | -    const int YEAR_IN_SECONDS = 31556952; // Average length of year in Gregorian calendar
 789 | +    constexpr int HOUR_IN_SECONDS{TicksSeconds(1h)};
 790 | +    constexpr int DAY_IN_SECONDS{TicksSeconds(24h)};
 791 | +    constexpr int WEEK_IN_SECONDS{TicksSeconds(7 * 24h)};
 792 | +    constexpr int YEAR_IN_SECONDS{TicksSeconds(std::chrono::years{1})};

janb84 commented at 12:18 PM on July 27, 2026:

Just to unify this section I would opt for using std::chrono for every line. (not Blocking bike-shedding NIT)

    constexpr int HOUR_IN_SECONDS{TicksSeconds(std::chrono::hours{1})};
    constexpr int DAY_IN_SECONDS{TicksSeconds(std::chrono::days{1})};
    constexpr int WEEK_IN_SECONDS{TicksSeconds(std::chrono::weeks{1})};
    constexpr int YEAR_IN_SECONDS{TicksSeconds(std::chrono::years{1})};

in src/net_processing.cpp:118 in 7bb44a4b75

 113 | @@ -114,11 +114,11 @@ static constexpr auto MINIMUM_CONNECT_TIME{30s};
 114 |  /** SHA256("main address relay")[0:8] */
 115 |  static constexpr uint64_t RANDOMIZER_ID_ADDRESS_RELAY = 0x3cac0035b5866b90ULL;
 116 |  /// Age after which a stale block will no longer be served if requested as
 117 | -/// protection against fingerprinting. Set to one month, denominated in seconds.
 118 | -static constexpr int STALE_RELAY_AGE_LIMIT = 30 * 24 * 60 * 60;
 119 | +/// protection against fingerprinting. Set to one month.
 120 | +static constexpr int STALE_RELAY_AGE_LIMIT{TicksSeconds(30 * 24h)};

janb84 commented at 12:33 PM on July 27, 2026:

NIT: why not go one step further for even more clarity ?

static constexpr int STALE_RELAY_AGE_LIMIT{TicksSeconds(std::chrono::days{30})};

Note: not to use std::chrono::month{1};

in src/net_processing.cpp:121 in 7bb44a4b75

 120 | +static constexpr int STALE_RELAY_AGE_LIMIT{TicksSeconds(30 * 24h)};
 121 |  /// Age after which a block is considered historical for purposes of rate
 122 | -/// limiting block relay. Set to one week, denominated in seconds.
 123 | -static constexpr int HISTORICAL_BLOCK_AGE = 7 * 24 * 60 * 60;
 124 | +/// limiting block relay. Set to one week.
 125 | +static constexpr int HISTORICAL_BLOCK_AGE{TicksSeconds(7 * 24h)};

janb84 commented at 12:34 PM on July 27, 2026:

static constexpr int HISTORICAL_BLOCK_AGE{TicksSeconds(std::chrono::weeks{1})};

janb84 commented at 12:38 PM on July 27, 2026: contributor

Concept ACK 7bb44a4b757c1d84091cd5518b5fa2abeb90168c

I like the clarity / readability this changes gives. It will even provide less proficient in C++ users to read consensus code.

Maybe split commit in kernel/consensus touching and the rest. In that way it's easy to ACK / review the consensus relevant changes.

Some NIT suggestions to use std::chrono::<<duration>> can be done on more places but I have limited the NITS to hear the current thinking first :)

stickies-v commented at 1:07 PM on July 27, 2026: contributor

~0, some minor clarity improvements here and there but for the most part I don't see the point as this doesn't add any actual type safety or clarity. I don't find 14 * 24 * 60 * 60; // two weeks harder to read than TicksSeconds(2 * 7 * 24h);

l0rinc force-pushed on Jul 28, 2026

l0rinc commented at 1:36 AM on July 28, 2026: contributor

Thanks for the review and feedback.

I agree that 14 * 24 * 60 * 60; // two weeks is readable in isolation - the main benefit is moving units from comments into expressions and (where possible), into types.

E.g. MIN_BLOCKS_TO_KEEP - 3600 / nPowTargetSpacing needed a (1 hour) comment (proving that others didn't consider them trivial), while MIN_BLOCKS_TO_KEEP - 1h / PowTargetSpacing() is self-describing and keeps both operands typed.

Likewise, 8 * 60 * 60; // in seconds becomes TicksSeconds(8h), making the conversion and units explicit.

The branch is now split by risk, with exact-value coverage for the consensus-sensitive changes.

l0rinc force-pushed on Jul 28, 2026

DrahtBot added the label CI failed on Jul 28, 2026

DrahtBot commented at 1:38 AM on July 28, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/30320524467/job/90155255838</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU detected missing/incorrect includes and forced an error (“Failure generated from IWYU” / exit status 1).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

l0rinc force-pushed on Jul 28, 2026

DrahtBot removed the label CI failed on Jul 28, 2026

in src/qt/bitcoingui.cpp:80 in 7104b41d04

  76 | @@ -76,7 +77,7 @@
  77 |   *
  78 |   * Ref: https://github.com/bitcoin/bitcoin/pull/1026
  79 |   */
  80 | -static constexpr int64_t MAX_BLOCK_TIME_GAP = 90 * 60;
  81 | +static constexpr int64_t MAX_BLOCK_TIME_GAP{TicksSeconds(90min)};

maflcko commented at 6:03 PM on July 28, 2026:

This seems like a step in the wrong direction. The docstring of TicksSeconds also says that it should be avoided in code.

the correct approach would be to write constexpr auto ...{90min}; and then use proper chrono types below: std::chrono::seconds secs{blockDate.secsTo(currentDate)};

maflcko commented at 6:04 PM on July 28, 2026: member

Tend to agree with stickies. The changes as-is also violate the util/time.h docs.

maflcko commented at 6:25 PM on July 28, 2026: member

Follow-up to #35792.

Also, the pull description seems wrong. I fail to see how any of the changes here are related to ODR?

rpc: type mockscheduler delta

Parse the requested delta as seconds, compare it against typed limits, and pass it directly to the scheduler interfaces.

d350ac187a

l0rinc force-pushed on Jul 29, 2026

l0rinc renamed this:
~~refactor: use chrono literals for durations~~
refactor: keep duration calculations typed
on Jul 29, 2026

qt: type nice time offsets

Make `formatNiceTimeOffset` accept seconds so its comparisons and divisions retain units.
Callers now construct durations from raw values provided by Qt or consensus code.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

01b15851de

qt: type peer ban durations

Pass typed durations from the peer menu to `banSelectedNode`, converting only at the integer `Node::ban` interface.
Keep the one-year action exactly 365 days.

Co-authored-by: janb84 <githubjanb.drainer976@passmail.net>

34303f5999

l0rinc commented at 6:06 AM on July 29, 2026: contributor

I fail to see how any of the changes here are related to ODR?

They're not, I noticed these raw duration expressions while reviewing #35792's changes, which is why I originally described this as a follow-up. Since that wording can read as grouping the two changes, I removed it from the PR description.

The docstring of TicksSeconds also says that it should be avoided in code.

Thanks for the suggestion, I reworked the branch so durations remain typed where the surrounding APIs already support chrono, and I like it a lot better this way. Where carrying chrono farther would broaden the change or make call sites harder to read, I left the expression unchanged or converted only where an existing interface still takes integer seconds.

qt: name blocks per day in prune estimate

Name the assumed blocks per day directly in the prune backup estimate.

818587ec0f

mempool: type rolling fee halflife as duration

Store the rolling fee halflife as `std::chrono::seconds` and convert it to a scalar only for the decay calculation.
The existing decay test independently pins the duration to 43,200 seconds.

e19adf415c

net, rpc: use typed target spacing accessor

Use `Consensus::Params::PowTargetSpacing()` so the pruning and RPC calculations divide durations directly.
This keeps their units visible and avoids converting either operand to raw seconds.

dd87400b47

refactor: use chrono for fixed durations

State fixed durations in their natural units while preserving integer interfaces and exact values.

79d651a0d0

net: type block relay age limits

Store relay age limits as durations and use typed block timestamps for the time arithmetic.
Convert only where `GetBlockProofEquivalentTime` still returns integer seconds.

Co-authored-by: janb84 <githubjanb.drainer976@passmail.net>

eaad99d30a

test: characterize PoW target timing

Pin literal target timespan and spacing values for every built-in chain.
These assertions provide a redundant units check before the following representation-only refactor.

f8710adff6

kernel: use chrono types for PoW timing

Express each built-in chain’s target timespan and spacing in its natural duration unit.
The preceding characterization pins the exact integer values consumed by consensus code.

Co-authored-by: janb84 <githubjanb.drainer976@passmail.net>

b2aedb6279

validation: type future block time limit

Store the two-hour allowance as a duration and use it directly in time-point comparisons.
Convert it only where existing interfaces still operate on integer seconds.

The `chain_tests/basic_tests` assertion pins the duration to 7,200 seconds.

861d3345fd

test: characterize assumevalid work threshold

Make the assumevalid test hit exactly two weeks of equivalent work and pin that scripts remain checked at the inclusive threshold.
Split longer header sequences at `MAX_HEADERS_RESULTS`.

d51e6b9d32

validation: use chrono for assumevalid horizon

Express the inclusive two-week comparison directly in integer seconds.
The preceding functional characterization pins the exact threshold.

178eb282e3

in src/qt/modaloverlay.cpp:40 in 7baa431d4c

  36 | @@ -36,7 +37,7 @@ ModalOverlay::ModalOverlay(bool enable_wallet, QWidget* parent)
  37 |  
  38 |      m_animation.setTargetObject(this);
  39 |      m_animation.setPropertyName("pos");
  40 | -    m_animation.setDuration(300 /* ms */);
  41 | +    m_animation.setDuration(count_milliseconds(300ms));

maflcko commented at 6:21 AM on July 29, 2026:

not sure what exactly this is trying to enforce or harden. 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.

I think the chrono types should only be used when the type adds some compile-time checks. And I don't think replacing a 300-raw-int literal with a 300-chrono-literal, and a cast to int is adding any checks.

l0rinc commented at 6:35 AM on July 29, 2026:

Sure, I’ve reverted the ones that were arguably noisier than the original code. Thanks for the pushback!

l0rinc commented at 6:38 AM on July 29, 2026:

think the chrono types should only be used when the type adds some compile-time checks.

I'd argue it's also valuable when we can remove comments that had to explain the values (e.g. 14 * 24 * 60 * 60; // two weeks) or when we can use the literals in range checks, e.g. if (delta_seconds <= 0 || delta_seconds > 3600) {.

l0rinc force-pushed on Jul 29, 2026

DrahtBot added the label CI failed on Jul 29, 2026

DrahtBot removed the label CI failed on Jul 29, 2026