p2p: add PoissonNextSend method that returns mockable time #17243

amitiuttarwar commented at 7:52 PM on October 24, 2019: contributor

Introduce a Poisson helper method that wraps the existing method to return std::chrono::duration type, which is mockable.

Needed for #16698.

fanquake added the label P2P on Oct 24, 2019

in src/net.h:949 in b1a523b006 outdated

 948 | -
 949 | -
 950 |  /** Return a timestamp in the future (in microseconds) for exponentially distributed events. */
 951 |  int64_t PoissonNextSend(int64_t now, int average_interval_seconds);
 952 |  
 953 | +/** Wrapper to return mockable type */

MarcoFalke commented at 7:54 PM on October 24, 2019:

/** Return a timestamp in the future for exponentially distributed events. */

amitiuttarwar commented at 8:00 PM on October 31, 2019:

I'd rather have the comment draw attention to the fact that its just calling through to the other Poisson method. Esp with the other function signature & comment right there.

fanquake renamed this:
~~tools: add PoissonNextSend method that returns mockable time~~
p2p: add PoissonNextSend method that returns mockable time
on Oct 28, 2019

fanquake requested review from MarcoFalke on Oct 28, 2019

MarcoFalke approved

MarcoFalke commented at 8:10 PM on October 29, 2019: member

Please switch nNextInvSend to the new type and use the new function there. Also, a unit test to assert that the two methods return the same value would be nice.

MarcoFalke added the label Refactoring on Oct 29, 2019

amitiuttarwar commented at 10:47 PM on October 30, 2019: contributor

@MarcoFalke updated nNextInvSend. Not sure how to unit test since the result of the poisson method would be different for two separate calls. LMK if you want deeper changes with chrono types, happy to update.

MarcoFalke commented at 2:42 AM on October 31, 2019: member

You can set g_mock_deterministic_tests = true; for just your test case, then set it to false again at the end.

amitiuttarwar force-pushed on Oct 31, 2019

amitiuttarwar commented at 12:12 AM on November 1, 2019: contributor

@MarcoFalke added tests. Ready for review.

MarcoFalke commented at 8:10 PM on November 1, 2019: member

ACK d504d70e602bf30aad18e1e5677daa3961757bb7

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash e887cddc7cfb3c4288a10e649645fa1687a1de16f54a8b1819200b2eaa1c86a8 -

</details>

in src/net_processing.cpp:3551 in d504d70e60 outdated

3547 | @@ -3548,6 +3548,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3548 |  
3549 |          // Address refresh broadcast
3550 |          int64_t nNow = GetTimeMicros();
3551 | +        auto current_time = GetTime<std::chrono::microseconds>();

ariard commented at 8:57 PM on November 1, 2019:

I think you can scope definition to inventory code part, would make code easier to read and also don't need to change drop constness of other current_time

amitiuttarwar commented at 5:14 AM on November 3, 2019:

tried to introduce scoping with { } around the inventory code but found that fFetch is assigned between the two definitions of current_time and accessed after. is there a way to scope independently?

ariard commented at 6:21 PM on November 4, 2019:

This compile : https://github.com/ariard/bitcoin/commit/1c1765e636db2c9484d10712596754dd774ed7bd, or are you describing a behavior change related with fFetch than I miss ?

amitiuttarwar commented at 3:44 PM on November 5, 2019:

ah gotcha. I was hesitant to move the declaration of current_time bc of potential change in logic based on how long code execution between the two spots would take. maybe its ok but I didn't dig into it / want to make the PR harder to review.

I'm going to leave it as is but thanks for the suggestion & clarification

ariard commented at 9:01 PM on November 1, 2019: member

Can you explain in commit message why you switch for mockable time for outbound peers announcement ? I mean how it's going to be used in #16698

in src/net.h:765 in d504d70e60 outdated

 761 | @@ -762,7 +762,7 @@ class CNode
 762 |          bool fSendMempool GUARDED_BY(cs_tx_inventory){false};
 763 |          // Last time a "MEMPOOL" request was serviced.
 764 |          std::atomic<std::chrono::seconds> m_last_mempool_req{std::chrono::seconds{0}};
 765 | -        int64_t nNextInvSend{0};
 766 | +        std::chrono::microseconds nNextInvSend{std::chrono::microseconds{0}};

