refactor address relay time

MarcoFalke commented at 3:10 pm on March 28, 2022: member

Those refactors are overlapping with, but otherwise largely unrelated to #24662.

MarcoFalke added the label Refactoring on Mar 28, 2022

MarcoFalke added the label P2P on Mar 28, 2022

MarcoFalke force-pushed on Mar 28, 2022

DrahtBot commented at 2:20 pm on March 29, 2022: contributor

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.

in src/net_processing.cpp:2967 in fa0e4eb108 outdated

2961@@ -2962,8 +2962,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2962 
2963         // Store the new addresses
2964         std::vector<CAddress> vAddrOk;
2965-        int64_t nNow = GetAdjustedTime();
2966-        int64_t nSince = nNow - 10 * 60;
2967+        const std::chrono::seconds adjusted_current_time{GetAdjustedTime()};

laanwj commented at 11:09 am on April 4, 2022:

If we use current_time as variable name here (like above), it’ll save on another round of renaming when use of adjusted times throughout address relay is removed.

MarcoFalke commented at 11:11 am on April 4, 2022:

It is already named that way, see two lines below (2969)

laanwj commented at 11:51 am on April 5, 2022:

OK

fanquake requested review from mzumsande on Apr 5, 2022

fanquake requested review from naumenkogs on Apr 5, 2022

fanquake requested review from ajtowns on Apr 5, 2022

in src/addrman.cpp:566 in fa0e4eb108 outdated

561@@ -562,10 +562,11 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
562 
563     if (pinfo) {
564         // periodically update nTime
565-        bool fCurrentlyOnline = (GetAdjustedTime() - addr.nTime < 24 * 60 * 60);
566-        int64_t nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60);
567-        if (addr.nTime && (!pinfo->nTime || pinfo->nTime < addr.nTime - nUpdateInterval - nTimePenalty))
568+        const bool currently_online{std::chrono::seconds{GetAdjustedTime()} - std::chrono::seconds{addr.nTime} < 24h};

mzumsande commented at 10:33 pm on April 6, 2022:

nit: could add include for <chrono> to addrman

MarcoFalke commented at 7:19 am on April 7, 2022:

This commit requires using namespace std::chrono_literals;, so I’ve #include <util/time.h> instead.

MarcoFalke commented at 11:04 am on April 7, 2022:

Removed commit for now

mzumsande approved

mzumsande commented at 10:55 pm on April 6, 2022: contributor

Code Review ACK fa0e4eb108b0a2a04bc703298626413587587168

As far as I can see all refactors are correct, and I agree that it makes sense to use std::chrono. I verified that the log message is unchanged.

ajtowns commented at 0:52 am on April 7, 2022: contributor

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.

MarcoFalke force-pushed on Apr 7, 2022

MarcoFalke commented at 7:38 am on April 7, 2022: member

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.

ajtowns commented at 8:41 am on April 7, 2022: contributor

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:

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

with READWRITECONVERT defined as:

 0#define READWRITECONVERT(obj, type, from, to) (::ReadWriteConvert<type>(s, ser_action, obj, from, to))
 1
 2template<typename X, typename From, typename To, typename Y, typename Stream> void ReadWriteConvert(Stream& s, CSerActionSerialize ser_action, const Y& obj, From from, To)
 3{
 4    X dummy{from(obj)};
 5    ::Serialize(s, dummy);
 6}
 7
 8template<typename X, typename From, typename To, typename Y, typename Stream> void ReadWriteConvert(Stream& s, CSerActionUnserialize ser_action, Y& obj, From, To to)
 9{
10    X dummy;
11    ::Unserialize(s, dummy);
12    obj = to(dummy);
13}

]

in src/addrman.cpp:566 in fadf5a7086 outdated

562@@ -562,10 +563,11 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, int64_
563 
564     if (pinfo) {
565         // periodically update nTime
566-        bool fCurrentlyOnline = (GetAdjustedTime() - addr.nTime < 24 * 60 * 60);
567-        int64_t nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60);
568-        if (addr.nTime && (!pinfo->nTime || pinfo->nTime < addr.nTime - nUpdateInterval - nTimePenalty))
569+        const bool currently_online{std::chrono::seconds{GetAdjustedTime()} - std::chrono::seconds{addr.nTime} < 24h};

ajtowns commented at 8:57 am on April 7, 2022:

