Refactor: Changes time variables from int to chrono by shaavan · Pull Request #23832 · bitcoin/bitcoin

shaavan · 2021-12-21T12:08:51Z

This PR is a follow-up to #23801.
This PR aims to make the following changes to all the time variables in net_processing.cpp wherever possible.

Convert all time variables to std::chrono.
Use chorno::literals wherever possible.
Use auto keywords wherever possible.
Use var{val} convention of initialization.

This PR also minimizes the number of times, serialization of time count_seconds(..) occurs.

maflcko

Copncept ACK. I haven't reviewed this, but I left comments

src/net_processing.cpp

theStack · 2021-12-21T12:53:18Z

Concept ACK

DrahtBot · 2021-12-21T19:52:01Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
#21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)
#21160 (net/net processing: Move tx inventory into net_processing by jnewbery)
#18642 (Use std::chrono for the time to rotate destination of addr messages + tests by naumenkogs)

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.

maflcko · 2021-12-22T09:52:30Z

src/net_processing.cpp

+static constexpr auto OVERLOADED_PEER_TX_DELAY{2s};
 /** How long to wait (in microseconds) before downloading a transaction from an additional peer */
-static constexpr std::chrono::microseconds GETDATA_TX_INTERVAL{std::chrono::seconds{60}};
+static constexpr auto GETDATA_TX_INTERVAL{60us};


This is wrong 60s is not equal to 60us

Sorry. I made an error interpreting this line of code earlier.

In the updated PR (commit 16d0b619d72971b23eabcb0a5849370720f93909), I have changed the variable to std::chrono::seconds.

std::chrono::microseconds GETDATA_TX_INTERVAL{std::chrono::seconds{60}}; -> auto GETDATA_TX_INTERVAL{60s};

As I could not see any apparent reason to keep it in microseconds.

shaavan · 2021-12-22T11:38:36Z

Updated from 17af89f to 16d0b619d72971b23eabcb0a5849370720f93909 (pr23832.01 -> pr23832.02)

Addressed @MarcoFalke comments

Changes:

Split PR into two commits.
- First one converting remaining time variables from int to Chrono
- The second one makes the other changes done in this PR.
Added line break between a long LogPrintf line.
Corrected one error in variable initialization.

w0xlt

crACK 16d0b61

klementtan · 2021-12-27T06:41:13Z

crACK 16d0b619d72971b23eabcb0a5849370720f93909

verified that the correct std::chrono::* is used when converting from int to std::chrono
verified that the correct chorno::literals is used

maflcko

ACK 16d0b619d72971b23eabcb0a5849370720f9390 🔠

Show signature

Signature:

src/net_processing.cpp

maflcko · 2022-01-03T11:28:19Z

src/net_processing.cpp

 static constexpr int32_t MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4;
-/** Timeout for (unprotected) outbound peers to sync to our chainwork, in seconds */
-static constexpr int64_t CHAIN_SYNC_TIMEOUT = 20 * 60; // 20 minutes
+/** Timeout for (unprotected) outbound peers to sync to our chainwork */


Will also need to remove "seconds" from where CHAIN_SYNC_TIMEOUT is used in docs?

Just to confirm. Are you talking about this line?

bitcoin/src/net_processing.cpp

Lines 729 to 730 in d69af93

* CHAIN_SYNC_TIMEOUT: if a peer's best known block has less work than our tip,

* set a timeout CHAIN_SYNC_TIMEOUT seconds in the future:

This line could use a fixing.

However, doing so will revoke the current 3 ACKs on the current top commit of this PR.
So would you suggest I should update this PR for this change?

If it is fixed, it should be fixed here to avoid another follow-up pull. If not, it might be best to leave it as-is.

A re-ACK is less overhead and effort than a new pull + fresh ACK.

Fixed in fe86eb5

This commit does following changes to time variables in net_processing.h: - Used {} initialization. - Uses universal initializer auto. - Uses chrono::literals The reason for these changes is to make code simpler, and easier to understand and rationalize.

shaavan · 2022-01-04T05:56:26Z

Updated from 16d0b619d72971b23eabcb0a5849370720f93909 to fe86eb5 (pr23832.02 -> pr23832.03, diff)
Addressed @MarcoFalke suggestion.

Removed the term second for CHAIN_SYNC_TIMEOUT in docs.

Should be trivial to reACK.

maflcko

re-ACK fe86eb5 🏕

only minor doc change

Show signature

Signature:

maflcko reviewed Dec 21, 2021

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

DrahtBot added P2P Refactoring labels Dec 21, 2021

This was referenced Dec 22, 2021

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

net/net processing: Move tx inventory into net_processing #21160

Merged

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

Merged

maflcko reviewed Dec 22, 2021

View reviewed changes

shaavan force-pushed the time_refactor branch from 17af89f to 16d0b61 Compare December 22, 2021 11:37

DrahtBot mentioned this pull request Dec 22, 2021

net processing: Reduce resource usage for inbound block-relay-only connections #22778

Merged

w0xlt approved these changes Dec 23, 2021

View reviewed changes

shaavan requested a review from maflcko December 25, 2021 06:38

klementtan approved these changes Dec 27, 2021

View reviewed changes

maflcko approved these changes Jan 3, 2022

View reviewed changes

shaavan added 2 commits January 4, 2022 11:17

Refactor: Changes remaining time variable type from int to chrono

6111b0d

shaavan force-pushed the time_refactor branch from 16d0b61 to fe86eb5 Compare January 4, 2022 05:54

maflcko approved these changes Jan 6, 2022

View reviewed changes

maflcko merged commit 0620957 into bitcoin:master Jan 6, 2022

shaavan deleted the time_refactor branch January 6, 2022 13:16

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 6, 2022

Merge bitcoin#23832: Refactor: Changes time variables from int to chrono

f313d0a

bitcoin locked and limited conversation to collaborators Jan 6, 2023

	* CHAIN_SYNC_TIMEOUT: if a peer's best known block has less work than our tip,
	* set a timeout CHAIN_SYNC_TIMEOUT seconds in the future:

Conversation

shaavan commented Dec 21, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theStack commented Dec 21, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaavan commented Dec 22, 2021

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

klementtan commented Dec 27, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaavan commented Jan 4, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Dec 21, 2021 •

edited

Loading