test: Check that message sends successfully when header is split across two buffers #19304

troygiorshev · 2020-06-17T15:03:40Z

This PR is a tweak of #19302. This sends a valid message.

Additionally, this test includes logging in the same vein as #19272.

jonatack

Concept ACK

test/functional/p2p_invalid_messages.py

jonatack · 2020-06-17T15:36:04Z

test/functional/p2p_invalid_messages.py

            msg = msg_getdata([CInv(MSG_TX, 1)] * 50001)
            conn.send_and_ping(msg)
-        with self.nodes[0].assert_debug_log(['Misbehaving', 'peer=4 (40 -> 60): headers message size = 2001']):
+        with self.nodes[0].assert_debug_log(['Misbehaving', '(40 -> 60): headers message size = 2001']):


If #19272 if merged first, these changes in test_large_inv() won't be needed.

+1, I'm more than happy to rebase this one if #19272 goes in first

test/functional/p2p_invalid_messages.py

maflcko · 2020-06-17T15:41:37Z

ACK. Thanks for adding the test, will test after assert_equal nit is addressed

troygiorshev · 2020-06-17T16:13:07Z

Compare c5904c8 to 3cd538a. Changed to assert_equal and made log more descriptive. Disconnects peer at the end of the test, I'll keep that discussion in #19272 and I'll follow the status quo here.

Had to add a short sleep, as I was finding my RPC call occationally went before the first half of the message was received by the node.

test/functional/p2p_invalid_messages.py

glozow

Concept ACK, hi Troy 👋 😁 !
Came here because I made a suggestion that impacts this test in #19272 - oops. I definitely see the need to make sure all peers are disconnected since getnettotals aggregates information across all peers.

I wrote some suggestions that build off of Marco's comment about races.

Side note: not sure if you already considered this, but if there's no other peers, you can also use getpeerinfo to see information on pings received from conn only.

test/functional/p2p_invalid_messages.py

troygiorshev · 2020-06-17T19:21:30Z

Implemented wait_until as discussed.

A message can be broken across two buffers, with the split inside its header. Usually this will occur when sending many messages, such that the first buffer fills. This test uses the RPC to verify that the message is actually being received in two pieces. There is a very rare chance of a race condition where the test framework sends a message in between the two halves of the message under test. In this case the peer will almost certainly disconnect and the test will fail. An assert has been added to help debugging that rare case.

troygiorshev · 2020-06-17T19:23:42Z

And removed unused import.

maflcko · 2020-06-17T19:29:35Z

tested ACK 80d4423 (added an assert(false) to observe deterministic coverage) 🌦

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

tested ACK 80d4423f997e15780bfa3f91bf4b4bf656b8ea45 (added an assert(false) to observe deterministic coverage) 🌦
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg1fgwAr3kntpEQs2aiIVSBgAO5mxLPeIy/CfdZirlGL0VYvHOPl2BvcIwBOSXt
/mjQVNutfafKTcpxz5pvMMX934qEA0K49aX92hU2jt+q5q5zggMsBjd996Cwm6ww
J5+Zz3AJFWSU2CVHll0vHIjNolBBe+HnB5w8enUgdz8TPGf4mS1LWtS9uE36a0ja
Uor7i7wdN46jV+c35KXfH8gxVNyEirHlDAdDfQkHZpjdFBoMmEK4yML3PflEqkpy
CVoKNfVr7ziGfjUDAG2JNl7wSDSJtomvBRcBsSwP0h0lPBONjv6JntoLSjeTHaY2
wzx/mwB2lXx1QWw3E/VU/H1ivNiwcI5uIt8EdiVNhficdOUwhEgVGhI6TREpn8W1
DgriZXSNI0v9SDhWM8fWV7MBR6XElZC5m31pM4aqknD5+4gftrZxUiI7Hg4t4tVC
Gz/G1FEAo2icWAgmodUbSp+ti87Y2HhPi+9rDdvuHgWG2BPFviGO55tbLta14tiO
U7YBmVXz
=QULl
-----END PGP SIGNATURE-----

Timestamp of file with hash 1e2f468bf771db8cdab09da071ec81ccf7eb4d5341ad83699ce0369e01a45610 -

glozow

ACK 80d4423 👊

Looks good to me, nits if you touch again and happy to re-ACK if you do.

test/functional/p2p_invalid_messages.py

DrahtBot · 2020-06-17T20:17:17Z

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, test: invalid p2p messages and test framework improvements #19272 (test: p2p_invalid_messages and test framework improvements by jonatack)

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 19304, `1810233`)	Reference (master, `35ed88f`)
Lines	+0.0249 %	90.7991 %
Functions	+0.0818 %	85.6072 %
Branches	+0.0126 %	51.9831 %

^{Updated at: 2020-06-17T20:17:17.128362.}

practicalswift · 2020-06-17T21:45:06Z

Concept ACK: good to have this code path tested deterministically!

jonatack · 2020-06-18T05:32:42Z

test/functional/p2p_invalid_messages.py

