[qa] mininode: Expose connection state through is_connected #13512
Conversation
faffbc0 to
fa1a3f5
Compare
There was a problem hiding this comment.
Great. Definitely concept ACK changing P2PConnection.state from a string and making it private, however I think it'd be a cleaner implementation to just turn state into an Enum class rather than two bools (asyncore_pre_connection and conn_open). Also rename to _state to indicate that it's private.
| @@ -77,14 +77,19 @@ def __init__(self): | |||
|
|
|||
| super().__init__(map=mininode_socket_map) | |||
|
|
|||
| self.conn_open = False | |||
|
|
|||
| def is_connected(self): | |||
There was a problem hiding this comment.
suggestion: change this to a property (use @property decorator)
There was a problem hiding this comment.
I believe this doesn't work when the implementation details change and we no longer return a member. That is, when we derive the return value with an expression. E.g. self._transport is not None or self._state == State.CONNECTED or ...
There was a problem hiding this comment.
It should still work. a @property decorated method can do anything any other method can do.
There was a problem hiding this comment.
Ah thanks for clarifying. Reworked the patch to make it a property.
|
The reason I split those into two bools is that we are only really interested in one ( |
faa1917 to
fac45ba
Compare
I disagree that this is artificially introducing an enum. tbh though, this is just nitting. Fine to split this to two bools now if that's what you prefer. I would suggest you name them |
fac45ba to
fa69902
Compare
|
Made both bools private members |
|
|
|
No need to ask for permission. Just throw them at me
…On Thu, Jun 21, 2018, 17:29 John Newbery ***@***.***> wrote:
p2p_sendheaders.py fails. I have logs from a local failure if you want
them.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#13512 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGGmv0vzYfGiS1Xd-kCvjPDwTpowq3Qbks5t_BBKgaJpZM4UxRTg>
.
|
fa69902 to
fa1eac9
Compare
|
I coudn't repro the p2p_sendheaders failure. Investigating a bit, I see that there's a pre-existing race condition in that test: #13522, so the failure that I saw locally and on travis isn't related to this PR. |
|
Tested ACK fa1eac9 |
…cted fa1eac9 [qa] mininode: Expose connection state through is_connected (MarcoFalke) Pull request description: This gets rid of some non-type safe string comparisons and access to members that are implementation details of `class P2PConnection(asyncore.dispatcher)`. Such refactoring is required to replace the deprecated asyncore with something more sane. Changes: * Get rid of non-enum member `state` and replace is with bool `connected` * Get rid of confusing argument `pushbuf` and literally just push to the buffer at the call site Tree-SHA512: 09074c7e5ed251a2e0509ef205ab82f89887c1e1fa1cc6efc1db60d196eb2403788a4987df8809fd06d80ef652e614c5d3c3fdef70096fc5815102243388288d
…s_connected fa1eac9 [qa] mininode: Expose connection state through is_connected (MarcoFalke) Pull request description: This gets rid of some non-type safe string comparisons and access to members that are implementation details of `class P2PConnection(asyncore.dispatcher)`. Such refactoring is required to replace the deprecated asyncore with something more sane. Changes: * Get rid of non-enum member `state` and replace is with bool `connected` * Get rid of confusing argument `pushbuf` and literally just push to the buffer at the call site Tree-SHA512: 09074c7e5ed251a2e0509ef205ab82f89887c1e1fa1cc6efc1db60d196eb2403788a4987df8809fd06d80ef652e614c5d3c3fdef70096fc5815102243388288d
…s_connected fa1eac9 [qa] mininode: Expose connection state through is_connected (MarcoFalke) Pull request description: This gets rid of some non-type safe string comparisons and access to members that are implementation details of `class P2PConnection(asyncore.dispatcher)`. Such refactoring is required to replace the deprecated asyncore with something more sane. Changes: * Get rid of non-enum member `state` and replace is with bool `connected` * Get rid of confusing argument `pushbuf` and literally just push to the buffer at the call site Tree-SHA512: 09074c7e5ed251a2e0509ef205ab82f89887c1e1fa1cc6efc1db60d196eb2403788a4987df8809fd06d80ef652e614c5d3c3fdef70096fc5815102243388288d
…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
This gets rid of some non-type safe string comparisons and access to members that are implementation details of
class P2PConnection(asyncore.dispatcher). Such refactoring is required to replace the deprecated asyncore with something more sane.Changes:
stateand replace is with boolconnectedpushbufand literally just push to the buffer at the call site