Rename and move PoissonNextSend functions #24021
Conversation
src/random.cpp
Outdated
|
|
||
| std::chrono::microseconds GetExponentialRand(std::chrono::microseconds now, std::chrono::seconds average_interval) | ||
| { | ||
| double unscaled = -std::log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */); |
There was a problem hiding this comment.
Is 1ULL guaranteed to be of the same width as uint64_t{1}? Even if they are, maybe still replace it while touching this line?
Also, the function could be moved closer to GetRand*Duration, to bundle related calls?
There was a problem hiding this comment.
I wasn't able to find systems where it wouldn't be the same width, but I don't think there is a guarantee. Changed it to uint64_t{1}.
Also moved the function up in random.h as suggested.
|
Concept ACK. Thanks for taking this over, @mzumsande! |
True. Concept ACK. |
35cf6e4 to
d74acdd
Compare
PoissonNextSend is used by net and net_processing and is stateless, so place it in the utility random.cpp translation unit.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
This distribution is used for more than just the next inv send, so make
the name more generic.
Also rename to "exponential" to avoid the confusion that this is a
poisson distribution.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1" ./src) ; }
ren PoissonNextSend GetExponentialRand
ren "a poisson timer" "an exponential timer"
-END VERIFY SCRIPT-
d74acdd to
4ab9913
Compare
|
utACK 4ab9913 |
There was a problem hiding this comment.
Approach ACK 4ab9913.
I verified "move" commits with git diff ... --color-moved=dimmed-zebra.
My only concerns are about the last commit 4ab9913 "[net processing] Guard NextInvToInbounds and m_next_inv_to_inbounds with cs_main":
- "No need for an atomic, since cs_main is held anyway when accessing this data" -- does not look like a decent justification
- it seems suboptimal to increase involvement of the
::cs_main, i.e., the "validation" lock, into P2P code - it makes a scope of the PR wider (other commits do not touch multithreading stuff)
4ab9913 to
9b8dcb2
Compare
I pushed an update, dropping the last commit (which changed the locking from While I think that if this had been new code (instead of being moved from net), we likely wouldn't have used an atomic, I agree that it's better if the focus of this PR is smaller and does not include locking changes. |
|
ACK 9b8dcb2 Only difference from previous iteration is that the final commit has been dropped. |
PoissonNextSendandPoissonNextSendInboundare used in the p2p code to obfuscate various regularly occurring processes, in order to make it harder for others to get timing-based information deterministically.The naming of these functions has been confusing to several people (including myself, see also #23347) because the resulting random timestamps don't follow a Poisson distribution but an exponential distribution (related to events in a Poisson process, hence the name). This PR
PoissonNextSend()out ofnettorandomand renames it toGetExponentialRand()PoissonNextSendInbound()out ofCConnmantoPeerManagerand renames it toNextInvToInbounds()This is work by jnewbery - due to him being less active currently, I opened the PR and will address feedback.