[Test] Add p2p invalid messages functional test (and test framework update)#2410
Merged
random-zebra merged 29 commits intoJun 28, 2021
Merged
Conversation
furszy
force-pushed
the
2021_test_p2p_invalid_messages
branch
from
e2a4b18 to
e594dd6
Compare
Author
|
rebased on master, ready to go. |
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
Looking great. ACK e594dd6e77cdf013fd6acdc36086eb67580f82a2, with only a minor point over p2p_invalid_tx.
furszy
force-pushed
the
2021_test_p2p_invalid_messages
branch
from
e594dd6 to
bce9947
Compare
Author
|
Done, feedback tackled and squashed in 583fabf |
furszy
force-pushed
the
2021_test_p2p_invalid_messages
branch
2 times, most recently
from
eadadcc to
b0871f7
Compare
furszy
force-pushed
the
2021_test_p2p_invalid_messages
branch
2 times, most recently
from
d694f98 to
2501173
Compare
random-zebra
previously approved these changes
Jun 12, 2021
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
ACK 2501173f073e80e0f4b034c5bc5ea44402c8ffe1
furszy
force-pushed
the
2021_test_p2p_invalid_messages
branch
from
2501173 to
b60bf0d
Compare
Author
|
Rebased on master, fixed conflict with one of the recently merged PRs. |
random-zebra
previously approved these changes
Jun 26, 2021
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
re-re-utACK b60bf0dd6675a48d353c249862ef5f920fe038ab
Adaptation of btc@fa87da2f172ae2e6dc15e9ed156a3564a8ecfbdd from MarkoFalke
…nsport - checks if _transport.is_closing() (added in python3.4.4/python3.5.1) before attempting to send messages on P2PConnection's send_message method.
bitcoin#13715 introduced a new check for _transport.is_closing() in mininode's P2PConnection's. This function is only available from Python 3.4.4, though, while Bitcoin is supposed to support all Python 3.4 versions. In this change, we make the check conditional on is_closing() being available. If it is not, then we revert to the behaviour before the check was introduced; this means that bitcoin#13579 is not fixed for old systems, but at least the tests work as they used to do before. This includes a small refactoring from a one-line lambda to an inline function, because this makes the code easier to read with more and more conditions being added. Fixes bitcoin#13745.
Adds a utility to get resident set size memory usage for a test node and a context manager that allows assertions based upon maximum memory use increase.
[Conjunction of several PRs coming from btc]
Adaptation of btc@fe1dc62cef88280d2490a619beded052f313c6fc
…and and raise sync_with_ping timeout
Adaptation of btc@fa4c29bc1d2425f861845bae4f3816d9817e622a with some needed customizations for us.
Test 1 is a duplicate of test_size() later in the file. Inexplicably, this test does not work on macOS, whereas test_size() does. Test 2 is problematic for two reasons. First, it always fails with an invalid checksum, which is probably not what was intended. Second, it's not defined at this layer what the behavior should be. Hypothetically, if this test was fixed so that it gave messages with valid checksums, then the message would pass successfully thought the network layer and fail only in the processing layer. A priori the network layer has no idea what the size of a message "actually" is. The "Why does behavior change at 78 bytes" is because of the following: print(len(node.p2p.build_message(msg))) # 125 => Payload size = 125 - 24 = 101 If we take 77 bytes, then there are 101 - 77 = 24 left That's exactly the size of a header So, bitcoind deserializes the header and rejects it for some other reason (Almost always an invalid size (too large)) But, if we take 78 bytes, then there are 101 - 78 = 23 left That's not enough to fill a header, so the socket stays open waiting for more data. That's why we sometimes have to push additional data in order for the peer to disconnect. Additionally, both of these tests use the "conn" variable. For fun, go look at where it's declared. (Hint: test_large_inv(). Don't we all love python's idea of scope?)
As well, this renames those variables to match PEP8 and this clears up the comment relating to VALID_DATA_LIMIT. Admittedly, this commit is mainly to make the following ones cleaner.
This test originally made a message with an invalid stated length, and an invalid checksum. This was because only the header was changed, but the checksum stayed the same. This was fine for now because we check the header first to see if it has a valid stated size, and we disconnect if it does not, so we never end up checking for the checksum. If this behavior was to change, this test would become a problem. (Indeed I discovered this when playing around with this behavior). By instead creating a message with an oversized payload from the start, we create a message with an invalid stated length but a valid checksum, as intended. Additionally, this takes advantage to the newly module-global VALID_DATA_LIMIT as opposed to the magic 0x02000000. Yes, 4MB < 32MiB, but at the moment when receiving a message we check both, so this makes the test tighter.
This is a simple refactor of the specified test. It is now brought in line with the rest of the tests in the module. This should make things easier to debug, as all of the tests are now grouped together at the top.
furszy
force-pushed
the
2021_test_p2p_invalid_messages
branch
from
b60bf0d to
7b04c0a
Compare
Author
|
Had to rebase this one again, fixed a tiny conflict in the test_runner file. Let's move ahead with it, it's part of Tor's v3 onion addresses work path. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the deep and long net and ser work that I'm doing (and Tor v3 onion addresses support). Friend of #2359.
Focused on the end goal of implementing the
p2p_invalid_messagesfunctional test which validates that invalid msg headers, msg types, oversized payloads and inventory msgs aren't accepted nor can cause a resource exhaustion. And an extra covered scenario, inp2p_invalid_tx.py, for the orphan pool overflow.Plus, to get up to the goal, had to work over the functional test framework as well.
So.. adapted list:
p2p_invalid_tx.pyandp2p_invalid_messages.py. We don't support the other tests yet).