net: Use mockable time for tx download #16197

MarcoFalke commented at 9:09 PM on June 12, 2019: member

Two commits:

First commit changes to mockable time for tx download (refactoring, should only have an effect on regtest)
Second commit adds a test that uses mocktime to test tx download

MarcoFalke commented at 9:10 PM on June 12, 2019: member

The test is from @sdaftuar, but heavily modified to use mocktime and less nodes. See https://github.com/sdaftuar/bitcoin/commit/db8fc5a2e25b6fcb08924e57f3a3a1acef6611ea

MarcoFalke force-pushed on Jun 12, 2019

in src/net_processing.cpp:3904 in facc98ad38 outdated

3899 | @@ -3900,6 +3900,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3900 |              connman->PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv));
3901 |  
3902 |          // Detect whether we're stalling
3903 | +        const auto current_time = GetTime<std::chrono::microseconds>();
3904 | +        // nNow is the current system time (not mockable) and should be removed eventually

instagibbs commented at 9:25 PM on June 12, 2019:

probably want to directly mention why nNow is mockable here

MarcoFalke commented at 10:08 PM on June 12, 2019:

It is not mockable

Added a comment

DrahtBot added the label P2P on Jun 12, 2019

DrahtBot added the label Tests on Jun 12, 2019

MarcoFalke force-pushed on Jun 12, 2019

MarcoFalke commented at 11:18 PM on June 12, 2019: member

@sipsorcery Any idea why this doesn't compile on msvc?

fanquake requested review from sdaftuar on Jun 13, 2019

sipsorcery commented at 6:17 AM on June 13, 2019: member

@sipsorcery Any idea why this doesn't compile on msvc? @MarcoFalke I'll be able to have a closer look later.

From a quick glance my first guess would be that if there's a clock involved gcc and msvc do things differently, see this SO question.

MarcoFalke commented at 11:09 AM on June 13, 2019: member

The build failure according to appveyor is

c:\projects\bitcoin\src\random.cpp(798): error C2893: Failed to specialize function template 'D GetRandTime(D) noexcept' [C:\projects\bitcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj]

I don't change anything how to deal with clocks. The only thing that changes is the type from long long to microseconds.

sipsorcery commented at 7:43 PM on June 13, 2019: member

@MarcoFalke adding noexcept to the template specialisation on line 675 fixes the msvc build for me.

675: template std::chrono::microseconds GetRandTime(std::chrono::microseconds) noexcept;

MarcoFalke commented at 8:01 PM on June 13, 2019: member

:pray: :heart: Thx

MarcoFalke force-pushed on Jun 13, 2019

MarcoFalke force-pushed on Jun 14, 2019

DrahtBot commented at 12:29 AM on June 17, 2019: member

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

Conflicts

No conflicts as of last run.

MarcoFalke force-pushed on Jun 17, 2019

Empact commented at 3:38 PM on June 17, 2019: member

Concept ACK

in src/net_processing.cpp:75 in fa77939624 outdated

  73 |  /** How long to wait (in microseconds) before downloading a transaction from an additional peer */
  74 | -static constexpr int64_t GETDATA_TX_INTERVAL = 60 * 1000000; // 1 minute
  75 | +static constexpr std::chrono::microseconds GETDATA_TX_INTERVAL{60 * 1000000}; // 1 minute
  76 |  /** Maximum delay (in microseconds) for transaction requests to avoid biasing some peers over others. */
  77 | -static constexpr int64_t MAX_GETDATA_RANDOM_DELAY = 2 * 1000000; // 2 seconds
  78 | +static constexpr std::chrono::microseconds MAX_GETDATA_RANDOM_DELAY{2 * 1000000}; // 2 seconds

Empact commented at 3:39 PM on June 17, 2019:

nit: Maybe a good time to added a std::chrono::microseconds second{1000000} declaration so these can be self-documenting? Alternatively, could declare them as std::chrono::seconds, std::chrono::minutes and allow the compiler to convert them on use.

MarcoFalke commented at 6:14 PM on June 17, 2019:

Good point. Removed the 1000000 factor and used compile time magic to derive it from std::chrono::seconds instead.

Empact commented at 7:57 PM on June 17, 2019:

