p2p: 25880 fixups (stalling timeout) #26982
Conversation
Using is_connected instead of num_test_p2p_connections ensures that python has taken notice that the p2p was disconnected.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
utACK e99207b |
src/net_processing.cpp
Outdated
| @@ -5736,7 +5736,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) | |||
| // bandwidth is insufficient. | |||
| const auto new_timeout = std::min(2 * stalling_timeout, BLOCK_STALLING_TIMEOUT_MAX); | |||
| if (stalling_timeout != new_timeout && m_block_stalling_timeout.compare_exchange_strong(stalling_timeout, new_timeout)) { | |||
| LogPrint(BCLog::NET, "Increased stalling timeout temporarily to %d seconds\n", m_block_stalling_timeout.load().count()); | |||
| LogPrint(BCLog::NET, "Increased stalling timeout temporarily to %d seconds\n", new_timeout.count()); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
make sense, changed to count_seconds (or did you have something else in mind?)
There was a problem hiding this comment.
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.
Also use count_seconds() instead of count() for type safety.
e99207b to
b2a1e47
Compare
There was a problem hiding this comment.
review ACK b2a1e47 🕧
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK b2a1e477444bfb90328b353e89967ace6ef10918 🕧
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUit2Qv/V8xz1BG6WizGuF0vhcwOlTAyBYDYpC+VsE3Yc5w9zNipOPOPQRpapj4R
LLMXex8AlWILIVcQZ8hKqla244kASglpKPhHE/kSoX8DuMsEDE7VCKO/Tc9Kg+qh
4/y4gX8gc/Zsj+v+lwCXGeQqeCJBqKCLlA4S86A+77QkcgM33hL+1ZYPVgq/nrNR
skfGkGoXJi7XBqGTfih8UBw5gicCrwZCqcn7etV+3AidGzNXIvNp2aX/nVj2ywqu
UCCJx75S1W+cYTRoGaI25Qot1QmKJ0EQQkWuB8IBk2CPy2xmpy0qqR1tk76KxB9o
Ecro1NKYEq0o8//OTuEYoctqfLkYf8Laaioux7PeLaHI3RjVZKBVlnekJKfu5uM2
IdMlobeu5qPzLXklh5YGeKQ2LB7XWkuzBm5ha2b5Xze+RgDhWS5xa7le+FWwWLV+
AkdFLx2lW+dR+eUOeCNQ1qKAFYPhpCFETzRLS58wBieNAd3JOTXI74Nu19qPqfk+
9VjCKV8S
=LYeM
-----END PGP SIGNATURE-----
b2a1e47 net_processing: simplify logging statement (Martin Zumsande) 6548ba6 test: fix intermittent errors in p2p_ibd_stalling.py (Martin Zumsande) Pull request description: Two small fixups to bitcoin#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. (bitcoin#25880 (comment)) - Simplify a logging statement (suggested in bitcoin#25880 (comment)) ACKs for top commit: MarcoFalke: review ACK b2a1e47 🕧 Tree-SHA512: 337f0883bf1c94cc26301a80dfa649093ed1e211ddda1acad8449a2add5be44e5c12d6073c209df9c7aa1edb9da33ec1cfdcb0deafd76178ed78785843e80bc7
Summary: This makes the stalling detection mechanism (previously a fixed timeout of 2s) adaptive: If we disconnect a peer for stalling, double the timeout for the next peer - and let it slowly relax back to its default value each time the tip advances. (Idea by Pieter Wuille) This makes situations more unlikely in which we'd keep on disconnecting many of our peers for stalling, even though our own bandwidth is insufficient to download a block in 2 seconds. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> This is a backport of [ [[bitcoin/bitcoin#25880 | core#25880]] and [[bitcoin/bitcoin#26982 | core#26982]] Note that we need to specify `version=4` when calling `create_block` in the test because we miss [[bitcoin/bitcoin#16333 | core#16333]] (which make 4 the default version for create_block) Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D15080
Two small fixups to #25880:
Use
is_connectedinstead ofnum_test_p2p_connectionsto avoid intermittent failures where the p2p MiniNode got disconnected but this info hasn't made it to python yet, so it fails a ping. (p2p: Make stalling timeout adaptive during IBD #25880 (comment))Simplify a logging statement (suggested in p2p: Make stalling timeout adaptive during IBD #25880 (comment))