net: use std::chrono throughout maxOutbound logic #20253

fanquake · 2020-10-27T11:54:02Z

Switch to using std::chrono types for the max outbound related logic.
Removes some unnecessary code from init.

practicalswift · 2020-10-28T07:04:36Z

Concept ACK

theStack

Concept ACK
Note that the functions SetMaxOutbound{Target,Timeframe} are also used in the fuzz test test/fuzz/connman.cpp, hence the uses should also be removed there.

fanquake · 2020-11-16T06:22:55Z

Note that the functions SetMaxOutbound{Target,Timeframe} are also used in the fuzz test test/fuzz/connman.cpp, hence the uses should also be removed there.

Thanks. I've dropped them from the fuzz tests now.

DrahtBot · 2020-11-17T14:18:04Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Remove nMyStartingHeight from CNode/Connman #20649 (refactor: Remove nMyStartingHeight from CNode/Connman by MarcoFalke)
addrman: Make addrman a top-level component #20228 (addrman: Make addrman a top-level component by jnewbery)

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-11-18T11:29:16Z

Concept ACK

DrahtBot · 2020-11-18T13:10:44Z

🐙 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".}

fanquake · 2020-11-18T13:15:47Z

Rebased for #20408 and fixed an oversight in the fuzz changes.

src/net.cpp

jnewbery

Concept ACK, especially cleaning up init.

Longer term, it'd be nice to move initialization of some of these CConnman parameters into the constructor and make them const. That clarifies the initialization and means that they don't need to be guarded by a mutex.

src/net.h

fanquake · 2020-12-09T12:53:20Z

I've rebased this, cherry-picked in fa11110 from 20602 and updated some std::chrono usage to use literals as suggested.

jonatack

ACK 832fc10e9e45abf92a4f04012c918156d92a3649

with some optional style ideas below

jonatack · 2020-12-09T15:51:06Z

src/net.cpp

832fc10e9e45abf92a perhaps use explicit typing here, const, and universal initialization to check for narrowing

- std::chrono::seconds cycleEndTime = nMaxOutboundCycleStartTime + MAX_UPLOAD_TIMEFRAME; - auto now = GetTime<std::chrono::seconds>(); + const std::chrono::seconds cycleEndTime{nMaxOutboundCycleStartTime + MAX_UPLOAD_TIMEFRAME}; + const std::chrono::seconds now{GetTime<std::chrono::seconds>()};

jonatack · 2020-12-09T15:55:59Z

src/net.cpp

832fc10e9e45abf92a4f04012c918156 maybe prefer to be explicit about types and narrowing conversions in the net code

- std::chrono::seconds timeLeftInCycle = GetMaxOutboundTimeLeftInCycle(); - uint64_t buffer = timeLeftInCycle / std::chrono::minutes{10} * MAX_BLOCK_SERIALIZED_SIZE; + const std::chrono::seconds timeLeftInCycle{GetMaxOutboundTimeLeftInCycle()}; + const uint64_t buffer{timeLeftInCycle / std::chrono::minutes{10} * static_cast<uint64_t>(MAX_BLOCK_SERIALIZED_SIZE)};

jonatack · 2020-12-09T16:01:36Z

src/net.cpp

832fc10e9e45abf9 const and maybe explicit typing + narrowing check

Suggested change

auto now = GetTime<std::chrono::seconds>();

const std::chrono::seconds now{GetTime<std::chrono::seconds>()};

Having std::chrono::seconds twice on the same line is pretty verbose. I'd like to think we can just use auto here because the type is clear from the call to GetTime<>().

Yes, the reason for the comment here was constness.

Note that existing code in the same file specifies the type explicitly, line 1646: const std::chrono::seconds now = GetTime<std::chrono::seconds>(); introduced in commit b24a17f

jnewbery · 2020-12-10T10:40:32Z

src/net.h

Since this is only used internally in net, it could now be moved to the implementation cpp file to avoid exporting this symbol to other translation units.

Done in the latest push.

src/net.h

jnewbery

utACK 832fc10e9e

In case you touch this branch again: In commit net: remove nMaxOutboundTimeframe from connection options I think you need to change "DEFAULT_MAX_UPLOAD_TARGET is a compile time constant." to "DEFAULT_MAX_UPLOAD_TIMEFRAME is a compile time constant."

This has been unused since f3552da.

This was introduced in 872fee3 and it's unclear if it's ever been used.

It's not actually possible to change this value, so remove the indirection of it being a conn option. DEFAULT_MAX_UPLOAD_TIMEFRAME is a compile time constant.

DEFAULT_MAX_UPLOAD_TARGET is a compile time constant.

