net: Use type-safe mockable time for peer connection time #23758

maflcko · 2021-12-13T11:33:49Z

Benefits:

Type-safe
Mockable
Allows to revert a temporary test workaround

This allows to revert the temporary commit 0bfb920 (test: fix test failures in test/functional/p2p_timeouts.py).

-BEGIN VERIFY SCRIPT- ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; } ren nLastBlockTime m_last_block_time ren nLastTXTime m_last_tx_time ren nTimeConnected m_connected -END VERIFY SCRIPT-

DrahtBot · 2021-12-13T18:34:08Z

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

Conflicts

No conflicts as of last run.

shaavan

Concept ACK

Used type-safe std::chrono::seconds to ensure that the compiler will validate types while compiling and throw an error if tried to assign the wrong type to a variable.
Checked that the std::chrono is introduced correctly in the first commit.
The second commit uses std::chrono::seconds variable time_init and time_later to SetMockTime in src/test/denialofservices_tests.cpp file. I have checked that these variables are used to SetMockTime at appropriate places.
However, I am not proficient enough (yet) to comment on if this fixes the bug that was initially described in #23739
The third commit renames the three variables touched in this PR and names them to our latest naming convention.

I have some suggestions:

Since the file src/test/net_peer_eviction_tests.cpp is touched in this PR, how about adding the clang formatting changes suggested by 🔽 to this PR itself.

git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v

shaavan · 2021-12-14T10:30:17Z

