net: Use mockable time for ping/pong, add tests #18638

maflcko · 2020-04-14T19:11:37Z

Switch CNode::m_ping_start and CNetMessage::m_time to mockable time, so that tests can be added.

Mockable time is also type-safe, since it uses std::chrono

DrahtBot · 2020-04-14T20:24:14Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect() #19472 ([net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect() by jnewbery)
p2p: Move all header verification into the network layer, extend logging #19107 (p2p: Move all header verification into the network layer, extend logging by troygiorshev)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Coverage

Coverage	Change (pull 18638, `0f96c4b`)	Reference (master, `75fcfda`)
Lines	+0.0020 %	90.3582 %
Functions	+0.1431 %	85.0787 %
Branches	+0.0089 %	51.8202 %

^{Updated at: 2020-04-15T00:09:25.322904.}

maflcko · 2020-04-15T00:50:36Z

Coverage changes:

and

laanwj · 2020-05-04T13:22:12Z

Concept ACK.

Won't this potentially cause issues (e.g. ping timeouts) in P2P tests that already use mockable time for other things, but are not expecting it to have an effect on pings?

(seems not, given that all the tests still pass)

maflcko · 2020-05-04T13:42:29Z

The -peertimeout is still measured in wall clock time, and because most of the test run through faster than what the -peertimeout is, this should not lead to issues in general. There are two tests test/functional/feature_maxuploadtarget.py and test/functional/feature_bip68_sequence.py which mine a lot of blocks (more than 100) under mocktime. This can take time when the tests are run under valgrind, so I had to bump the -peertimeout for those tests.

Other tests should not be affected. If they are, it should be trivial to fix them up.

practicalswift · 2020-05-04T14:03:04Z

Concept ACK: mockable is better than non-mockable :)

jonatack

re-ACK fa94d6f changes per git range-diff b3ec1fe fabb382 fa94d6f are updates for a recent merge converting pointers to references in net processing code. Re-did clang and fuzz debug builds, re-ran the new test p2p_ping and the process_message and p2p_transport_deserializer fuzz tests locally.

jonatack · 2020-06-08T14:08:22Z

test/functional/p2p_ping.py

+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+PING_INTERVAL = 2 * 60


nit: if you retouch, could add a comment expressing the time units or along the lines of:

PING_INTERVAL = 2 * 60 # 2 minutes (corresponds to net_processing::PING_INTERVAL)

maflcko · 2020-06-19T11:51:38Z

Rebased. Should be easy to re-ACK with git range-diff bitcoin-core/master A B

jonatack

Code review re-ACK fa33654 re-read code, verified rebase per git range-diff 4b5c919 fa94d6f fa33654, previous tested ACKs still valid

fridokus · 2020-06-22T11:48:14Z

test/functional/p2p_ping.py

+
+        self.log.info('Check that ping is sent after connection is established')
+        no_pong_node = self.nodes[0].add_p2p_connection(NodeNoPong())
+        self.mock_forward(3)


This is peertimeout right? If yes, we use peertimeout at 3 places any maybe it's more clear to define it at the outermost scope as well like PING_INTERVAL? Or maybe there's no need :)

peertimeout is wall clock time, not mock time, I think. So this can be any value, as long as it matches pingwait=3 below, I'd say

fridokus · 2020-06-22T11:48:28Z

ACK faab4aa, ran functional tests and read the python scripts.

troygiorshev

ACK fa33654

Reviewed, verified everything runs.

I enjoyed how each sub-test flows into the next one. Hard to see at first but they really work together.

A few questions.

troygiorshev · 2020-06-24T00:43:41Z

test/functional/p2p_ping.py

+        self.log.info('Check that ping is sent after connection is established')
+        no_pong_node = self.nodes[0].add_p2p_connection(NodeNoPong())
+        self.mock_forward(3)
+        assert no_pong_node.last_message.pop('ping').nonce != 0


Why pop here, as opposed to just accessing the value? Seems like having (message_count['ping'] == 1 and 'ping' not in last_message) == True could be confusing.

