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

jonatack · 2020-06-14T11:48:06Z

...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

test/functional/p2p_invalid_messages.py

DrahtBot · 2020-06-14T18:01:39Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Add hash_type MUHASH for gettxoutsetinfo #19145 (Add hash_type options for gettxoutsetinfo by fjahr)
p2p: Move all header verification into the network layer, extend logging #19107 (p2p: Refactor, move all header verification into the network layer, without changing behavior by troygiorshev)
Use std::chrono for the time to rotate destination of addr messages + tests #18642 (Use std::chrono for the time to rotate destination of addr messages + tests by naumenkogs)

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.

troygiorshev · 2020-06-16T02:51:37Z

ACK 6b18bfe

troygiorshev

Reviewed and ran all tests. Just one suggestion if you plan on making any changes.

test/functional/p2p_invalid_messages.py

jonatack · 2020-06-16T08:10:03Z

Thanks for reviewing @troygiorshev. Took your suggestion and also removed an unnecessary disconnect_p2ps() call in test_oversized_msg().

glozow

Concept ACK
Mentioned this elsewhere but my edit is already outdated. Small suggestion for p2p_invalid_messages.py, feel free to ignore:
If you replace the 3 (specifically, test_checksum, test_size, and test_msgtype) self.nodes[0].p2p with conn, all of the subtest disconnect_p2ps can also be removed. The only reason they need it now is to clear p2ps.

jonatack · 2020-06-17T10:14:47Z

Thank you for reviewing, @gzhao408. Took your suggestion and credited you as co-author in the commit.

jnewbery · 2020-06-17T15:57:39Z

I don't think we should be removing the disconnect_p2ps calls at the end of each subtest. Ideally the subtest finishes by leaving the node and fixture in as close a state to they were in originally. See #19304 (comment)

glozow · 2020-06-17T16:53:13Z

@jnewbery good point...

Ideally the subtest finishes by leaving the node and fixture in as close a state to they were in originally

In terms of state, I'm thinking state = (1) the test_node.p2ps array and (2) the connections between the node and its peers.

(1) I think it's ok if the array has an extra entry if we're not using the .p2p property? Would you agree? If not, then yeah, we need to put disconnect_p2ps() back 😄 .

(2) For connections: Definitely agree that subtests should disconnect peers to clean up after themselves. Either node.disconnect_p2ps or peer.peer_disconnect Some of the subtests call wait_for_disconnect which makes sure that they are disconnected from the peer's POV (no peer._transport). With #19252,disconnect_p2ps waits for node's POV (not in node.getpeerinfo()).

I'm not sure if disconnected from peer's POV == disconnected from the node's POV. If they're effectively the same, I'd say it's unnecessary to call disconnect_p2ps after wait_for_disconnect.

I might be overthinking this haha, I dug into this when I thought superfluous disconnect_p2ps would cause performance problems 😅 .

jonatack · 2020-06-18T04:53:24Z

AFAICT the tests are order-independent with this PR, and I checked that this passes when rebased to current master:

+from test_framework.util import assert_equal
  
+    def assert_no_connected_peers(self):
+        assert_equal(self.nodes[0].num_connected_mininodes(), 0)
+
     def run_test(self):
         self.test_magic_bytes()
+        self.assert_no_connected_peers()
         self.test_checksum()
+        self.assert_no_connected_peers()
         self.test_size()
+        self.assert_no_connected_peers()
         self.test_msgtype()
+        self.assert_no_connected_peers()
         self.test_oversized_inv_msg()
+        self.assert_no_connected_peers()
         self.test_oversized_getdata_msg()
+        self.assert_no_connected_peers()
         self.test_oversized_headers_msg()
+        self.assert_no_connected_peers()
         self.test_resource_exhaustion()
+        self.assert_no_connected_peers()

jonatack · 2020-06-18T05:14:39Z

Anyway, removed the removal of disconnect_p2ps, and rebased to master to be sure all is well with the latest version of disconnect_p2ps. Edit: Improved the logging and the assertions with new ideas after reviewing #19107.