src/net_processing.cpp

 {
    LOCK(cs_main);

    int64_t time_in_seconds = GetTime();


Suggested change

int64_t time_in_seconds = GetTime();

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

EvictExtraOutboundPeers(time_in_seconds);

The diff as suggested wouldn't compile and needs more changes, so I'll leave it for a follow-up.

maflcko · 2021-12-14T12:28:54Z

Since the file src/test/net_peer_eviction_tests.cpp is touched in this PR, how about adding the clang formatting changes suggested by arrow_down_small to this PR itself.

Looks like this will cause re-formats of lines in the file I haven't touched? Also, I think the file will/might be touched soon by other PRs, so I want to minimize the conflicts for now. Maybe it can be formatted in a pull that touches those lines directly?

naumenkogs · 2021-12-15T08:24:21Z

ACK fad9438

shaavan · 2021-12-15T11:03:05Z

ACK fad9438

The diff as suggested wouldn't compile and needs more changes, so I'll leave it for a follow-up.

I wasn't aware of this. How about leaving a comment above this line explaining the needed follow-up.

Maybe it can be formatted in a pull that touches those lines directly?

Formatting can be taken up as a direct follow-up to this PR as soon as this one gets merged.

maflcko · 2021-12-15T12:09:48Z

I wasn't aware of this. How about leaving a comment above this line explaining the needed follow-up.

Personally I am not a fan of adding comments to source code to list future refactoring that can be done. I think if it is worth it, it should be done in a pull request (or an issue created to track it).

Do you want to work on that?

shaavan · 2021-12-15T13:06:48Z

Do you want to work on that?

I think I can give it a try!

…tion time

maflcko · 2021-12-16T14:10:28Z

I think I can give it a try!

Nice. Let me know if you have any questions. If not, you can also ping me for review.

rebroad · 2021-12-18T21:58:42Z

These variable rename pull requests are a pain in the ass - I thought it had been agreed previously that PRs should not be created to rename variables?

sipa · 2021-12-18T22:38:03Z

The purpose of this PR is not renaming variables; the point is moving to more type-safe representation of time.

When code is being changed already, the developer guidelines state that the new code should follow the style guidelines, and that's what is happening here.

Furthermore, in this specific case it also makes the change more reviewable. By changing the name, you know every access site is updated.

…::chrono::seconds in net_processing.cpp 92082ea Change time variable type to std::chrono::seconds in src/net_processing.cpp (Shashwat) Pull request description: - This is a follow-up to PR #23758 - This changes the remaining time variable in `net_processing.cpp` from **int64_t** to **std::chrono::seconds** ACKs for top commit: naumenkogs: ACK 92082ea hebasto: re-ACK 92082ea Tree-SHA512: 559e351d9046d4ba2b842ae38da13b4befc7feee71f0762f97907812471e2840b0d43c90c92222d15619fe40cc21f11d40900500ca07b470f7ac8b0046cc1d68

Summary: This is a backport of [[bitcoin/bitcoin#23758 | core#23758]] [1/3] bitcoin/bitcoin@fad7ead Test Plan: `ninja all check-all bitcoin-bench bitcoin-fuzzers && src/bench/bitcoin-bench` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11084

Summary: This allows to revert the temporary fix in test test/functional/p2p_timeouts.py (see 10965) This is a backport of [[bitcoin/bitcoin#23758 | core#23758]] [2/3] bitcoin/bitcoin@fa663a4 Depends on D11084 Note: due to out of sequence backports, we do not yet have `void SetMockTime(std::chrono::seconds mock_time_in)`, so I used `void SetMockTime(int64_t nMockTimeIn)` instead. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11085

Summary: ``` -BEGIN VERIFY SCRIPT- ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; } ren nLastBlockTime m_last_block_time ren nLastTXTime m_last_tx_time ren nTimeConnected m_connected ren nLastProofTime m_last_proof_time -END VERIFY SCRIPT- ``` This is a backport of [[bitcoin/bitcoin#23758 | core#23758]] [3/3] bitcoin/bitcoin@fad9438 Depends on D11085 Test Plan: `ninja all check-all bitcoin-bench bitcoin-fuzzers && src/bench/bitcoin-bench` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11086

refactor: Use type-safe std::chrono in net

fad7ead

maflcko added the Refactoring label Dec 13, 2021

maflcko force-pushed the 2112-timeMockp2p branch from 4bc39a2 to fac1b90 Compare December 13, 2021 11:34

MarcoFalke added 2 commits December 13, 2021 13:32

Use mockable time for peer connection time

fa663a4

This allows to revert the temporary commit 0bfb920 (test: fix test failures in test/functional/p2p_timeouts.py).

scripted-diff: Rename touched member variables

fad9438

-BEGIN VERIFY SCRIPT- ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; } ren nLastBlockTime m_last_block_time ren nLastTXTime m_last_tx_time ren nTimeConnected m_connected -END VERIFY SCRIPT-

maflcko force-pushed the 2112-timeMockp2p branch from fac1b90 to fad9438 Compare December 13, 2021 12:32

This was referenced Dec 13, 2021

Use Sock in CNode #23604

Merged

Make all networking code mockable #21878

Closed

shaavan reviewed Dec 14, 2021

View reviewed changes

maflcko merged commit 60b5795 into bitcoin:master Dec 15, 2021

maflcko deleted the 2112-timeMockp2p branch December 15, 2021 12:09

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

Merge bitcoin#23758: net: Use type-safe mockable time for peer connec…

d4fc833

…tion time

shaavan mentioned this pull request Dec 17, 2021

Refactor: Change time variable type from int64_t to std::chrono::seconds in net_processing.cpp #23801

Merged

bitcoin locked and limited conversation to collaborators Dec 18, 2022

-    int64_t time_in_seconds = GetTime();
+    auto time_in_seconds = GetTime<std::chrono::seconds>();
+    EvictExtraOutboundPeers(time_in_seconds);

net: Use type-safe mockable time for peer connection time #23758

net: Use type-safe mockable time for peer connection time #23758

Uh oh!

Conversation

maflcko commented Dec 13, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 14, 2021

Uh oh!

naumenkogs commented Dec 15, 2021

Uh oh!

shaavan commented Dec 15, 2021

Uh oh!

maflcko commented Dec 15, 2021

Uh oh!

shaavan commented Dec 15, 2021

Uh oh!

maflcko commented Dec 16, 2021

Uh oh!

rebroad commented Dec 18, 2021

Uh oh!

sipa commented Dec 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Dec 13, 2021 •

edited

Loading