Having std::chrono::seconds GetAdjustedTimeChrono() { return std::chrono::seconds{GetAdjustedTime()}; } seems like it would make some of this less clunky?

ajtowns commented at 9:19 am on April 7, 2022: contributor

ACK fadf5a70862f9bb8e582a2a0991148def94ad077

I think the advantage of using std::chrono is type checking and brevity/simplicity – being able to say “now + 2h” and have the compiler ensure that everything’s using seconds or microseconds or whatever correctly.

So I think it’s a problem if we’re having to do a lot of manual conversions (via std::chrono::seconds{nTime} and count_seconds(now)), especially doing them in the middle of expressions (eg pinfo->nTime < addr.nTime - count_seconds(update_interval) - nTimePenalty). Most of the type changes here are making the code longer and harder to read, rather than shorter and simpler, for instance…

That’s okay while we’re in the middle of changing things, I guess; but I think the endgame should be that we’re only manually converting to/from microseconds/seconds/hours when we’re logging/serializing…

MarcoFalke force-pushed on Apr 7, 2022

MarcoFalke commented at 11:34 am on April 7, 2022: member

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

MarcoFalke closed this on Apr 7, 2022

MarcoFalke deleted the branch on Apr 7, 2022

MarcoFalke renamed this:
~~refactor: Address relay time refactors~~
refactor: Use type-safe time for address relay
on Apr 7, 2022

MarcoFalke restored the branch on Apr 8, 2022

MarcoFalke reopened this on Apr 8, 2022

MarcoFalke commented at 11:23 am on April 8, 2022: member

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.

MarcoFalke force-pushed on Apr 8, 2022

jonatack commented at 11:56 am on April 8, 2022: contributor

Skimmed the changes so far, concept ACK.

MarcoFalke force-pushed on Apr 8, 2022

ajtowns commented at 8:49 am on April 9, 2022: contributor

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)

0template<class Rep, class Period>
1std::chrono::duration<Rep,Period> rand_dur(std::chrono::duration<Rep,Period> max) { ... }
2
3template<class T>
4auto rand_seconds(T max) { return rand_dur<std::chrono::seconds>(max); }

MarcoFalke renamed this:
~~refactor: Use type-safe time for address relay~~
refactor address relay time
on Apr 9, 2022

MarcoFalke marked this as a draft on Apr 9, 2022

MarcoFalke commented at 8:53 am on April 9, 2022: member

Thanks, made title more general, since it also includes serialize refactoring changes.

I am still working on the “clocks” version of this and will push it when ready for evaluation.

MarcoFalke marked this as ready for review on Apr 9, 2022

MarcoFalke force-pushed on Apr 9, 2022

in src/addrman.cpp:81 in fa0c9d8ce1 outdated

81+    if (CountSeconds(nTime) == 0 || now - nTime > ADDRMAN_HORIZON) { // not seen in recent history
82         return true;
83+    }
84 
85-    if (m_last_success == 0 && nAttempts >= ADDRMAN_RETRIES) // tried N times and never a success
86+    if (CountSeconds(m_last_success) == 0 && nAttempts >= ADDRMAN_RETRIES) { // tried N times and never a success

ajtowns commented at 1:56 am on April 11, 2022:

Maybe make NeverTried() and NeverSucceeded() helpers for the CountSeconds(m_*) == 0 checks to make the intent a little clearer?

MarcoFalke commented at 11:43 am on April 11, 2022:

This is the only place where the value is compared against 0 with the trailing comment explaining the intent, so I don’t think it makes sense to introduce a helper. A helper can be introduced when more places call it.

ajtowns commented at 12:45 pm on April 11, 2022:

Yeah, I think it was mostly the nTime == 0 ones that were triggering my noise threshold.

in src/addrman.cpp:77 in fa0c9d8ce1 outdated

76+    if (nTime > now + 10min) { // came in a flying DeLorean
77         return true;
78+    }
79 
80-    if (nTime == 0 || nNow - nTime > ADDRMAN_HORIZON_DAYS * 24 * 60 * 60) // not seen in recent history
81+    if (CountSeconds(nTime) == 0 || now - nTime > ADDRMAN_HORIZON) { // not seen in recent history

ajtowns commented at 2:03 am on April 11, 2022:

Are the nTime == 0 checks really needed?

MarcoFalke commented at 11:43 am on April 11, 2022:

Thx, removed code.

in src/net.cpp:458 in fa0c9d8ce1 outdated

452@@ -453,10 +453,9 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
453         }
454     }
455 
456-    /// debug print
457     LogPrint(BCLog::NET, "trying connection %s lastseen=%.1fhrs\n",
458-        pszDest ? pszDest : addrConnect.ToString(),
459-        pszDest ? 0.0 : (double)(GetAdjustedTime() - addrConnect.nTime)/3600.0);
460+             pszDest ? pszDest : addrConnect.ToString(),
461+             CountHoursDouble(pszDest ? 0h : AdjustedTime() - addrConnect.nTime));