… header is split across two buffers 80d4423 Test buffered valid message (Troy Giorshev) Pull request description: This PR is a tweak of bitcoin#19302. This sends a valid message. Additionally, this test includes logging in the same vein as bitcoin#19272. ACKs for top commit: MarcoFalke: tested ACK 80d4423 (added an assert(false) to observe deterministic coverage) 🌦 gzhao408: ACK bitcoin@80d4423 👊 Tree-SHA512: 3b1aa5ec480a1661917354788923d64595e2886448c9697ec0606a81293e8b4a4642b2b3cc9afb2206ce6f74e5c6d687308c5ad19cb73c5b354d3071ad8496f8

jonatack · 2020-06-18T11:47:06Z

@MarcoFalke several PRs on this test have been merged but no approval from you on this one. I think they are good improvements but feel free to close it if you disagree; it will save me from spinning my wheels.

maflcko · 2020-06-18T11:56:48Z

I generally use the conflicts list of DrahtBot to see which pr has the most reviews and is the most ready to merge and then get them in one by one if they have received their ACKs.

Will take another look here. One sec

maflcko

Fine with me

test/functional/p2p_invalid_messages.py

jonatack · 2020-06-18T12:57:04Z

Rebased for merged #19304, reverted the chain setting, and removed a line left over from the last change (thanks Marco).

-MSG_LIMIT = 4 * 1000 * 1000  # 4MB, per MAX_PROTOCOL_MESSAGE_LENGTH
VALID_DATA_LIMIT = MAX_PROTOCOL_MESSAGE_LENGTH - 5  # Account for the 5-byte length prefix

-        self.setup_clean_chain = False
+        self.setup_clean_chain = True

jnewbery

Nice changes, Jon. I've left a few minor comments inline.

src/net_processing.cpp

test/functional/p2p_invalid_messages.py

troygiorshev · 2020-06-18T17:44:26Z

reACK ca65836

Reviewed and stepped through each test. Verified that none of these changes caused linting errors.

maflcko

c6e7519647ef8736aaa5d8e7f804d0340a36029a

test/functional/p2p_invalid_messages.py

troygiorshev · 2020-06-18T21:11:30Z

reACK 98af0f0

so that oversized ADDR, GETDATA, HEADERS and INV messages print the same consistent debug logs.

- call disconnect_p2ps() outside of the assert_debug_log scopes - send messages directly from the p2p conn rather than via nodes[0].p2p - add an assertion

jonatack · 2020-06-22T14:10:07Z

Rebased, no changes. Thank you for ACKing @troygiorshev! Should be trivial to re-ack with git range-diff 8ef15e8 98af0f0 56010f9

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

reACK 56010f9

rajarshimaitra

Concept ACK. Nice changes, easy to review. Compiled without issue, verified all unit and functions tests passing.
Below are a nonblocking nit and a question for explanation.

rajarshimaitra · 2020-06-24T14:24:33Z

test/functional/p2p_invalid_messages.py

@@ -24,8 +27,7 @@
    wait_until,
 )

-MSG_LIMIT = 4 * 1000 * 1000  # 4MB, per MAX_PROTOCOL_MESSAGE_LENGTH
-VALID_DATA_LIMIT = MSG_LIMIT - 5  # Account for the 5-byte length prefix
+VALID_DATA_LIMIT = MAX_PROTOCOL_MESSAGE_LENGTH - 5  # Account for the 5-byte length prefix


Probably I am missing something, but this 5 bytes message length seems to be coming from ser_string() in messages.py. But is it something that the node also recognizes? It doesn't seem to be part of the p2p protocol.

We set VALID_DATA_LIMIT so that the length of the serialised msg_unrecognized message built with it will equal MAX_PROTOCOL_MESSAGE_LENGTH. This is verified at line 153; you can also add the following assertion at line 111

msg = msg_unrecognized(str_data="d" * (VALID_DATA_LIMIT + 1)) + assert_equal(len(msg.serialize()), MAX_PROTOCOL_MESSAGE_LENGTH + 1)

