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

MarcoFalke commented at 11:33 am on December 13, 2021: member

Benefits:

Type-safe
Mockable
Allows to revert a temporary test workaround

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

MarcoFalke added the label Refactoring on Dec 13, 2021

MarcoFalke force-pushed on Dec 13, 2021

Use mockable time for peer connection time

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

fa663a4c0d

scripted-diff: Rename touched member variables

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

fad943821e

MarcoFalke force-pushed on Dec 13, 2021

DrahtBot commented at 6:34 pm on December 13, 2021: member

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

Conflicts

No conflicts as of last run.

in src/net_processing.cpp:4338 in fad943821e

4336@@ -4335,7 +4337,7 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
4337 
4338     int64_t time_in_seconds = GetTime();

shaavan commented at 10:30 am on December 14, 2021:

0    auto time_in_seconds = GetTime<std::chrono::seconds>();
1    
2    EvictExtraOutboundPeers(time_in_seconds);

MarcoFalke commented at 12:26 pm on December 14, 2021:

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

shaavan commented at 10:56 am on December 14, 2021: contributor

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 :arrow_down_small: to this PR itself.

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

MarcoFalke commented at 12:28 pm on December 14, 2021: member

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 commented at 8:24 am on December 15, 2021: member

ACK fad943821e35d0eb2143061e718f0193e12a4c71

shaavan commented at 11:03 am on December 15, 2021: contributor

ACK fad943821e35d0eb2143061e718f0193e12a4c71

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.

MarcoFalke merged this on Dec 15, 2021

MarcoFalke closed this on Dec 15, 2021

MarcoFalke commented at 12:09 pm on December 15, 2021: member

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?

MarcoFalke deleted the branch on Dec 15, 2021

shaavan commented at 1:06 pm on December 15, 2021: contributor

Do you want to work on that?

I think I can give it a try!

sidhujag referenced this in commit d4fc833fc9 on Dec 15, 2021

MarcoFalke commented at 2:10 pm on December 16, 2021: member

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 commented at 9:58 pm on December 18, 2021: contributor

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 commented at 10:38 pm on December 18, 2021: member

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.

MarcoFalke referenced this in commit 3ec8f9f123 on Dec 20, 2021

Fabcien referenced this in commit 35eee65f61 on Feb 21, 2022

Fabcien referenced this in commit e3884eec0d on Feb 21, 2022

Fabcien referenced this in commit 73c309dc24 on Feb 21, 2022

DrahtBot locked this on Dec 18, 2022

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

Conflicts