This works as well fyi:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 7ed8c49c1..0b3219e55 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -67,14 +67,14 @@ static constexpr int HISTORICAL_BLOCK_AGE = 7 * 24 * 60 * 60;
 static constexpr int32_t MAX_PEER_TX_IN_FLIGHT = 100;
 /** Maximum number of announced transactions from a peer */
 static constexpr int32_t MAX_PEER_TX_ANNOUNCEMENTS = 2 * MAX_INV_SZ;
-/** How many microseconds to delay requesting transactions from inbound peers */
-static constexpr std::chrono::microseconds INBOUND_PEER_TX_DELAY{std::chrono::seconds{2}};
-/** How long to wait (in microseconds) before downloading a transaction from an additional peer */
-static constexpr std::chrono::microseconds GETDATA_TX_INTERVAL{std::chrono::seconds{60}};
-/** Maximum delay (in microseconds) for transaction requests to avoid biasing some peers over others. */
-static constexpr std::chrono::microseconds MAX_GETDATA_RANDOM_DELAY{std::chrono::seconds{2}};
-/** How long to wait (in microseconds) before expiring an in-flight getdata request to a peer */
-static constexpr std::chrono::microseconds TX_EXPIRY_INTERVAL{GETDATA_TX_INTERVAL * 10};
+/** How long to delay requesting transactions from inbound peers */
+static constexpr std::chrono::seconds INBOUND_PEER_TX_DELAY{2};
+/** How long to wait before downloading a transaction from an additional peer */
+static constexpr std::chrono::seconds GETDATA_TX_INTERVAL{60};
+/** Maximum delay for transaction requests to avoid biasing some peers over others. */
+static constexpr std::chrono::seconds MAX_GETDATA_RANDOM_DELAY{2};
+/** How long to wait before expiring an in-flight getdata request to a peer */
+static constexpr auto TX_EXPIRY_INTERVAL{GETDATA_TX_INTERVAL * 10};
 static_assert(INBOUND_PEER_TX_DELAY >= MAX_GETDATA_RANDOM_DELAY,
 "To preserve security, MAX_GETDATA_RANDOM_DELAY should not exceed INBOUND_PEER_DELAY");
 /** Limit to avoid sending big packets. Not used in processing incoming GETDATA for compatibility */

MarcoFalke commented at 9:05 PM on June 17, 2019:

I know, but it opted for the least difference from the previous code. Also seems safer to use microseconds in combination with GetRand (which wouldn't work with seconds)

MarcoFalke force-pushed on Jun 17, 2019

net: Use mockable time for tx download fa883ab35a

MarcoFalke force-pushed on Jun 17, 2019

jonatack commented at 6:28 PM on June 17, 2019: member

Concept ACK. Review in progress.

[qa] Test that getdata requests work as expected

We should eventually request a transaction from all peers that announce
it (assuming we never receive it).

We should prefer requesting from outbound peers over inbound peers.

Enforce the max tx requests in flight, and the eventual expiry of those
requests.

Test author:    Suhas Daftuar <sdaftuar@gmail.com>
Adjusted by:    MarcoFalke

fab3658356

MarcoFalke force-pushed on Jun 19, 2019

laanwj approved

laanwj commented at 1:59 PM on July 3, 2019: member

code review ACK 16197/commits/fab365835639a3da03f8ad9a58a0db6c6c4c2314 Changing the durations to std::chrono::* instead of raw integers is an improvement in itself

sdaftuar commented at 6:05 PM on July 19, 2019: member

Concept ACK -- I wasn't sure if others wanted a change like this when I first wrote this code, but I will try to review this since it seems now that others also think this is useful.

practicalswift commented at 11:12 AM on July 22, 2019: contributor

Concept ACK

jamesob approved

jamesob commented at 7:47 PM on August 1, 2019: member

ACK https://github.com/bitcoin/bitcoin/pull/16197/commits/fab365835639a3da03f8ad9a58a0db6c6c4c2314

First commit is just changing a bunch of int_64ts to std::chrono::microseconds and should be pretty uncontroversial. The second commit adds good test coverage around P2P tx request flows and rate-limiting. IMO worth merging for the added test coverage.

MarcoFalke merged this on Aug 5, 2019

MarcoFalke closed this on Aug 5, 2019

MarcoFalke referenced this in commit c77f7cdbd1 on Aug 5, 2019

MarcoFalke deleted the branch on Aug 5, 2019

sidhujag referenced this in commit bb197a14b2 on Aug 10, 2019

deadalnix referenced this in commit fc9835f6ae on May 20, 2020

DrahtBot locked this on Dec 16, 2021