Rename and move PoissonNextSend functions #24021

mzumsande commented at 3:14 PM on January 10, 2022: member

PoissonNextSend and PoissonNextSendInbound are 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

moves PoissonNextSend() out of net to random and renames it to GetExponentialRand()
moves PoissonNextSendInbound() out of CConnman to PeerManager and renames it to NextInvToInbounds()
adds documentation for these functions

This is work by jnewbery - due to him being less active currently, I opened the PR and will address feedback.

in src/random.cpp:721 in 35cf6e4ebb outdated

 714 | @@ -714,3 +715,10 @@ void RandomInit()
 715 |  
 716 |      ReportHardwareRand();
 717 |  }
 718 | +
 719 | +std::chrono::microseconds GetExponentialRand(std::chrono::microseconds now, std::chrono::seconds average_interval)
 720 | +{
 721 | +    double unscaled = -std::log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */);

MarcoFalke commented at 3:37 PM on January 10, 2022:

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?

mzumsande commented at 2:58 PM on January 13, 2022:

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.

DrahtBot added the label P2P on Jan 10, 2022

jnewbery commented at 4:14 PM on January 10, 2022: member

Concept ACK. Thanks for taking this over, @mzumsande!

hebasto commented at 4:30 PM on January 10, 2022: member

the resulting random timestamps don't follow a Poisson distribution but an exponential distribution

True.

Concept ACK.

in src/net_processing.cpp:839 in 35cf6e4ebb outdated

 836 | +{
 837 | +    AssertLockHeld(cs_main);
 838 | +    if (m_next_inv_to_inbounds < now) {
 839 | +        // If this function were called from multiple threads simultaneously
 840 | +        // it would possible that both update the next send variable, and return a different result to their caller.
 841 | +        // This is not possible in practice as only the net processing thread invokes this function.

jnewbery commented at 4:34 PM on January 10, 2022:

Oops. I should have removed this comment since it's obvious from the function signature and the assert above that cs_main is always held when reading/writing this data.

mzumsande commented at 2:59 PM on January 13, 2022:

Thanks - removed.

jnewbery commented at 4:35 PM on January 10, 2022: member

ACK 35cf6e4ebb550dda6c3ef7853090ace2560af877

mzumsande force-pushed on Jan 13, 2022

[move] Move PoissonNextSend to src/random and update comment

PoissonNextSend is used by net and net_processing and is stateless, so
place it in the utility random.cpp translation unit.

9e64d69bf7

[refactor] Use uint64_t and std namespace in PoissonNextSend

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

03cfa1b603

scripted-diff: replace PoissonNextSend with GetExponentialRand

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-

bb060746df

[net processing] Move PoissonNextSendInbound to PeerManager ea99f5d01e

[net processing] Rename PoissonNextSendInbound to NextInvToInbounds 9b8dcb25b5

mzumsande force-pushed on Jan 13, 2022

jnewbery commented at 6:23 PM on January 13, 2022: member

utACK 4ab99133c4f72db57d80c0c6f5a6ab1f2b736f87

in src/random.cpp:719 in 4ab99133c4 outdated

 714 | @@ -714,3 +715,9 @@ void RandomInit()
 715 |  
 716 |      ReportHardwareRand();
 717 |  }
 718 | +
 719 | +std::chrono::microseconds GetExponentialRand(std::chrono::microseconds now, std::chrono::seconds average_interval)

hebasto commented at 7:56 PM on January 15, 2022:

While here why not

std::chrono::microseconds GetExponentialRand(std::chrono::microseconds now, std::chrono::microseconds average_interval)

and drop the following std::chrono::duration_cast?

mzumsande commented at 4:19 PM on January 19, 2022:

I don't think that it is possible: The duration cast is there not because of the type of average_interval, but because we do floating-point arithmetic inside of GetExponentialRand(): The expression unscaled * average_interval + 0.5us is of type std::chrono::duration<long double, std::micro>, no matter whether average_interval is of type std::chrono::seconds or std::chrono::microseconds.

So in order to convert to the integer-type std::chrono::microseconds, a std::chrono::duration_cast is necessary anyway.

hebasto commented at 9:25 PM on January 19, 2022:

You're right. Sorry.

hebasto commented at 7:59 PM on January 15, 2022: member

Approach ACK 4ab99133c4f72db57d80c0c6f5a6ab1f2b736f87.

I verified "move" commits with git diff ... --color-moved=dimmed-zebra.

My only concerns are about the last commit 4ab99133c4f72db57d80c0c6f5a6ab1f2b736f87 "[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)

mzumsande force-pushed on Jan 19, 2022

mzumsande commented at 4:33 PM on January 19, 2022: member

My only concerns are about the last commit 4ab9913 "[net processing] Guard NextInvToInbounds and m_next_inv_to_inbounds with cs_main":

I pushed an update, dropping the last commit (which changed the locking from std::atomic to using cs_main).

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.

theStack commented at 6:30 PM on January 19, 2022: member

Concept ACK

tiny nit: now that the call to (std::)log1p has moved to src/random.cpp, I think including math.h in src/net.cpp should not be needed anymore and can be removed (though I may have overlooked something).

hebasto approved

hebasto commented at 9:29 PM on January 19, 2022: member

ACK 9b8dcb25b57ad31b77c9f37d9a1f5b07dc6378b4, I have reviewed the code and it looks OK, I agree it can be merged.

jnewbery commented at 3:56 PM on January 20, 2022: member

ACK 9b8dcb25b5

Only difference from previous iteration is that the final commit has been dropped.

theStack approved

theStack commented at 10:08 PM on January 22, 2022: member

ACK 9b8dcb25b57ad31b77c9f37d9a1f5b07dc6378b4 📊

fanquake merged this on Jan 23, 2022

fanquake closed this on Jan 23, 2022

sidhujag referenced this in commit 38df8d3d54 on Jan 23, 2022

DrahtBot locked this on Jan 23, 2023

mzumsande deleted the branch on Jun 23, 2023