Skip to content

qa: Make swap_magic_bytes in p2p_invalid_messages atomic #15697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Apr 23, 2019
Merged

qa: Make swap_magic_bytes in p2p_invalid_messages atomic #15697

maflcko merged 1 commit into from
Apr 23, 2019

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Mar 29, 2019

@DrahtBot DrahtBot added the Tests label Mar 29, 2019
@maflcko maflcko force-pushed the 1904-qaMagicAtomic branch 2 times, most recently from fa31421 to fa9c1a8 Compare March 29, 2019 18:13
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 29, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15197 (Refactor and slightly stricter p2p message processing by jonasschnelli)

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.

@maflcko maflcko force-pushed the 1904-qaMagicAtomic branch from fa9c1a8 to faca95e Compare April 4, 2019 20:51
@maflcko maflcko merged commit faca95e into bitcoin:master Apr 23, 2019
@maflcko maflcko deleted the 1904-qaMagicAtomic branch April 23, 2019 17:14
maflcko pushed a commit that referenced this pull request Apr 23, 2019
Merge #15697: qa: Make swap_magic_bytes in p2p_invalid_messages atomic
40a720a 
faca95e qa: Make swap_magic_bytes in p2p_invalid_messages atomic (MarcoFalke)

Pull request description:

  Otherwise, this will lead to errors logged in the network thread:

  https://travis-ci.org/MarcoFalke/bitcoin/jobs/513076282#L2765

ACKs for commit faca95:

Tree-SHA512: 8bc8280f9c0e8d40bc68c0ff1cab1c3386cbfef0728dbab5f29c2606dd65a9ac3c695b0c286be707a9f2bd62a6f87ac2032d7749fc2cf8b9fa1eba3c4cf70933
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 24, 2019
@sidhujag
Merge bitcoin#15697: qa: Make swap_magic_bytes in p2p_invalid_message…
5d87bb7 
…s atomic

faca95e qa: Make swap_magic_bytes in p2p_invalid_messages atomic (MarcoFalke)

Pull request description:

  Otherwise, this will lead to errors logged in the network thread:

  https://travis-ci.org/MarcoFalke/bitcoin/jobs/513076282#L2765

ACKs for commit faca95:

Tree-SHA512: 8bc8280f9c0e8d40bc68c0ff1cab1c3386cbfef0728dbab5f29c2606dd65a9ac3c695b0c286be707a9f2bd62a6f87ac2032d7749fc2cf8b9fa1eba3c4cf70933
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 8, 2020
@jasonbcox
Merge #15697: qa: Make swap_magic_bytes in p2p_invalid_messages atomic
294237a 
Summary:
faca95effd qa: Make swap_magic_bytes in p2p_invalid_messages atomic (MarcoFalke)

Pull request description:

  Otherwise, this will lead to errors logged in the network thread:

  https://travis-ci.org/MarcoFalke/bitcoin/jobs/513076282#L2765

ACKs for commit faca95:

Tree-SHA512: 8bc8280f9c0e8d40bc68c0ff1cab1c3386cbfef0728dbab5f29c2606dd65a9ac3c695b0c286be707a9f2bd62a6f87ac2032d7749fc2cf8b9fa1eba3c4cf70933