fanquake · 2020-12-13T03:45:24Z

Rebased & addressed some comments.

In case you touch this branch again:

I've fixed the commit message.

maflcko

review ACK 0475c8b 🎭

Show signature and timestamp

Signature:

Timestamp of file with hash 963300e8377971b0f423721ee34e6ab50d44d071f41bb4f7f3598935d178a88d -

jnewbery · 2020-12-15T10:04:44Z

utACK 0475c8b

Summary: ``` Switch to using std::chrono types for the max outbound related logic. Removes some unnecessary code from init. ``` Backport of [[bitcoin/bitcoin#20253 | core#20253]]. Depends on D10901. 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/D10902

fanquake added Refactoring P2P labels Oct 27, 2020

theStack reviewed Nov 15, 2020

View reviewed changes

fanquake force-pushed the net_unused_outbound branch from 788a945 to 3c84c6d Compare November 16, 2020 06:23

DrahtBot mentioned this pull request Nov 17, 2020

CConnman: move initialization to declaration #20408

Merged

fanquake mentioned this pull request Nov 18, 2020

net: Remove SetMaxOutboundTimeframe() #20416

Closed

DrahtBot added the Needs rebase label Nov 18, 2020

fanquake force-pushed the net_unused_outbound branch from 3c84c6d to 803212c Compare November 18, 2020 13:14

fanquake removed the Needs rebase label Nov 18, 2020

theStack reviewed Nov 22, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

fanquake force-pushed the net_unused_outbound branch from 803212c to 2404f78 Compare November 23, 2020 03:47

maflcko approved these changes Nov 23, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Nov 23, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 4, 2020

addrman: Make addrman a top-level component #20228

Merged

fanquake force-pushed the net_unused_outbound branch from 2404f78 to 79d8998 Compare December 9, 2020 12:52

fanquake mentioned this pull request Dec 9, 2020

util: Allow use of C++14 chrono literals #20602

Merged

fanquake force-pushed the net_unused_outbound branch from 79d8998 to 832fc10 Compare December 9, 2020 13:11

jonatack reviewed Dec 9, 2020

View reviewed changes

jnewbery reviewed Dec 10, 2020

View reviewed changes

DrahtBot mentioned this pull request Dec 10, 2020

Refactoring and minor improvement for self-advertisements #19843

Closed

fanquake added 2 commits December 13, 2020 10:38

net: remove SetMaxOutboundTarget

2f3f1ae

This has been unused since f3552da.

net: remove SetMaxOutboundTimeframe

b117eb1

This was introduced in 872fee3 and it's unclear if it's ever been used.

fanquake added 3 commits December 13, 2020 11:10

net: remove nMaxOutboundTimeframe from connection options

173d0d3

It's not actually possible to change this value, so remove the indirection of it being a conn option. DEFAULT_MAX_UPLOAD_TIMEFRAME is a compile time constant.

init: set nMaxOutboundLimit connection option directly

f805933

DEFAULT_MAX_UPLOAD_TARGET is a compile time constant.

net: use std::chrono throughout maxOutbound logic

0475c8b

fanquake force-pushed the net_unused_outbound branch from 832fc10 to 0475c8b Compare December 13, 2020 03:44

DrahtBot mentioned this pull request Dec 14, 2020

refactor: Remove nMyStartingHeight from CNode/Connman #20649

Merged

maflcko approved these changes Dec 15, 2020

View reviewed changes

fanquake merged commit f1f2418 into bitcoin:master Dec 15, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 15, 2020

Merge bitcoin#20253: net: use std::chrono throughout maxOutbound logic

83106bd

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

fanquake deleted the net_unused_outbound branch November 9, 2022 16:31

	auto now = GetTime<std::chrono::seconds>();
	const std::chrono::seconds now{GetTime<std::chrono::seconds>()};

net: use std::chrono throughout maxOutbound logic #20253

net: use std::chrono throughout maxOutbound logic #20253

Conversation

fanquake commented Oct 27, 2020

Uh oh!

practicalswift commented Oct 28, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Nov 16, 2020

Uh oh!

DrahtBot commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Nov 18, 2020

Uh oh!

DrahtBot commented Nov 18, 2020

Uh oh!

fanquake commented Nov 18, 2020

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fanquake commented Dec 9, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake Dec 13, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Dec 13, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Dec 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

fanquake Dec 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Dec 13, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Nov 17, 2020 •

edited

Loading

jnewbery left a comment •

edited

Loading

jonatack Dec 9, 2020 •

edited

Loading

jonatack Dec 13, 2020 •

edited

Loading