ajtowns commented at 2:08 am on April 11, 2022:

(pszDest ? 0.0 : CountHoursDouble( ... )) ?

Would it be better to have a FormatDuration function that takes a duration and return a string directly, or a couple of them with precisions of hours / seconds / milliseconds / microseconds?

MarcoFalke commented at 1:55 pm on April 11, 2022:

Maybe something that can be done after C++20 with https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt ? Not sure if worth it right now

MarcoFalke commented at 11:22 am on April 16, 2022:

On a second thought it might probably be best to introduce a chrono auto-formatter to pick an appropriate precision and type at run time depending on the value of the duration.

MarcoFalke commented at 4:11 pm on April 19, 2022:

Started in https://github.com/bitcoin/bitcoin/pull/24923

MarcoFalke commented at 8:45 am on June 29, 2022:

Closing thread. Let me know if there is anything left do be done here (in this pull)

in src/util/time.h:26 in fa4e9ca651 outdated

20+    /** Return current system time or mocked time, if set */
21+    static time_point now() noexcept;
22+    static std::time_t to_time_t(const time_point&) = delete; // unused
23+    static time_point from_time_t(std::time_t) = delete;      // unused
24+};
25+using TimeSeconds = std::chrono::time_point<Clock, std::chrono::seconds>;

ajtowns commented at 2:12 am on April 11, 2022:

Maybe call these MockableClock and MockableTimeSeconds (or similar) so that if (when) we introduce a non-mockable (monotonic?) clock, it’s always clear which one is which.

MarcoFalke commented at 9:02 am on April 11, 2022:

I don’t think there is a need to. A non-mockable clock is the system clock: std::chrono::system_clock (C++11) and std::chrono::sys_seconds (C++20). A monotonic clock is std::chrono::steady_clock.

ajtowns commented at 12:45 pm on April 11, 2022:

Well, there’s certainly no “need” to – you could call it HumptyDumpty and WallSeconds and it would still work…

But the C++ clocks all have an adjective describing their main point (there’s also gps_clock and utc_clock and high_resolution_clock and tai_clock and file_clock). It seems like a sensible convention to follow; having GetTime() be mockable and GetTimeSeconds() be non-mockable has certainly confused me previously…

MarcoFalke commented at 12:56 pm on April 11, 2022:

certainly confused me previously…

Me too, but I think the solution to that is to rename the function names around system time to mention “system” or “sys”, similar to what is done in std::chrono:: itself. Maybe even replace all of them with std::chrono::system_clock?

I think there are almost no use cases for a system clock in our codebase. Debug logging might be the only use case, so forcing all other places that deal with time to specify MockableTimeSeconds seems verbosity for no benefit. If the only use case of the system clock is for debug logging, which is likely disabled anyway, and one accidentally uses a mockable clock, no user will notice a difference. The only difference could happen in tests, but even there, most tests don’t rely on mocktime, so I am highly dubious of any benefits.

ajtowns commented at 1:55 pm on April 11, 2022:

I think we definitely want system_clock for (a) logging, and (b) implementing mockable_clock; I think we want a non-mockable clock, but maybe would prefer steady_clock for scheduling (currently uses system_clock), benchmarking, gui responsiveness(?), RPC call duration(?).

Calling it mock_seconds by analogy to sys_seconds seems pretty fine if you want something shorter. const auto now = Now<mock_seconds>(); or more normally (when you don’t need to hash/serialize the value in seconds), const auto now = mock_clock.now(); (or MockSec if you want camel case and even shorter)

“If .. one accidentally uses a mockable clock, no user will notice a difference. The only difference could happen in tests” – is true for GetTime() vs GetTimeSeconds() too…

