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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #25673 (refactor: make member functions const when applicable by aureleoules)
• #25284 ([WIP] consensus: Remove dependency on net (BIP 155 / ADDRV2_FORMAT) by MarcoFalke)
• #25221 (Improve CMedianFilter algorithm by antoinedesbois)
• #23829 (refactor: use braced init for integer literals instead of c style casts by PastaPastaPasta)
• #21878 (Make all networking code mockable by vasild)
• #21283 (Implement BIP 370 PSBTv2 by achow101)

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.

I guess I'm not entirely seeing the point of this? If we're trying to keep AdjustedTime, shouldn't we have a GetChronoAdjustedTime() rather than casting the result of the existing function? If we're trying to get rid of it, shouldn't we just do that, rather than fiddling with whether we treat it as a seconds count or not? If you just want to write constants as "1h" instead of "60*60", could do that via count_seconds(1h)?

EDIT: I suppose you're not really doing much more than that anyway... Would be nice to have a motivation in the PR description I guess; the code changes don't really seem obviously better to me.

I guess I'm not entirely seeing the point of this? If we're trying to keep AdjustedTime, shouldn't we have a GetChronoAdjustedTime() rather than casting the result of the existing function? If we're trying to get rid of it, shouldn't we just do that, rather than fiddling with whether we treat it as a seconds count or not?

I assume that adjusted time will be removed from addrman (see concept acks in the conflicting pull).
Once it is removed, it will be switched to use std::chrono (type safe, human readable, mockable).
This means that any std::chrono::seconds{GetAdjustedTime()} will be changed to GetTime<std::chrono::seconds>() (or a pre-existing variable storing the result). However, CAddress::nTime will not be converted to std::chrono::seconds, because it is a serialized field, which needs to be an exact-width unsigned type, not an implementation defined signed type.

If you just want to write constants as "1h" instead of "60*60", could do that via count_seconds(1h)?

Sure that is possible, but I don't see where it could make sense. count_seconds returns an integral value, so using it instead of 3600.0 still requires double{count_seconds(1h)}. I think CountHoursDouble is nicer, but I am happy to drop the commit.
Using count_seconds in nUpdateInterval could make sense, though I am assuming that nTimePenalty will use std::chrono after commit 89a08ec82fe8547cfccccada62b59af01715d538 (from the conflicting pull) as well, so I think switching it now is fine. Happy to drop the commit, though.

Would be nice to have a motivation in the PR description I guess;

I've added some text to each commit to motivate it, no code changes. Can be checked with git range-diff bitcoin-core/master fa0e4eb108 fadf5a7086.

the code changes don't really seem obviously better to me.

There are four commit, and I am happy to drop any or all of them.

However, CAddress::nTime will not be converted to std::chrono::seconds, because it is a serialized field, which needs to be an exact-width unsigned type, not an implementation defined signed type.

Could have accessors that convert to chrono, or convert from chrono to uint32_t when serializing (and back when deserializing).

[EDIT: can do the conversion when de/serializing as something like:

READWRITECONVERT(obj.chrono_time, uint32_t,
    [](const auto& t) { return static_cast<uint32_t>(count_seconds(t)); },
    [](const uint32_t t) { return std::chrono::seconds{t}; }
);

with READWRITECONVERT defined as:

#define READWRITECONVERT(obj, type, from, to) (::ReadWriteConvert<type>(s, ser_action, obj, from, to))

template<typename X, typename From, typename To, typename Y, typename Stream> void ReadWriteConvert(Stream& s, CSerActionSerialize ser_action, const Y& obj, From from, To)
{
    X dummy{from(obj)};
    ::Serialize(s, dummy);
}

template<typename X, typename From, typename To, typename Y, typename Stream> void ReadWriteConvert(Stream& s, CSerActionUnserialize ser_action, Y& obj, From, To to)
{
    X dummy;
    ::Unserialize(s, dummy);
    obj = to(dummy);
}

]

Thanks for the review. At this point it seems easier to include the commits in the conflicting pull request.

Going to reopen this with all refactoring changes split out. Switching adjusted time to system time can then be done with a trivial scripted-diff.

Skimmed the changes so far, concept ACK.

Concept ACK, but the "Remove default time arguments" seems to also be switching from adjusted time to GetTime, which seems like a bit more than a refactor for type safety?

Could do: EDIT: (no you can't)

template<class Rep, class Period>
std::chrono::duration<Rep,Period> rand_dur(std::chrono::duration<Rep,Period> max) { ... }

template<class T>
auto rand_seconds(T max) { return rand_dur<std::chrono::seconds>(max); }

Rebased

utACK fa64dd6