Make all of net_processing (and some of net) use std::chrono types #20044

pull sipa wants to merge 5 commits into bitcoin:master from sipa:202009_chrono_types changing 12 files +119 −138

sipa commented at 8:36 AM on September 30, 2020: member

This changes various uses of integers to represent timestamps and durations to std::chrono duration types with type-safe conversions, getting rid of various .count(), constructors, and conversion factors.

Builds on #20027.
fanquake added the label P2P on Sep 30, 2020
sipa added the label Refactoring on Sep 30, 2020
fanquake commented at 8:39 AM on September 30, 2020: member

Concept ACK
naumenkogs commented at 8:45 AM on September 30, 2020: member

Concept ACK.
DrahtBot commented at 9:27 AM on September 30, 2020: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #20027 (Use mockable time everywhere in net_processing by sipa)
- #19988 (Overhaul transaction request logic by sipa)
- #19972 (tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable. by practicalswift)
- #19893 (test: Remove or explain syncwithvalidationinterfacequeue by MarcoFalke)
- #19884 (p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty by dhruv)
- #19869 (Better intervals between feelers by naumenkogs)
- #19858 (Periodically make block-relay connections and sync headers by sdaftuar)
- #18642 (Use std::chrono for the time to rotate destination of addr messages + tests by naumenkogs)
- #17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)
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.
jnewbery commented at 1:35 PM on September 30, 2020: member

Concept ACK. Is it worth waiting for C++17 and switching directly to chrono literals (eg https://en.cppreference.com/w/cpp/chrono/operator%22%22h)?
MarcoFalke commented at 1:56 PM on September 30, 2020: member

C++17

And operator++ ;)

https://en.cppreference.com/w/cpp/chrono/duration/operator_arith2
sipa force-pushed on Sep 30, 2020
sipa commented at 6:18 PM on September 30, 2020: member

Concept ACK. Is it worth waiting for C++17 and switching directly to chrono literals (eg https://en.cppreference.com/w/cpp/chrono/operator%22%22h)?

That would be nice, but it's also kind of an independent improvement (there are a bunch of time durations constants in the code that aren't touched by this PR).

I don't like waiting for C++17, as we don't actually know when that may happen. I saw there were some issues with using C++17's std::filesystem, which may push things back for example (I may be mistaken here, but in general I don't like a "We shouldn't do X yet, because we expect that Soon(tm) we will be able to do it better").
jnewbery commented at 6:22 PM on September 30, 2020: member

in general I don't like a "We shouldn't do X yet, because we expect that Soon(tm) we will be able to do it better"

I totally agree, but in this case I thought Soon(tm) was about 4 weeks from now when 0.21 is branched (https://github.com/bitcoin/bitcoin/issues/18947) and master moves onto C++17 (https://github.com/bitcoin/bitcoin/issues/16684). Is that still the plan?
sipa commented at 6:25 PM on September 30, 2020: member

I totally agree, but in this case I thought Soon(tm) was about 4 weeks from now when 0.21 is branched (#18947) and master moves onto C++17 (#16684). Is that still the plan?

Oh that's fair. I don't know why I was conflating "not being able to use certain C++17 features like std::filesystem" with "not being able to switch compilation to C++17".
practicalswift commented at 6:28 PM on September 30, 2020: contributor

Concept ACK
sipa force-pushed on Sep 30, 2020
Use mockable time everywhere in net_processing d7fad1690c
Change all ping times to std::chrono types d9a994a794
Convert block/header sync timeouts to std::chrono types 6143414858
Make all Poisson delays use std::chrono types b38ff67f24
Make tx relay data structure use std::chrono types e1fc6418f3

in src/rpc/net.cpp:187 in f41f4dd097 outdated

 182 | @@ -183,14 +183,14 @@ static RPCHelpMan getpeerinfo()
 183 |          obj.pushKV("bytesrecv", stats.nRecvBytes);
 184 |          obj.pushKV("conntime", stats.nTimeConnected);
 185 |          obj.pushKV("timeoffset", stats.nTimeOffset);
 186 | -        if (stats.m_ping_usec > 0) {
 187 | -            obj.pushKV("pingtime", ((double)stats.m_ping_usec) / 1e6);
 188 | +        if (stats.m_ping_time.count() > 0) {
 189 | +            obj.pushKV("pingtime", count_microseconds(stats.m_ping_time) * 0.000001);

ajtowns commented at 4:35 PM on October 3, 2020:

* 1e-6 ?

sipa force-pushed on Oct 7, 2020
DrahtBot commented at 11:39 AM on October 8, 2020: member



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
DrahtBot added the label Needs rebase on Oct 8, 2020
MarcoFalke commented at 7:05 PM on January 15, 2021: member

Still needs rebase. Let me know if this is up for grabs.
ajtowns commented at 2:54 AM on January 18, 2021: member

Hmm, apparently I started a review of this at some point and got distracted?

Concept ACK
MarcoFalke commented at 6:19 PM on January 21, 2021: member

@sipa Weekly ping to ask whether this is up for grabs :)
jnewbery added the label Up for grabs on Jan 26, 2021
jnewbery commented at 3:07 PM on January 26, 2021: member

This is up for grabs.
dhruv commented at 4:16 PM on January 26, 2021: member

I'll take a swing at rebasing this code shortly.
MarcoFalke closed this on Jan 26, 2021
MarcoFalke removed the label Needs rebase on Jan 26, 2021
MarcoFalke removed the label Up for grabs on Jan 26, 2021
vasild commented at 4:46 PM on January 26, 2021: member

@dhruv Excellent! I subscribe as reviewer :)
dhruv commented at 5:34 AM on January 27, 2021: member

#21015 is ready for review.
fanquake referenced this in commit 33921379b6 on Mar 4, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#20027 Use mockable time everywhere in net_processing #21015 Make all of net_processing (and some of net) use std::chrono types

Make all of net_processing (and some of net) use std::chrono types #20044

Conflicts