@@ -42,13 +48,33 @@ def set_test_params(self):
        self.setup_clean_chain = True

    def run_test(self):
+        self.test_buffer()


This would allow not changing test_large_inv():

def run_test(self): - self.test_buffer() self.test_magic_bytes() self.test_checksum() self.test_size() self.test_msgtype() self.test_large_inv() + self.test_buffer() self.test_resource_exhaustion()

maflcko · 2020-06-18T11:37:22Z

Going to merge this. I think it is more important to have this test than it is to find the exact right location for this test. I picked an invalid message in #19302 to accomodate the file name, but if it really turns out to be problematic, one could add a comment like # This test can equivalently be done with an invalid or valid message, for simplicity it is done here with a valid message

…ovements 56010f9 test: hoist p2p values to test framework constants (Jon Atack) 75447f0 test: improve msg sends and p2p disconnections in p2p_invalid_messages (Jon Atack) 5796019 test: refactor test_large_inv() into 3 tests with common method (Jon Atack) e2b21d8 test: add p2p_invalid_messages logging (Jon Atack) 9fa494d net: update misbehavior logging for oversized messages (Jon Atack) Pull request description: ...seen while reviewing #19264, #19252, #19304 and #19107: in `net_processing.cpp` - make the debug logging for oversized message size misbehavior the same for `addr`, `getdata`, `headers` and `inv` messages in `p2p_invalid_messages` - add missing logging - improve assertions/message sends, move cleanup disconnections outside the assertion scopes - split a slowish 3-part test into 3 order-independent tests - add a few p2p constants to the test framework ACKs for top commit: troygiorshev: reACK 56010f9 MarcoFalke: ACK 56010f9 🎛 Tree-SHA512: db67b70278f8d4c318907e105af54b54eb3afd15500f9aa0c98034f6fd4bd1cf9ad1663037bd9b237ff4890f3059b37291a6498d8d6ae2cc38efb9f045f73310

…der is split across two buffers Summary: 80d4423f997e15780bfa3f91bf4b4bf656b8ea45 Test buffered valid message (Troy Giorshev) Pull request description: This PR is a tweak of #19302. This sends a valid message. Additionally, this test includes logging in the same vein as #19272. --- Depends on D9513 Backport of [[bitcoin/bitcoin#19304 | core#19304]] Test Plan: ninja all && ./test/functional/test_runner.py p2p_invalid_messages Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9514

fanquake added the Tests label Jun 17, 2020

troygiorshev mentioned this pull request Jun 17, 2020

test: Check that peer is disconnected for bad buffered message #19302

Closed

jonatack reviewed Jun 17, 2020

View reviewed changes

test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved

test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved

jonatack reviewed Jun 17, 2020

View reviewed changes

jnewbery reviewed Jun 17, 2020

View reviewed changes

test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved

test/functional/p2p_invalid_messages.py Show resolved Hide resolved

jnewbery mentioned this pull request Jun 17, 2020

net, test: invalid p2p messages and test framework improvements #19272

Merged

troygiorshev force-pushed the 2020-06-test-partial branch 2 times, most recently from cf742ce to 3cd538a Compare June 17, 2020 16:07

maflcko reviewed Jun 17, 2020

View reviewed changes

test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved

glozow reviewed Jun 17, 2020

View reviewed changes

test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved

test/functional/p2p_invalid_messages.py Show resolved Hide resolved

troygiorshev force-pushed the 2020-06-test-partial branch 2 times, most recently from 0cf655e to cac346c Compare June 17, 2020 19:20

maflcko reviewed Jun 17, 2020

View reviewed changes

test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved

troygiorshev force-pushed the 2020-06-test-partial branch from cac346c to 80d4423 Compare June 17, 2020 19:23

glozow reviewed Jun 17, 2020

View reviewed changes

test/functional/p2p_invalid_messages.py Show resolved Hide resolved

test/functional/p2p_invalid_messages.py Show resolved Hide resolved

test/functional/p2p_invalid_messages.py Show resolved Hide resolved

jonatack reviewed Jun 18, 2020

View reviewed changes

maflcko merged commit 343c0bf into bitcoin:master Jun 18, 2020

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

test: Check that message sends successfully when header is split across two buffers #19304

test: Check that message sends successfully when header is split across two buffers #19304

Uh oh!

Conversation

troygiorshev commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

troygiorshev Jun 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jun 17, 2020

Uh oh!

troygiorshev commented Jun 17, 2020

Uh oh!

Uh oh!

glozow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

troygiorshev commented Jun 17, 2020

Uh oh!

troygiorshev commented Jun 17, 2020

Uh oh!

maflcko commented Jun 17, 2020

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Coverage

Uh oh!

practicalswift commented Jun 17, 2020

Uh oh!

jonatack Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 18, 2020

Uh oh!

Uh oh!

troygiorshev commented Jun 17, 2020 •

edited

Loading

glozow left a comment •

edited

Loading

DrahtBot commented Jun 17, 2020 •

edited

Loading