ajtowns commented at 2:54 PM on November 4, 2019:

std::chrono::microseconds nNextInvsend{0}; seems like it would be fine?

in src/test/net_tests.cpp:314 in d504d70e60 outdated

 309 | +
 310 | +    int64_t now = 5000;
 311 | +    int average_interval_seconds = 600;
 312 | +
 313 | +    std::chrono::microseconds now_chrono = std::chrono::microseconds{now};
 314 | +    std::chrono::seconds interval_chrono = std::chrono::seconds{average_interval_seconds};

ajtowns commented at 3:04 PM on November 4, 2019:

std::chrono::microseconds now_chrono{now}; likewise, or could just say ::PoissonNextSend(std::chrono::microseconds{now}, std::chrono::seconds{average_interval_seconds})

ajtowns approved

ajtowns commented at 3:32 PM on November 4, 2019: member

ACK d504d70e602bf30aad18e1e5677daa3961757bb7 nits only

ariard commented at 6:22 PM on November 4, 2019: member

ACK d504d70, checked directly on parent PR the rational of intended changes

MarcoFalke commented at 6:25 PM on November 4, 2019: member

@amitiuttarwar This has three ACKs, so is ready to go. Do you want to address the nits or leave them for later?

naumenkogs commented at 6:43 PM on November 4, 2019: member

ACK d504d70 willing to do quick re-ACK if those tiny nits are resolved :)

[tools] add PoissonNextSend method that returns mockable time 4de630354f

[tools] update nNextInvSend to use mockable time 1a8f0d5a74

amitiuttarwar force-pushed on Nov 5, 2019

ajtowns commented at 2:41 PM on November 5, 2019: member

ACK 1a8f0d5a74d5cc0000456932babf35301f5c1686

MarcoFalke commented at 3:26 PM on November 5, 2019: member

re-ACK 1a8f0d5a74d5cc0000456932babf35301f5c1686

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash c18dd002e27bda4c5087dbc652b61c6e0ece0bbd73a3416548d7cf0624aca3fb -

</details>

naumenkogs commented at 4:22 PM on November 5, 2019: member

Is it a good idea to have both nNow and current_time? This might lead to counter-intuitive consequences, specifically in the test cases (I don't have the exact example right now).

amitiuttarwar commented at 4:58 PM on November 5, 2019: contributor

@naumenkogs I'd like to work on replacing nNow in SendMessages entirely with current_time, but haven't prioritized yet bc I'm focused on rebroadcast stuff.

Are your concerns limited to the test case introduced or do you also have concerns about the code path? I don't see how problems could occur, so I'd love if you could explain further?

ah from IRL convo with AJ, he pointed out how it can be confusing from tests based on what functionality executes based on clock time vs needing to update mocktime. Is that what you're alluding to?

naumenkogs commented at 5:36 PM on November 5, 2019: member

tests based on what functionality executes based on clock time vs needing to update mocktime. Is that what you're alluding to?

Yes. Also, potentially future code in net_processing. At the same time, I guess it blocks you from proceeding with rebroadcast (even in terms of better testing) today.

ACK 1a8f0d5, and let's merge it and come back to it later.

MarcoFalke referenced this in commit c7e6b3b343 on Nov 5, 2019

MarcoFalke merged this on Nov 5, 2019

MarcoFalke closed this on Nov 5, 2019

amitiuttarwar deleted the branch on Nov 5, 2019

codablock referenced this in commit 1cd6797373 on Apr 9, 2020

codablock referenced this in commit a26a2d82cf on Apr 12, 2020

codablock referenced this in commit debd93a84c on Apr 12, 2020

codablock referenced this in commit 02ee99718a on Apr 12, 2020

codablock referenced this in commit c52c1e1ce5 on Apr 14, 2020

deadalnix referenced this in commit 73791ff552 on Nov 4, 2020

ckti referenced this in commit fb46609fd9 on Mar 28, 2021

random-zebra referenced this in commit 668bc000c3 on Jul 15, 2021

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

MarcoFalke locked this on Dec 16, 2021