p2p: 25880 fixups (stalling timeout) #26982

pull mzumsande wants to merge 2 commits into bitcoin:master from mzumsande:202201_fix_stalling_test changing 2 files +4 −4

mzumsande commented at 8:54 PM on January 27, 2023: contributor
Two small fixups to #25880:
- Use is_connected instead of num_test_p2p_connections to avoid intermittent failures where the p2p MiniNode got disconnected but this info hasn't made it to python yet, so it fails a ping. (https://github.com/bitcoin/bitcoin/pull/25880#discussion_r1089217720)
- Simplify a logging statement (suggested in #25880 (review))

test: fix intermittent errors in p2p_ibd_stalling.py

Using is_connected instead of num_test_p2p_connections
ensures that python has taken notice that the p2p was
disconnected.

6548ba68e8

DrahtBot commented at 8:54 PM on January 27, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK MarcoFalke

Stale ACK sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label P2P on Jan 27, 2023
sipa commented at 10:48 PM on January 27, 2023: member

utACK e99207b908b25bbab70077ebbeca1dca989facb2
in src/net_processing.cpp:5739 in e99207b908 outdated
```
5735 | @@ -5736,7 +5736,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
5736 |              // bandwidth is insufficient.
5737 |              const auto new_timeout = std::min(2 * stalling_timeout, BLOCK_STALLING_TIMEOUT_MAX);
5738 |              if (stalling_timeout != new_timeout && m_block_stalling_timeout.compare_exchange_strong(stalling_timeout, new_timeout)) {
5739 | -                LogPrint(BCLog::NET, "Increased stalling timeout temporarily to %d seconds\n", m_block_stalling_timeout.load().count());
5740 | +                LogPrint(BCLog::NET, "Increased stalling timeout temporarily to %d seconds\n", new_timeout.count());
```
maflcko commented at 9:32 AM on January 28, 2023:

nit (feel free to ignore): it is not recommended to use the count method on a symbol whose type is unknown or whose type can easily change in a refactor. (see comment in util/time.h)

Otherwise, if the type changes to std::chrono::milliseconds, count() will no longer return the number of seconds.

mzumsande commented at 10:37 PM on January 29, 2023:

make sense, changed to count_seconds (or did you have something else in mind?)

maflcko commented at 9:45 AM on January 30, 2023:

Ticks<std::chrono::seconds> could be used as well, in theory. The only difference is that count_seconds is more strict at compile-time. count_seconds will refuse to compile if the precision is higher than seconds, while Ticks<std::chrono::seconds> truncates it.

No strong opinion which one to use, unless you need a truncation already, in which case you are better off with Ticks.