Make all of net_processing (and some of net) use std::chrono types by dhruv · Pull Request #21015 · bitcoin/bitcoin

dhruv · 2021-01-26T22:24:40Z

(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.

DrahtBot · 2021-01-27T01:24:03Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: update inbound eviction protection for multiple networks, add I2P peers #21261 (p2p, test: add inbound eviction protection for I2P peers by jonatack)
net processing: Extract addr send functionality into MaybeSendAddr() #21236 (Net processing: Extract addr send functionality into MaybeSendAddr() by jnewbery)
net/net processing: Move addr data into net_processing #21186 (Net/Net Processing: Move addr data into net_processing by jnewbery)
net/net processing: Move tx inventory into net_processing #21160 (Net/Net processing: Move tx inventory into net_processing by jnewbery)
p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197 (p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage by jonatack)

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.

dhruv · 2021-01-27T06:02:47Z

I am still working on figuring out why test/functional/wallet_groups.py is failing with an off-by-one error only on some platforms. In the meantime, ready for initial review.

test/functional/wallet_resendwallettransactions.py

src/net.cpp

maflcko · 2021-01-27T06:38:40Z

Concept ACK. This refactoring should only be changing the types at compile-time, not any values at run time.

maflcko

left some style-nits. Feel free to ignore.

review ACK c9a8cbc64eddd98d2183380148a3d9c79c1bd7d2 🐉

Show signature and timestamp

Signature:

Timestamp of file with hash 21b8f1b48c83567a179a56af130b796e2cfb94ca44d7acdf169927f865176af6 -

src/net.h

src/qt/guiutil.cpp

src/qt/guiutil.h

src/rpc/net.cpp

src/net_processing.cpp

dhruv · 2021-01-27T19:08:13Z

Thanks for the review, @MarcoFalke. Comments addressed. Ready for further review.

felipsoarez

Concept ACK

dhruv · 2021-01-27T21:54:10Z

Pushed to fix doxygen comment.

maflcko

re-ACK f4450e105781441020964af1e488a033ec1c4be4 only change is rebase and replacing round with cast 🥈

Show signature and timestamp

Signature:

Timestamp of file with hash e074c33ed68514a9ea03925c2afa9f30b80c656ab5d0cf97a31ed6c37918737f -

src/net.h

maflcko · 2021-02-26T08:56:55Z

@ajtowns if there are specifics that you want to see addressed, it might help to post a patch which compiles and that can be taken in the next rebase. Otherwise I think this is ready for merge (mod the nits, which can be fixed in the next rebase as well).

jnewbery

Thanks for all your work in rebasing this and keeping it current, @dhruv!

utACK f4450e105781441020964af1e488a033ec1c4be4

src/net_processing.cpp

dhruv · 2021-03-03T17:57:57Z

Rebased. master changed while I was rebasing, so we see 2 force pushes. Reviewers can use git range-diff cabe637 f4450e1 0eaea66 for convenience.

Comments addressed:

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

Ready for further review.

jnewbery · 2021-03-03T19:36:04Z

utACK 0eaea66

vasild · 2021-03-04T11:02:24Z

ACK 0eaea66

maflcko

re-ACK 0eaea66, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 0eaea66e8bfdfb23ff86c0f0924c2d75f5aca75f, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgJigwAlGgnegNU2yOZwT9wE9VjciuGc8zXxc4Su130pNLHYd3yirTDUUKwbk4l
pzjUIP6GGSFNHWKjMkGTXgIqzlKQ3YUWfBqCBZKt60G+bwAPfm+tri4lBxBsVXk6
D2e16A8c/mdFfyRPeFl59qtfBE3Wdu7AtHL47u77jLywCUnAPopm9PaHz/xp+w70
O0w5FEr2cazU8tp0u2CY6rMufqjyCZZ/qR4fqNwmOFSR+luBma1ZW7Y0VbmTkjVy
JDrlsITpSyQy9yTosRD0RZ+86BfuGuCR5M3wWsKoepuVpX886pHl/jZEyi9C+eG1
b6ijElFEcU79COu+zocyThQqAkemegbPUBQYttpPtI9d6FZuBUVMK7P/siBVUUPi
75xu76QgF2wuUkCFXDtBobLOGTUlUiKair0lcy/vBWjy/oT12LCh1YZTeYuPwYle
a51LXkhN+dyNUsfYOrN0NkR58VPcKPRk26sbEEzXS750uZx4SIdJ+qSsy5w5LNZw
518vxGAE
=d/DI
-----END PGP SIGNATURE-----

Timestamp of file with hash ba138de68b952c6d098d60b477ecdf0b15cc363ee617f9836e4dafe46273e159 -

ajtowns

utACK 0eaea66

ajtowns · 2021-03-04T10:53:46Z

src/qt/guiutil.cpp

+QString formatPingTime(std::chrono::microseconds ping_time)
 {
-    return (ping_usec == std::numeric_limits<int64_t>::max() || ping_usec == 0) ? QObject::tr("N/A") : QString(QObject::tr("%1 ms")).arg(QString::number((int)(ping_usec / 1000), 10));
+    return (ping_time == std::chrono::microseconds::max() || ping_time == 0us) ?


Having this be ping_time == ping_time.max() would ensure you don't accidently get the wrong type (eg if ping_time was changed to milliseconds, and set to max it would no longer compare equal to microseconds max). There's a few places where the code might be improved by that change.

… std::c… …hrono types

Summary: Partial backport of [[bitcoin/bitcoin#21015 | core#21015]]: bitcoin/bitcoin@4d98b40 Depends on D10902. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10903

Summary: Partial backport of [[bitcoin/bitcoin#21015 | core#21015]]: bitcoin/bitcoin@c733ac4 Depends on D10903. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10906

Summary: Partial backport of [[bitcoin/bitcoin#21015 | core#21015]]: bitcoin/bitcoin@55e8288 Depends on D10911. The OUTBOUND_INVENTORY_BROADCAST_INTERVAL constant has been skipped to keep the changes from rABC3252c21e2856bce86e6222d246073fbde8ad9916, but the INBOUND_INVENTORY_BROADCAST_INTERVAL renaming kept to closer match the source material. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10913

Summary: Completes backport of [[bitcoin/bitcoin#21015 | core#21015]]: bitcoin/bitcoin@0eaea66 Depends on D10913. Ref T1696. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10914

DrahtBot added GUI P2P RPC/REST/ZMQ Utils/log/libs labels Jan 26, 2021

dhruv force-pushed the pr-20044 branch from 6902489 to f1cfafa Compare January 27, 2021 00:43

dhruv force-pushed the pr-20044 branch from f1cfafa to a90a899 Compare January 27, 2021 01:40

dhruv force-pushed the pr-20044 branch from a90a899 to f4bc3c6 Compare January 27, 2021 03:16

DrahtBot mentioned this pull request Jan 27, 2021

p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty #19884

Merged

dhruv mentioned this pull request Jan 27, 2021

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

Closed

maflcko reviewed Jan 27, 2021

View reviewed changes

test/functional/wallet_resendwallettransactions.py Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

maflcko added Refactoring and removed GUI RPC/REST/ZMQ Utils/log/libs labels Jan 27, 2021

dhruv force-pushed the pr-20044 branch 2 times, most recently from 6f286b4 to c9a8cbc Compare January 27, 2021 07:11

DrahtBot mentioned this pull request Jan 27, 2021

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

Merged

maflcko approved these changes Jan 27, 2021

View reviewed changes

dhruv force-pushed the pr-20044 branch from c9a8cbc to 147dd6a Compare January 27, 2021 19:03

felipsoarez reviewed Jan 27, 2021

View reviewed changes

dhruv force-pushed the pr-20044 branch from 147dd6a to c0822a7 Compare January 27, 2021 21:50

maflcko approved these changes Feb 18, 2021

View reviewed changes

maflcko reviewed Feb 18, 2021

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 22, 2021

p2p: update inbound eviction protection for multiple networks, add I2P peers #21261

Merged

DrahtBot added the Needs rebase label Feb 25, 2021

jnewbery reviewed Mar 3, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

dhruv force-pushed the pr-20044 branch from f4450e1 to 8e86d98 Compare March 3, 2021 17:40

sipa added 4 commits March 3, 2021 09:48

Change all ping times to std::chrono types

4d98b40

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

c733ac4

Make all Poisson delays use std::chrono types

55e8288

Make tx relay data structure use std::chrono types

0eaea66

dhruv force-pushed the pr-20044 branch from 8e86d98 to 0eaea66 Compare March 3, 2021 17:48

DrahtBot removed the Needs rebase label Mar 3, 2021

DrahtBot mentioned this pull request Mar 4, 2021

net processing: Extract addr send functionality into MaybeSendAddr() #21236

Merged

maflcko approved these changes Mar 4, 2021

View reviewed changes

ajtowns reviewed Mar 4, 2021

View reviewed changes

fanquake merged commit 3392137 into bitcoin:master Mar 4, 2021

ryanofsky mentioned this pull request Mar 4, 2021

Multiprocess bitcoin #10102

Draft

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 4, 2021

Merge bitcoin#21015: Make all of net_processing (and some of net) use…

1506583

… std::c… …hrono types

This was referenced Mar 6, 2021

Handle peer addition/removal in a right way bitcoin-core/gui#164

Merged

Add peertablesortproxy module bitcoin-core/gui#18

Merged

JaredTate mentioned this pull request Oct 30, 2021

WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code DigiByte-Core/digibyte#77

Closed

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

Conversation

dhruv commented Jan 26, 2021 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

dhruv commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jan 27, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhruv commented Jan 27, 2021

Uh oh!

felipsoarez left a comment

Choose a reason for hiding this comment

Uh oh!

dhruv commented Jan 27, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Feb 26, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dhruv commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Mar 3, 2021

Uh oh!

vasild commented Mar 4, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

dhruv commented Jan 26, 2021 •

edited by maflcko

Loading

DrahtBot commented Jan 27, 2021 •

edited

Loading

dhruv commented Jan 27, 2021 •

edited

Loading

dhruv commented Mar 3, 2021 •

edited

Loading