Skip to content

Refactor: Changes time variables from int to chrono #23832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 6, 2022
Merged

Refactor: Changes time variables from int to chrono #23832

merged 2 commits into from
Jan 6, 2022

Conversation

shaavan
Copy link
Contributor

@shaavan shaavan commented Dec 21, 2021

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
maflcko reviewed Dec 21, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@theStack
Copy link
Contributor

Concept ACK

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 21, 2021 •
edited
Loading

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.

This was referenced Dec 22, 2021
Closed
Merged
Merged
maflcko
maflcko reviewed Dec 22, 2021
View reviewed changes
src/net_processing.cpp Outdated
/** 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};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong 60s is not equal to 60us

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

In the updated PR (commit 16d0b61), 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
Copy link
Contributor Author

shaavan commented Dec 22, 2021

Updated from 17af89f to 16d0b61 (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.

@DrahtBot DrahtBot mentioned this pull request Dec 22, 2021
Merged
w0xlt
w0xlt approved these changes Dec 23, 2021
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK 16d0b61

@shaavan shaavan requested a review from maflcko December 25, 2021 06:38
@klementtan
Copy link
Contributor

crACK 16d0b61

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

maflcko
maflcko approved these changes Jan 3, 2022
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 16d0b61 🔠

Show signature

Signature:

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

ACK 16d0b619d72971b23eabcb0a5849370720f9390 🔠
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgWGgv/XEpKkyorblBTjNK5oX47xGt3LfLxMXjnRW59LYfE9yo1W+TkYiEYALGN
ltJw9WQCyMpsJYLHrjL9ll6Z6w+ZE4F6MAZybTM2tOMzToDJgLT0+L6eQsb6HDgn
bZhp+HJXtWDrL+kD7RxpNbeeSDVRGDLTUElzuBQRacShUM+aXX2vrf6/tQyih1F+
H7E4EKH9f5ntWlxnT0l2h5ildkX6txh31fU3H9fwrKvRZ5bNZEogGF07fOaZFreE
y06UR4wxelPw4eRKdavhe79GlV6C+uFrf7SNLDzg7CziQTKJbU41m+WZ8f06gtQK
5EkT11i7tKaEiMbUeGIFDFOYsu9xCeTRr6oCM1nomb4iNREKAIClp24XtiN8hQye
kN7QPWoPot5LOk6geT5UN0mti6j5O//fW8kT+7Z1D7aFI7fSBwVqHNjK+YAnXrrk
O3TSZVyXu/+ljzvYWLZUPpqQaa3durNCjOFtV+VyysP7ApasVXK6W/FXx9Zf2uiw
49855sGB
=q2XZ
-----END PGP SIGNATURE-----

src/net_processing.cpp
@@ -57,8 +57,8 @@ static constexpr auto HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1ms;
* behind headers chain.
*/
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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fe86eb5

shaavan added 2 commits January 4, 2022 11:17
@shaavan
Refactor: Changes remaining time variable type from int to chrono
6111b0d
@shaavan
Refactor: Uses c++ init convention for time variables
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
Copy link
Contributor Author

shaavan commented Jan 4, 2022

Updated from 16d0b61 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
maflcko approved these changes Jan 6, 2022
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK fe86eb5 🏕

only minor doc change

Show signature

Signature:

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

re-ACK fe86eb50c986f7b5ccce63f984d8a51cd9ee9e2c  🏕

only minor doc change
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhSQAv/aT3khOx+9Ns7XrLCiJPyXT33dwP69vqblTWRrSrga/mRkuvsGrAG89xG
z3zhR8Mc5KUA2+OOshFwM4twxSzh/8ugcWAiIQ8P0zkmF+oL+wV1K0l3TLf8W8Vt
gPhIgKt+7t+lNZYvvMCD3bTImrG0/0+/TpuV6NaP+hNKGEmswOaXoqle/kez2ZXU
ObsjBJvlSk3EeBZRDQxh3ZuO99tkhHFM4OzHaAdhoE8zi0c6FCi+uNxSDPs/UH6Z
rYIJnSAYxUZ/DnJUqLZTd59N/tUAZPjBbs/yso6V/tr7yt4tbVVaPaOSzbj/e95B
pqg3xSssL6KYqUzeYudFJY0K2S9nvQM+si7STkqFZ3aSPf1GRcXCbkUEoqVENRMG
yrgIaSC23WrRMas+CSk5gaqKHUTlpM/6WV8KnNaosQbSBhU7mi/lTf6SZNc5XomC
mkek8R4TRft1Q7qivmOrVsSlBuGrrCRjMtc7ddwlJ2f2bz2kLgkwUzw1hsGcf+Lj
nmp944H6
=S8MN
-----END PGP SIGNATURE-----

@maflcko maflcko merged commit 0620957 into bitcoin:master Jan 6, 2022
@shaavan 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 bitcoin locked and limited conversation to collaborators Jan 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@maflcko maflcko maflcko approved these changes

@klementtan klementtan klementtan approved these changes

@w0xlt w0xlt w0xlt approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shaavan @theStack @DrahtBot @klementtan @maflcko @w0xlt