or try changing the offset from 5 to 4 or 6.

I've added this assertion and some other ones while testing @troygiorshev's net message headers refactoring and may propose the changes if they look worthwhile to me after a few days' time.

test/functional/p2p_invalid_messages.py

maflcko · 2020-06-24T19:54:01Z

ACK 56010f9 🎛

Show signature and timestamp

Signature:

Timestamp of file with hash 937d2e5bea5f5ff38ae343e4664be89319c7ac06458b75aca1a579bbd3abbcd1 -

…mprovements Summary: 56010f92564a94b0ca6c008c0e6f74a19fad4a2a test: hoist p2p values to test framework constants (Jon Atack) 75447f0893f9ad9bf83d182b301d139430d8de1c test: improve msg sends and p2p disconnections in p2p_invalid_messages (Jon Atack) 57960192a5362ff1a7b996995332535f4c2a25c3 test: refactor test_large_inv() into 3 tests with common method (Jon Atack) e2b21d8a597c536a8617408d43958bfe9f98a442 test: add p2p_invalid_messages logging (Jon Atack) 9fa494dc0969c61d5ef33708a08923cca19ce091 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 --- Backport of [[bitcoin/bitcoin#19272 | core#19272]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9530

maflcko reviewed Jun 14, 2020

View reviewed changes

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

DrahtBot added the Tests label Jun 14, 2020

jonatack force-pushed the improve-p2p_invalid_messages branch from bc42270 to 6b18bfe Compare June 14, 2020 19:48

DrahtBot mentioned this pull request Jun 14, 2020

Add hash_type MUHASH for gettxoutsetinfo #19145

Merged

jonatack mentioned this pull request Jun 15, 2020

test: wait for disconnect in disconnect_p2ps + bloomfilter test followups #19252

Merged

troygiorshev approved these changes Jun 16, 2020

View reviewed changes

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

jonatack force-pushed the improve-p2p_invalid_messages branch from 6b18bfe to beecd6b Compare June 16, 2020 08:08

This was referenced Jun 16, 2020

p2p: Move all header verification into the network layer, extend logging #19107

Merged

net: Avoid redundant and confusing FAILED log #19293

Merged

glozow reviewed Jun 16, 2020

View reviewed changes

jonatack force-pushed the improve-p2p_invalid_messages branch from beecd6b to 2ecb8f7 Compare June 17, 2020 10:09

troygiorshev mentioned this pull request Jun 17, 2020

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

Merged

jonatack force-pushed the improve-p2p_invalid_messages branch from 2ecb8f7 to 06a275f Compare June 18, 2020 05:12

jonatack force-pushed the improve-p2p_invalid_messages branch from 06a275f to efb9a95 Compare June 18, 2020 10:36

maflcko reviewed Jun 18, 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 force-pushed the improve-p2p_invalid_messages branch from efb9a95 to 255fbca Compare June 18, 2020 12:28

jnewbery reviewed Jun 18, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

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

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

jonatack force-pushed the improve-p2p_invalid_messages branch from 255fbca to 11c1184 Compare June 18, 2020 16:46

jonatack force-pushed the improve-p2p_invalid_messages branch from 11c1184 to ca65836 Compare June 18, 2020 17:25

jonatack changed the title ~~test: p2p_invalid_messages and test framework improvements~~ net, test: p2p_invalid_messages and test framework improvements Jun 18, 2020

jonatack changed the title ~~net, test: p2p_invalid_messages and test framework improvements~~ net, test: invalid p2p messages and test framework improvements Jun 18, 2020

maflcko suggested changes Jun 18, 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 force-pushed the improve-p2p_invalid_messages branch 2 times, most recently from 0730884 to 98af0f0 Compare June 18, 2020 20:39

DrahtBot mentioned this pull request Jun 19, 2020

Use std::chrono for the time to rotate destination of addr messages + tests #18642

Merged

DrahtBot added the Needs rebase label Jun 19, 2020

jonatack added 5 commits June 19, 2020 14:14

net: update misbehavior logging for oversized messages

9fa494d

so that oversized ADDR, GETDATA, HEADERS and INV messages print the same consistent debug logs.

test: add p2p_invalid_messages logging

e2b21d8

test: refactor test_large_inv() into 3 tests with common method

5796019

test: improve msg sends and p2p disconnections in p2p_invalid_messages

75447f0

- call disconnect_p2ps() outside of the assert_debug_log scopes - send messages directly from the p2p conn rather than via nodes[0].p2p - add an assertion

test: hoist p2p values to test framework constants

56010f9

jonatack force-pushed the improve-p2p_invalid_messages branch from 98af0f0 to 56010f9 Compare June 19, 2020 12:28

DrahtBot removed the Needs rebase label Jun 19, 2020

jonatack requested a review from maflcko June 23, 2020 04:36

rajarshimaitra reviewed Jun 24, 2020

View reviewed changes

maflcko merged commit 67881de into bitcoin:master Jun 24, 2020

jonatack deleted the improve-p2p_invalid_messages branch June 25, 2020 07:13

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jonatack commented Jun 14, 2020 •

edited

Loading

DrahtBot commented Jun 14, 2020 •

edited

Loading

troygiorshev commented Jun 16, 2020

troygiorshev left a comment

jonatack commented Jun 16, 2020

glozow left a comment

jonatack commented Jun 17, 2020

jnewbery commented Jun 17, 2020

glozow commented Jun 17, 2020 •

edited

Loading

jonatack commented Jun 18, 2020

jonatack commented Jun 18, 2020 •

edited

Loading

jonatack commented Jun 18, 2020

maflcko commented Jun 18, 2020

maflcko left a comment

jonatack commented Jun 18, 2020 •

edited

Loading

jnewbery left a comment

troygiorshev commented Jun 18, 2020

maflcko left a comment

troygiorshev commented Jun 18, 2020

jonatack commented Jun 22, 2020 •

edited by maflcko

Loading

troygiorshev commented Jun 24, 2020

rajarshimaitra left a comment

rajarshimaitra Jun 24, 2020

jonatack Jun 25, 2020

maflcko commented Jun 24, 2020

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

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

Conversation

jonatack commented Jun 14, 2020 • edited Loading

DrahtBot commented Jun 14, 2020 • edited Loading

Conflicts

troygiorshev commented Jun 16, 2020

troygiorshev left a comment

Choose a reason for hiding this comment

jonatack commented Jun 16, 2020

glozow left a comment

Choose a reason for hiding this comment

jonatack commented Jun 17, 2020

jnewbery commented Jun 17, 2020

glozow commented Jun 17, 2020 • edited Loading

jonatack commented Jun 18, 2020

jonatack commented Jun 18, 2020 • edited Loading

jonatack commented Jun 18, 2020

maflcko commented Jun 18, 2020

maflcko left a comment

Choose a reason for hiding this comment

jonatack commented Jun 18, 2020 • edited Loading

jnewbery left a comment

Choose a reason for hiding this comment

troygiorshev commented Jun 18, 2020

maflcko left a comment

Choose a reason for hiding this comment

troygiorshev commented Jun 18, 2020

jonatack commented Jun 22, 2020 • edited by maflcko Loading

troygiorshev commented Jun 24, 2020

rajarshimaitra left a comment

Choose a reason for hiding this comment

rajarshimaitra Jun 24, 2020

Choose a reason for hiding this comment

jonatack Jun 25, 2020

Choose a reason for hiding this comment

maflcko commented Jun 24, 2020

jonatack commented Jun 14, 2020 •

edited

Loading

DrahtBot commented Jun 14, 2020 •

edited

Loading

glozow commented Jun 17, 2020 •

edited

Loading

jonatack commented Jun 18, 2020 •

edited

Loading

jonatack commented Jun 18, 2020 •

edited

Loading

jonatack commented Jun 22, 2020 •

edited by maflcko

Loading