Maybe decrementing message_count['ping'] every pop is an easier solution, being that this idea is used throughout the test.

Suggested change

assert no_pong_node.last_message.pop('ping').nonce != 0

assert no_pong_node.last_message['ping'].nonce != 0

Or maybe

Suggested change

assert no_pong_node.last_message.pop('ping').nonce != 0

assert no_pong_node.last_message.get('ping').nonce != 0

or

Suggested change

assert no_pong_node.last_message.pop('ping').nonce != 0

assert no_pong_node.last_message.pop('ping').nonce != 0

no_pong_node.message_count['ping'] -= 1

I wasn't aware that message_count exists, nor am I using it in this test. Given that it is almost unused right now and causes (potential) confusion, I'd say to remove it

the reason I use pop is to ensure the value is discarded after a read and has no way to poison follow-up test cases

troygiorshev · 2020-06-24T00:46:35Z

test/functional/p2p_ping.py

+        with self.nodes[0].assert_debug_log([
+                'pong peer=0: Unsolicited pong without ping, 0 expected, 0 received, 8 bytes',
+        ]):
+            no_pong_node.send_and_ping(msg_pong())


Why send_and_ping and not send_message, here and throughout? Doesn't seem like it would make a difference, and maybe it's best that we don't have extraneous pings and pongs.

I don't like to use assert_debug_log as a way to synchronize/poll for a change

send_and_ping is a way to flush all messages in the buffer (on both sides, send and receive) and ensure they are processed (and any errors have been logged).

Also, sending a ping and receiving a ping should never interfere, unless I am missing something obvious

troygiorshev · 2020-06-24T00:53:44Z

test/functional/p2p_ping.py

+from test_framework.messages import (
+    msg_pong,
+)


nit: single line for a single import?

Good point. Can be fixed in a follow-up

maflcko · 2020-07-10T14:05:59Z

Thanks for the review everyone. If there are ways to improve the test, I am happy to review them in a follow-up

fa33654 net: Use mockable time for ping/pong, add tests (MarcoFalke) faab4aa util: Add count_microseconds helper (MarcoFalke) Pull request description: Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added. Mockable time is also type-safe, since it uses `std::chrono` ACKs for top commit: jonatack: Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid troygiorshev: ACK fa33654 Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e

