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

sipa · 2020-09-30T08:36:39Z

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 · 2020-09-30T08:39:49Z

Concept ACK

naumenkogs · 2020-09-30T08:45:26Z

Concept ACK.

DrahtBot · 2020-09-30T09:27:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

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)?

maflcko · 2020-09-30T13:56:04Z

C++17

And operator++ ;)

https://en.cppreference.com/w/cpp/chrono/duration/operator_arith2

sipa · 2020-09-30T18:18:32Z

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 · 2020-09-30T18:22:52Z

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 (#18947) and master moves onto C++17 (#16684). Is that still the plan?

sipa · 2020-09-30T18:25:20Z

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 · 2020-09-30T18:28:38Z

Concept ACK

DrahtBot · 2020-10-08T11:39:20Z

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

maflcko · 2021-01-15T19:05:50Z

Still needs rebase. Let me know if this is up for grabs.

ajtowns

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

Concept ACK

ajtowns · 2020-10-03T16:35:22Z

src/rpc/net.cpp

-        if (stats.m_ping_usec > 0) {
-            obj.pushKV("pingtime", ((double)stats.m_ping_usec) / 1e6);
+        if (stats.m_ping_time.count() > 0) {
+            obj.pushKV("pingtime", count_microseconds(stats.m_ping_time) * 0.000001);


maflcko · 2021-01-21T18:19:48Z

@sipa Weekly ping to ask whether this is up for grabs :)

jnewbery · 2021-01-26T15:07:07Z

This is up for grabs.

dhruv · 2021-01-26T16:16:59Z

I'll take a swing at rebasing this code shortly.

vasild · 2021-01-26T16:46:53Z

@dhruv Excellent! I subscribe as reviewer :)

dhruv · 2021-01-27T05:34:23Z

#21015 is ready for review.

…hrono types 0eaea66 Make tx relay data structure use std::chrono types (Pieter Wuille) 55e8288 Make all Poisson delays use std::chrono types (Pieter Wuille) c733ac4 Convert block/header sync timeouts to std::chrono types (Pieter Wuille) 4d98b40 Change all ping times to std::chrono types (Pieter Wuille) Pull request description: (Picking up #20044. Rebased against master.) 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. ACKs for top commit: jnewbery: utACK 0eaea66 vasild: ACK 0eaea66 MarcoFalke: re-ACK 0eaea66, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚 ajtowns: utACK 0eaea66 Tree-SHA512: 2dbd8d53bf82e98f9b4611e61dc14c448e8957d1a02575b837fadfd59f80e98614d0ccf890fc351f960ade76a6fb8051b282e252e81675a8ee753dba8b1d7f57

fanquake added the P2P label Sep 30, 2020

sipa added the Refactoring label Sep 30, 2020

This was referenced Sep 30, 2020

Use std::chrono for the time to rotate destination of addr messages + tests #18642

Merged

p2p: Try to preserve outbound block-relay-only connections during restart #17428

Merged

sipa force-pushed the 202009_chrono_types branch from 8d9a5c4 to 50ede47 Compare September 30, 2020 18:10

sipa force-pushed the 202009_chrono_types branch from 50ede47 to 2430b13 Compare September 30, 2020 21:18

DrahtBot mentioned this pull request Oct 5, 2020

Better intervals between feelers #19869

Closed

sipa added 5 commits October 7, 2020 16:02

Use mockable time everywhere in net_processing

d7fad16

Change all ping times to std::chrono types

d9a994a

Convert block/header sync timeouts to std::chrono types

6143414

Make all Poisson delays use std::chrono types

b38ff67

Make tx relay data structure use std::chrono types

e1fc641

sipa force-pushed the 202009_chrono_types branch from 2430b13 to e1fc641 Compare October 7, 2020 23:40

DrahtBot added the Needs rebase label Oct 8, 2020

ajtowns reviewed Jan 18, 2021

View reviewed changes

jnewbery added the Up for grabs label Jan 26, 2021

maflcko closed this Jan 26, 2021

maflcko removed Needs rebase Up for grabs labels Jan 26, 2021

dhruv mentioned this pull request Jan 26, 2021

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

Merged

maflcko mentioned this pull request Feb 16, 2021

Net: Move ping data to net_processing #20721

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sipa commented Sep 30, 2020

fanquake commented Sep 30, 2020

naumenkogs commented Sep 30, 2020

DrahtBot commented Sep 30, 2020 •

edited

Loading

jnewbery commented Sep 30, 2020

maflcko commented Sep 30, 2020

sipa commented Sep 30, 2020

jnewbery commented Sep 30, 2020

sipa commented Sep 30, 2020

practicalswift commented Sep 30, 2020

DrahtBot commented Oct 8, 2020

maflcko commented Jan 15, 2021

ajtowns left a comment

ajtowns Oct 3, 2020

maflcko commented Jan 21, 2021

jnewbery commented Jan 26, 2021

dhruv commented Jan 26, 2021

vasild commented Jan 26, 2021

dhruv commented Jan 27, 2021

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

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

Conversation

sipa commented Sep 30, 2020

fanquake commented Sep 30, 2020

naumenkogs commented Sep 30, 2020

DrahtBot commented Sep 30, 2020 • edited Loading

Conflicts

jnewbery commented Sep 30, 2020

maflcko commented Sep 30, 2020

sipa commented Sep 30, 2020

jnewbery commented Sep 30, 2020

sipa commented Sep 30, 2020

practicalswift commented Sep 30, 2020

DrahtBot commented Oct 8, 2020

maflcko commented Jan 15, 2021

ajtowns left a comment

Choose a reason for hiding this comment

ajtowns Oct 3, 2020

Choose a reason for hiding this comment

maflcko commented Jan 21, 2021

jnewbery commented Jan 26, 2021

dhruv commented Jan 26, 2021

vasild commented Jan 26, 2021

dhruv commented Jan 27, 2021

DrahtBot commented Sep 30, 2020 •

edited

Loading