Backport of Core [[bitcoin/bitcoin#15697 | PR15697]]

Depends on D6395

Test Plan: `test_runner.py p2p_invalid_messages`

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6396
@jnewbery jnewbery mentioned this pull request Jun 12, 2020
Merged
@maflcko
Copy link
Member Author

maflcko commented Jun 12, 2020

The link in OP is no longer valid. New link https://travis-ci.org/github/MarcoFalke/bitcoin-core/jobs/513076282#L2765

excerpt:

2019-03-29T15:48:29.764000Z TestFramework (INFO): Tests successful

stderr:

--- Logging error ---

Traceback (most recent call last):

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 174, in _on_data

    raise ValueError("magic bytes mismatch: {} != {}".format(repr(self.magic_bytes), repr(self.recvbuf)))

ValueError: magic bytes mismatch: b'\x00\x11"2' != b'\xfa\xbf\xb5\xdafeefilter\x00\x00\x00\x08\x00\x00\x00\xe8\x0f\xd1\x9f'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):

  File "/usr/lib/python3.6/logging/__init__.py", line 994, in emit

    msg = self.format(record)

  File "/usr/lib/python3.6/logging/__init__.py", line 840, in format

    return fmt.format(record)

  File "/usr/lib/python3.6/logging/__init__.py", line 577, in format

    record.message = record.getMessage()

  File "/usr/lib/python3.6/logging/__init__.py", line 338, in getMessage

    msg = msg % self.args

TypeError: not all arguments converted during string formatting

Call stack:

  File "/usr/lib/python3.6/threading.py", line 884, in _bootstrap

    self._bootstrap_inner()

  File "/usr/lib/python3.6/threading.py", line 916, in _bootstrap_inner

    self.run()

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 444, in run

    self.network_event_loop.run_forever()

  File "/usr/lib/python3.6/asyncio/base_events.py", line 427, in run_forever

    self._run_once()

  File "/usr/lib/python3.6/asyncio/base_events.py", line 1440, in _run_once

    handle._run()

  File "/usr/lib/python3.6/asyncio/events.py", line 145, in _run

    self._callback(*self._args)

  File "/usr/lib/python3.6/asyncio/selector_events.py", line 732, in _read_ready

    self._protocol.data_received(data)

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 161, in data_received

    self._on_data()

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 196, in _on_data

    logger.exception('Error reading message:', repr(e))

Message: 'Error reading message:'

Arguments: ('ValueError(\'magic bytes mismatch: b\\\'\\\\x00\\\\x11"2\\\' != b\\\'\\\\xfa\\\\xbf\\\\xb5\\\\xdafeefilter\\\\x00\\\\x00\\\\x00\\\\x08\\\\x00\\\\x00\\\\x00\\\\xe8\\\\x0f\\\\xd1\\\\x9f\\\'\',)',)

--- Logging error ---

Traceback (most recent call last):

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 174, in _on_data

    raise ValueError("magic bytes mismatch: {} != {}".format(repr(self.magic_bytes), repr(self.recvbuf)))

ValueError: magic bytes mismatch: b'\x00\x11"2' != b'\xfa\xbf\xb5\xdafeefilter\x00\x00\x00\x08\x00\x00\x00\xe8\x0f\xd1\x9f'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):

  File "/usr/lib/python3.6/logging/__init__.py", line 994, in emit

    msg = self.format(record)

  File "/usr/lib/python3.6/logging/__init__.py", line 840, in format

    return fmt.format(record)

  File "/usr/lib/python3.6/logging/__init__.py", line 577, in format

    record.message = record.getMessage()

  File "/usr/lib/python3.6/logging/__init__.py", line 338, in getMessage

    msg = msg % self.args

TypeError: not all arguments converted during string formatting

Call stack:

  File "/usr/lib/python3.6/threading.py", line 884, in _bootstrap

    self._bootstrap_inner()

  File "/usr/lib/python3.6/threading.py", line 916, in _bootstrap_inner

    self.run()

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 444, in run

    self.network_event_loop.run_forever()

  File "/usr/lib/python3.6/asyncio/base_events.py", line 427, in run_forever

    self._run_once()

  File "/usr/lib/python3.6/asyncio/base_events.py", line 1440, in _run_once

    handle._run()

  File "/usr/lib/python3.6/asyncio/events.py", line 145, in _run

    self._callback(*self._args)

  File "/usr/lib/python3.6/asyncio/selector_events.py", line 732, in _read_ready

    self._protocol.data_received(data)

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 161, in data_received

    self._on_data()

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 196, in _on_data

    logger.exception('Error reading message:', repr(e))

Message: 'Error reading message:'

Arguments: ('ValueError(\'magic bytes mismatch: b\\\'\\\\x00\\\\x11"2\\\' != b\\\'\\\\xfa\\\\xbf\\\\xb5\\\\xdafeefilter\\\\x00\\\\x00\\\\x00\\\\x08\\\\x00\\\\x00\\\\x00\\\\xe8\\\\x0f\\\\xd1\\\\x9f\\\'\',)',)

Exception in callback _SelectorSocketTransport._read_ready()

handle: <Handle _SelectorSocketTransport._read_ready()>

Traceback (most recent call last):

  File "/usr/lib/python3.6/asyncio/events.py", line 145, in _run

    self._callback(*self._args)

  File "/usr/lib/python3.6/asyncio/selector_events.py", line 732, in _read_ready

    self._protocol.data_received(data)

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 161, in data_received

    self._on_data()

  File "/home/travis/build/MarcoFalke/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 174, in _on_data

    raise ValueError("magic bytes mismatch: {} != {}".format(repr(self.magic_bytes), repr(self.recvbuf)))