Summary: ``` Switch CNode::m_ping_start and CNetMessage::m_time to mockable time, so that tests can be added. Mockable time is also type-safe, since it uses std::chrono ``` Backport of core [[bitcoin/bitcoin#18638 | PR18638]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9129

maflcko changed the title ~~net: Make ping mockable, add tests~~ net: Use mocktime for ping/pong, add tests Apr 14, 2020

maflcko changed the title ~~net: Use mocktime for ping/pong, add tests~~ net: Use mockable time for ping/pong, add tests Apr 14, 2020

maflcko force-pushed the 2004-netMockPing branch 2 times, most recently from fa3076a to 5555f1c Compare April 14, 2020 19:35

DrahtBot added P2P Tests Utils/log/libs labels Apr 14, 2020

maflcko removed Tests Utils/log/libs labels Apr 14, 2020

maflcko force-pushed the 2004-netMockPing branch from 5555f1c to fa7bbfb Compare April 14, 2020 21:24

DrahtBot mentioned this pull request Apr 14, 2020

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

maflcko force-pushed the 2004-netMockPing branch 2 times, most recently from fafdfe8 to fa2edad Compare April 14, 2020 23:51

This was referenced Apr 15, 2020

net: Reduce TransportDeserializer interface to 2 methods #18644

Closed

Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479

Closed

Make g_chainman internal to validation #18698

Merged

This was referenced Apr 22, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

Refactor: Move consts to their correct translation units #17383

Merged

test: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer #18757

Merged

DrahtBot added the Needs rebase label Apr 24, 2020

maflcko force-pushed the 2004-netMockPing branch from fa2edad to fa21481 Compare April 25, 2020 12:44

DrahtBot removed the Needs rebase label Apr 25, 2020

maflcko force-pushed the 2004-netMockPing branch from fa21481 to fa3fcb4 Compare April 25, 2020 13:24

DrahtBot mentioned this pull request May 2, 2020

Serve BIP 157 compact filters #16442

Closed

DrahtBot mentioned this pull request May 5, 2020

Serve cfcheckpt requests #18877

Merged

jonatack reviewed Jun 8, 2020

View reviewed changes

DrahtBot removed the Needs rebase label Jun 8, 2020

This was referenced Jun 10, 2020

refactor: replace CConnman pointers by references in net_processing.cpp #19174

Merged

net: Avoid redundant and confusing FAILED log #19293

Merged

DrahtBot mentioned this pull request Jun 17, 2020

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

DrahtBot added the Needs rebase label Jun 19, 2020

MarcoFalke added 2 commits June 19, 2020 07:25

util: Add count_microseconds helper

faab4aa

net: Use mockable time for ping/pong, add tests

fa33654

maflcko force-pushed the 2004-netMockPing branch from fa94d6f to fa33654 Compare June 19, 2020 11:26

DrahtBot removed the Needs rebase label Jun 19, 2020

jonatack reviewed Jun 21, 2020

View reviewed changes

fridokus reviewed Jun 22, 2020

View reviewed changes

troygiorshev reviewed Jun 24, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 8, 2020

[net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect() #19472

Merged

maflcko merged commit 107b855 into bitcoin:master Jul 10, 2020

maflcko deleted the 2004-netMockPing branch July 10, 2020 14:40

troygiorshev mentioned this pull request Jul 24, 2020

test: Remove message_count #19580

Closed

kwvg mentioned this pull request Jul 13, 2021

merge #16117, #18358, #17383, #21052, #14424, #15159, #14689, #14978, partial #16908, #14978, #13932: Auxillary Backports dashpay/dash#4229

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021

merge bitcoin#18638: Use mockable time for ping/pong, add tests

f6d0b79

kwvg added a commit to kwvg/dash that referenced this pull request Jul 13, 2021

merge bitcoin#18638: Use mockable time for ping/pong, add tests

0a2e395

kwvg added a commit to kwvg/dash that referenced this pull request Jul 15, 2021

merge bitcoin#18638: Use mockable time for ping/pong, add tests

e119999

kwvg added a commit to kwvg/dash that referenced this pull request Jul 15, 2021

merge bitcoin#18638: Use mockable time for ping/pong, add tests

ee30f09

kwvg added a commit to kwvg/dash that referenced this pull request Jul 16, 2021

merge bitcoin#18638: Use mockable time for ping/pong, add tests

2360ac9

kwvg added a commit to kwvg/dash that referenced this pull request Aug 1, 2021

merge bitcoin#18638: Use mockable time for ping/pong, add tests

aa36b71

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	assert no_pong_node.last_message.pop('ping').nonce != 0
	assert no_pong_node.last_message['ping'].nonce != 0

	assert no_pong_node.last_message.pop('ping').nonce != 0
	assert no_pong_node.last_message.get('ping').nonce != 0

	assert no_pong_node.last_message.pop('ping').nonce != 0
	assert no_pong_node.last_message.pop('ping').nonce != 0
	no_pong_node.message_count['ping'] -= 1

net: Use mockable time for ping/pong, add tests #18638

net: Use mockable time for ping/pong, add tests #18638

Uh oh!

Conversation

maflcko commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Coverage

Uh oh!

maflcko commented Apr 15, 2020

Uh oh!

laanwj commented May 4, 2020

Uh oh!

maflcko commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented May 4, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 19, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fridokus commented Jun 22, 2020

Uh oh!

troygiorshev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 10, 2020

Uh oh!

Uh oh!

maflcko commented Apr 14, 2020 •

edited

Loading

DrahtBot commented Apr 14, 2020 •

edited

Loading

maflcko commented May 4, 2020 •

edited

Loading