MarcoFalke commented at 3:17 pm on April 11, 2022:

I think we definitely want system_clock for (a) logging, and (b) implementing mockable_clock; I think we want a non-mockable clock, but maybe would prefer steady_clock for scheduling (currently uses system_clock), benchmarking, gui responsiveness(?), RPC call duration(?).

Agree.

Calling it mock[…]

My general thinking is that test code shouldn’t distract and clutter the main code, if possible. So for example, any test logic should ideally be implemented in test code, outside the main code. Also, naming should be done ignoring the test logic, if possible.

It looks like we agree on the logic changes, but disagree on the name. Maybe other reviewers could chime in?

ajtowns commented at 4:00 pm on April 12, 2022:

Could call it by some other adjective; I thought “mock” was the obvious one, since that’s how we distinguish the clocks when talking about them anyway. That our clock can stop for extended periods makes it pretty different to regular clocks and even though that’s test only behaviour, it’s something you need to know about when using it. NodeClock maybe? shrug

MarcoFalke commented at 11:11 am on May 10, 2022:

Thanks, changed to NodeClock in https://github.com/bitcoin/bitcoin/pull/25101

in src/net.cpp:1805 in fa1b64a092 outdated

1800@@ -1801,9 +1801,8 @@ void CConnman::ThreadDNSAddressSeed()
1801             unsigned int nMaxIPs = 256; // Limits number of IPs learned from a DNS seed
1802             if (LookupHost(host, vIPs, nMaxIPs, true)) {
1803                 for (const CNetAddr& ip : vIPs) {
1804-                    int nOneDay = 24*3600;
1805                     CAddress addr = CAddress(CService(ip, Params().GetDefaultPort()), requiredServiceBits);
1806-                    addr.nTime = GetTime() - 3*nOneDay - rng.randrange(4*nOneDay); // use a random age between 3 and 7 days old
1807+                    addr.nTime = Now<TimeSeconds>() - 3 * 24h - rng.rand_seconds(4 * 24h); // use a random age between 3 and 7 days old

ajtowns commented at 3:33 am on April 11, 2022:

Would it be better to write something like addr.n_Time = rand_uniform_delay(Now<TimeSeconds>(), /range=*/ 4*24h, /*offset=*/ -7*24h) ? You can pick up the precision from the first argument that way. (Similar thing would work for the poisson delays, I think)

MarcoFalke commented at 11:58 am on April 11, 2022:

You can pick up the precision from the first argument that way

I don’t think this is possible. If you want a uniform delay in [2h, 3h), typing rand_uniform_delay(2h, 1h) will produce the wrong result.

ajtowns commented at 12:33 pm on April 11, 2022:

I meant something like:

0template<typename TP>
1TP rand_uniform_delay(const TP& base, typename TP::duration range)
2{
3    return base + TP::duration{rng.randrange(range.count())};
4}

so the invocation above would be rand_uniform_delay(TimeSeconds{2h}, 1h) ? (EDIT: or rand_uniform_delay(Now<TimeSeconds()> + 2h, /*range=*/ 1h))

MarcoFalke commented at 12:41 pm on April 11, 2022:

This will still break if you call tp_2 = rand_uniform_delay(tp_1, 1h) and tp_1 is of type TimeHours in this context, and tp_2 of type TimeSeconds. If you want to make this templated, I think the std::common_type approach that GetRandDur uses is the only way. Though, as this forces you to specify the precision, you might as well put the precision right into the function name.

Moreover, I am not sure if we should force time points here. After all this is just a duration and it is up to the caller to apply it to a time point or further use it as a duration.

So I think all I can do here is rename rand_seconds to rand_uniform_seconds.

ajtowns commented at 2:20 pm on April 11, 2022:

Using TimeHours would already be a mistake: if you’re measuring time only with a precision of hours, you’ve already got +/- 30min error to whatever real time you’re trying to target.

We should be using full precision time points (MockClock::time_point) everywhere we can; only reason not to do that for addr.nTime is that we only have seconds since the epoch in the serialization format, and using higher precision data when we throw it through the hash on the first run and lower precision data on later runs would be confusing.

MarcoFalke commented at 3:12 pm on April 11, 2022:

Hmmm, one way to implement “I want to run this exactly once per hour, but at a uniform random time within that hour” is to have tp_2 to be the execution time in seconds and tp_1 be tp_2 cast to hours + 1h.

The code with rand_uniform_delay would look like:

0tp_2 = rand_uniform_delay(std::chrono::time_point_cast<TimeSeconds>(std::chrono::time_point_cast<TimeHours>(tp_2 + 1h)), 1h)

or

0tp_2 = rand_uniform_delay<TimeSeconds>(std::chrono::time_point_cast<TimeHours>(tp_2 + 1h), 1h) // not sure if this compiles

Whereas with rand_uniform_seconds it would look like:

0tp_2 = rand_uniform_seconds(std::chrono::time_point_cast<TimeHours>(tp_2 + 1h), 1h)

or (if durations are used)

0tp_2 = std::chrono::time_point_cast<TimeHours>(tp_2 + 1h) + rand_uniform_seconds(0h, 1h)

I am not sure how often quantization is used with random delays, but I wouldn’t call it a “mistake” to intentionally quantize time.

Absent a use case for quantization, rand_uniform_seconds still is a bit less code/less <>. Moreover it can be used for non-time-point “plain” durations. So I have a slight preference for it. If there was a use case for quantization, it would also be safer.

ajtowns commented at 4:56 pm on April 11, 2022:

Hmmm, one way to implement “I want to run this exactly once per hour, but at a uniform random time within that hour” is to have tp_2 to be the execution time in seconds and tp_1 be tp_2 cast to hours + 1h.

Is there anywhere that we do that? Everything I’ve come across is a variation on GetTime() + random(range) + offset.

I’d probably prefer to write that as:

0when = rand_uniform_delay(round_up(now(), 1h), 1h);
1   
2template<typename TP>
3TP round_up(TP tp, typename TP::duration dur)
4{
5    return tp + dur - (tp.time_since_epoch() % dur);
6}

since then you don’t have to declare a new type if you want things every 90 minutes instead of every hour. Don’t think you can get a compile time check that dur > 0 though (at least with C++17)

0tp_2 = rand_uniform_seconds(std::chrono::time_point_cast<TimeHours>(tp_2 + 1h), 1h)

I guess I don’t really think you should ever want/need to use a time_point_cast outside of util/time.h/cpp.

I am not sure how often quantization is used with random delays, but I wouldn’t call it a “mistake” to intentionally quantize time.

No, the mistake’s doing it via the time_point object. The whole benefit of durations and time_points is the compiler can automatically deal with you specifying things in minutes or hours without having to do manual conversions all the time.

Absent a use case for quantization, rand_uniform_seconds still is a bit less code/less <>.

Maybe I’m misunderstanding what rand_uniform_seconds is, but:

0auto rand_seconds(std::chrono::seconds max) noexcept { return decltype(max){randrange(max.count())}; }
1auto rand_millis(std::chrono::milliseconds max) noexcept { return decltype(max){randrange(max.count())}; }
2auto rand_micros(std::chrono::microseconds max) noexcept { return decltype(max){randrange(max.count())}; }

doesn’t seem like notably less code than:

0template<typename TP>
1auto rand_uniform_delay(TP tp, typename TP::duration range) noexcept
2{ 
3    return tp + (typename TP::duration){randrange(range.count())};
4}

Moreover it can be used for non-time-point “plain” durations. So I have a slight preference for it. If there was a use case for quantization, it would also be safer.

I think all the current uses of randrange against chrono inputs are getting added to GetTime() of one sort or another, so I don’t think there’d be a use case for that once the conversion to time_points is done.

MarcoFalke commented at 7:47 am on April 12, 2022:

I think all the current uses of randrange against chrono inputs are getting added to GetTime() of one sort or another, so I don’t think there’d be a use case for that once the conversion to time_points is done.

scheduleFromNow takes a duration, not a time point, and I don’t think this is going to change as we start using clocks and time points.

ajtowns commented at 3:55 pm on April 12, 2022:

scheduleFromNow is already an inline function that takes a duration and adds it to a time point in order to call schedule.

MarcoFalke commented at 7:02 am on April 14, 2022:

Ah good point. I’ll take a stab at this soon(TM)

MarcoFalke commented at 11:20 am on April 16, 2022:

Thanks, fixed and resolved conversation.

in src/bench/addrman.cpp:116 in fa1b64a092 outdated

108@@ -109,9 +109,10 @@ static void AddrManGetAddr(benchmark::Bench& bench)
109 static void AddrManAddThenGood(benchmark::Bench& bench)
110 {
111     auto markSomeAsGood = [](AddrMan& addrman) {
112+        const auto now{AdjustedTime()};
113         for (size_t source_i = 0; source_i < NUM_SOURCES; ++source_i) {
114             for (size_t addr_i = 0; addr_i < NUM_ADDRESSES_PER_SOURCE; ++addr_i) {
115-                addrman.Good(g_addresses[source_i][addr_i]);
116+                addrman.Good(g_addresses[source_i][addr_i], now);

ajtowns commented at 3:35 am on April 11, 2022:

Apart from this one change here, I don’t really see the point of the commit that drops the default time arg.

MarcoFalke commented at 11:47 am on April 11, 2022:

I didn’t like the implicit logic, relying on a “global” (the time), so I removed the default arg. But I am happy to drop the commit.

ajtowns commented at 12:29 pm on April 11, 2022:

I had the same intuition – had to look up the standard to check AdjustedTime() would even be called each time rather than just once at startup – but the refactor makes it a lot more wordy, so it just doesn’t seem like a win to me shrug

MarcoFalke commented at 9:18 am on May 23, 2022:

Removed commit 8ad4f4d17b for now.

ajtowns commented at 7:06 am on April 11, 2022: contributor

Seems like a reasonable approach to me

MarcoFalke force-pushed on Apr 11, 2022

MarcoFalke marked this as a draft on Apr 14, 2022

MarcoFalke force-pushed on Apr 16, 2022

MarcoFalke marked this as ready for review on Apr 16, 2022

MarcoFalke force-pushed on Apr 19, 2022

ajtowns commented at 10:37 am on April 20, 2022: contributor

Approach ACK – Looks pretty good to me now; still think Clock should have an adjective, still don’t see much value in the last commit, and didn’t really look through the “time-safe addrman” commit.

DrahtBot added the label Needs rebase on May 10, 2022

MarcoFalke force-pushed on May 10, 2022

DrahtBot removed the label Needs rebase on May 10, 2022

MarcoFalke marked this as a draft on May 10, 2022

DrahtBot added the label Needs rebase on May 12, 2022

fanquake referenced this in commit 6407c0e8a3 on May 20, 2022

MarcoFalke marked this as ready for review on May 23, 2022

MarcoFalke force-pushed on May 23, 2022

DrahtBot removed the label Needs rebase on May 23, 2022

MarcoFalke force-pushed on May 24, 2022

DrahtBot added the label Needs rebase on May 24, 2022

MarcoFalke force-pushed on May 25, 2022

DrahtBot removed the label Needs rebase on May 25, 2022

DrahtBot added the label Needs rebase on May 26, 2022

fanquake referenced this in commit ba48fcf4a4 on May 28, 2022

sidhujag referenced this in commit d148a072ea on May 28, 2022

sidhujag referenced this in commit 617d0bd427 on May 28, 2022

laanwj referenced this in commit d8ae504448 on Jun 7, 2022

MarcoFalke force-pushed on Jun 8, 2022

DrahtBot removed the label Needs rebase on Jun 8, 2022

fanquake referenced this in commit 9edc5133d4 on Jun 9, 2022

DrahtBot added the label Needs rebase on Jun 9, 2022

MarcoFalke force-pushed on Jun 9, 2022

DrahtBot removed the label Needs rebase on Jun 9, 2022

sidhujag referenced this in commit 56db887052 on Jun 13, 2022

MarcoFalke force-pushed on Jun 27, 2022

in src/addrman.cpp:38 in fa9070acc5 outdated

36 /** How many successive failures are allowed ... */
37 static constexpr int32_t ADDRMAN_MAX_FAILURES{10};
38-/** ... in at least this many days */
39-static constexpr int64_t ADDRMAN_MIN_FAIL_DAYS{7};
40+/** ... in at least this duration */
41+static constexpr auto ADDRMAN_MIN_FAIL{7 * 24h};

naumenkogs commented at 11:38 am on July 6, 2022:

perhaps it’s a good opportunity to give a better variable name here. ADDRMAN_FAIL_DUR_UNIT or something

MarcoFalke commented at 11:45 am on July 6, 2022:

Sure, happy to do if people agree. Please leave a comment here or upvote glebs comment.

DrahtBot added the label Needs rebase on Jul 19, 2022

MarcoFalke force-pushed on Jul 19, 2022

MarcoFalke commented at 12:12 pm on July 19, 2022: member

Rebased

DrahtBot removed the label Needs rebase on Jul 19, 2022

aureleoules commented at 9:13 am on July 20, 2022: member

ACK fad1f55cdefe457fd8fbdfee5a7e2fd2b69be850. I agree that using std::chrono instead of raw ints makes the code easier to read and allows for a better type checking of timestamps.

refactor: Remove not needed std::max fa9284c3e9

scripted-diff: Rename addrman time symbols

-BEGIN VERIFY SCRIPT-
 ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

 ren nLastTry          m_last_try
 ren nLastSuccess      m_last_success
 ren nLastGood         m_last_good
 ren nLastCountAttempt m_last_count_attempt
 ren nSinceLastTry     since_last_try
 ren nTimePenalty      time_penalty
 ren nUpdateInterval   update_interval
 ren fCurrentlyOnline  currently_online
-END VERIFY SCRIPT-

fa21fc60c2

util: Add HoursDouble fa253d385f

Add ChronoFormatter to serialize fa5103a9f5

Add type-safe AdjustedTime() getter to timedata

Also, fix includes.

The getter will be used in a future commit.

fa2ae373f3

refactor: Use type-safe std::chrono for addrman time fa64dd6673

in src/addrman.cpp:95 in fad1f55cde outdated

 98-double AddrInfo::GetChance(int64_t nNow) const
 99+double AddrInfo::GetChance(NodeSeconds now) const
100 {
101     double fChance = 1.0;
102-    int64_t since_last_try = std::max<int64_t>(nNow - m_last_try, 0);
103+    auto since_last_try = std::max(now - m_last_try, 0s);

naumenkogs commented at 8:19 am on July 26, 2022:

Why does this need std::max?

MarcoFalke commented at 10:55 am on July 26, 2022:

Thx, removed as a new first commit.

MarcoFalke force-pushed on Jul 26, 2022

in src/addrman.cpp:830 in fa64dd6673

825@@ -823,9 +826,10 @@ void AddrManImpl::Connected_(const CService& addr, int64_t nTime)
826     AddrInfo& info = *pinfo;
827 
828     // update info
829-    int64_t update_interval = 20 * 60;
830-    if (nTime - info.nTime > update_interval)
831-        info.nTime = nTime;
832+    const auto update_interval{20min};
833+    if (time - info.nTime > update_interval) {

naumenkogs commented at 11:31 am on July 26, 2022:

it’s unfortunate github highlights time as it’s a keyword.

in src/addrman.cpp:997 in fa64dd6673

993@@ -990,8 +994,9 @@ int AddrManImpl::CheckAddrman() const
994         int n = entry.first;
995         const AddrInfo& info = entry.second;
996         if (info.fInTried) {
997-            if (!info.m_last_success)
998+            if (!TicksSinceEpoch<std::chrono::seconds>(info.m_last_success)) {

naumenkogs commented at 11:32 am on July 26, 2022:

I’m generally not a big fan of casting 0 to bool :)

naumenkogs commented at 11:54 am on July 26, 2022: member

utACK fa64dd6673767992eb4e0e775fb0afdfd298610d

fanquake requested review from mzumsande on Jul 26, 2022

fanquake requested review from dergoegge on Jul 26, 2022

in src/addrman.cpp:92 in fa64dd6673

 97 
 98-double AddrInfo::GetChance(int64_t nNow) const
 99+double AddrInfo::GetChance(NodeSeconds now) const
100 {
101     double fChance = 1.0;
102-    int64_t nSinceLastTry = std::max<int64_t>(nNow - nLastTry, 0);

dergoegge commented at 8:52 am on July 27, 2022:

nit (feel free to ignore): This variable is still being renamed in the scripted diff in the next commit but it no longer exists.

dergoegge commented at 8:56 am on July 27, 2022: member

Code review ACK fa64dd6673767992eb4e0e775fb0afdfd298610d

fanquake merged this on Jul 27, 2022

fanquake closed this on Jul 27, 2022

sidhujag referenced this in commit 7b36c8f266 on Jul 27, 2022

MarcoFalke deleted the branch on Jul 29, 2022

bitcoin locked this on Jul 29, 2023

refactor address relay time #24697

Conflicts