ValueError: magic bytes mismatch: b'\x00\x11"2' != b'\xfa\xbf\xb5\xdafeefilter\x00\x00\x00\x08\x00\x00\x00\xe8\x0f\xd1\x9f'

@furszy furszy mentioned this pull request Jun 4, 2021
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jun 28, 2021
@random-zebra
Merge #2410: [Test] Add p2p invalid messages functional test (and tes…
bffe509 
…t framework update)

7b04c0a [Test] Clean duplicate connections creation in mining_pos_coldStaking.py (furszy)
15a799e [Test] MAX_PROTOCOL_MESSAGE_LENGTH PIVXified. (furszy)
0aedf35 [tests] Don't import asyncio to test magic bytes (John Newbery)
9bb5309 Refactor resource exhaustion test (Troy Giorshev)
589a780 Fix "invalid message size" test (Troy Giorshev)
8a0c12b Move size limits to module-global (Troy Giorshev)
b3391c5 Remove two unneeded tests (Troy Giorshev)
1404e68 test: Add various low-level p2p tests (furszy)
8aaf7e1 test: Remove fragile assert_memory_usage_stable (MarcoFalke)
f68e22c [Test] p2p_invalid_messages.py do not change msg.command in test_command and raise sync_with_ping timeout (furszy)
c851c0b Test framework: Wait for verack by default on every new connection (furszy)
c02e9a0 [Test] move framework subversion to string (furszy)
3472a39 Replace coroutine with async def in p2p_invalid_messages.py (Elichai Turkel)
33c7b19 net_processing: align msg checksum check properly. (furszy)
7f71b1b Hash P2P messages as they are received instead of at process-time (furszy)
215a638 test: Skip flaky p2p_invalid_messages test on macOS (Fabian Jahr)
c11f565 qa: Make swap_magic_bytes in p2p_invalid_messages atomic (MarcoFalke)
be979ad test: Fix race in p2p_invalid_messages (MarcoFalke)
6a72f0c qa: Add tests for invalid message headers (MarcoFalke)
51ddd3d Introduce p2p_invalid_messages.py test (furszy)
55a37b5 net: fix missing jump line in "Oversized message from peer.." error. (furszy)
0edfce5 test_node: get_mem_rss fixups (MarcoFalke)
6f21213 tests: add P2PConnection.send_raw_message (James O'Beirne)
ae68c6e tests: add utility to assert node memory usage hasn't increased (James O'Beirne)
8469afe test: forward timeouts properly in send_blocks_and_test (James O'Beirne)
db28a53 Skip is_closing() check when not available. (Daniel Kraft)
be9dacb tests: fixes mininode's P2PConnection sending messages on closing transport (marcoagner)
53599c8 qa: Avoid start/stop of the network thread mid-test (furszy)
688190c [qa] mininode: Expose connection state through is_connected (MarcoFalke)

Pull request description:

  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_messages` functional 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, in `p2p_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:

  * bitcoin#9045.
  * bitcoin#13512.
  * bitcoin#13517.
  * bitcoin#13715.
  * bitcoin#13747.
  * bitcoin#14456.
  * bitcoin#14522.
  * bitcoin#14672.
  * bitcoin#14693.
  * bitcoin#14812.
  * bitcoin#15246.
  * bitcoin#15330.
  * bitcoin#15697.
  * bitcoin#16445.
  * bitcoin#17931.
  * bitcoin#17469.
  * bitcoin#18628 (only `p2p_invalid_tx.py` and `p2p_invalid_messages.py`. We don't support the other tests yet).
  * bitcoin#19177.
  * bitcoin#19264.

ACKs for top commit:
  random-zebra:
    utACK 7b04c0a and merging...

Tree-SHA512: 48d1f1a6acc24a9abce033fbf1f281ba4392147da39d118a694a09d63c3e0610cc1a29d711b434b16cc0d0e030dd9f1a89843870091b6999b862b9ab18a20679
kwvg added a commit to kwvg/dash that referenced this pull request Nov 3, 2021
@kwvg
merge bitcoin#15697: Make swap_magic_bytes in p2p_invalid_messages at…
a5445a4 
…omic
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@kwvg @pravblockc
merge bitcoin#15697: Make swap_magic_bytes in p2p_invalid_messages at…
bb80da4 
…omic
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@maflcko @DrahtBot