Skip to content

[tests] [demonstration] Simplify/clarify the NodeConn/NodeConnCB mininode classes #11518

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
Closed
wants to merge 14 commits into from
Closed

[tests] [demonstration] Simplify/clarify the NodeConn/NodeConnCB mininode classes #11518

wants to merge 14 commits into from

Conversation

jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Oct 17, 2017 •
edited
Loading

The mininode.py module is confusing, and the relationship between the NodeConn and NodeConnCB classes is messy. This PR is an attempt to simplify the code and the external interface.

First, the motivation:

  • make the interface to the mininode classes less confusing. Right now the individual tests need to create a NodeConnCB, then append a NodeConn connection to that NodeConnCB, while passing the NodeConnCB into the NodeConn as an argument. The internal interface between the NodeConn and NodeConnCB is equally tortuous. Note that [tests] Add P2P interface to TestNode #11182 hides that complexity from the user behind TestNode, but this PR actually simplifies the internal implementation.
  • fully separate the low-level networking connection from the higher-level P2P interface. That means that in future we can swap out the network implementation (ie remove the deprecated asyncore and replace with asyncio), and the individual tests and P2P interface classes should be minimally impacted.

Even though this is marked as [demonstration], it is already a complete implementation. It's too much to review in one PR, so this PR demonstrates the concepts. Separate PRs will be opened for the individual steps:

some notes on the implementation:

  • The main structural change in this PR is that the old NodeConnCB class is now a subclass of NodeConn. Individual tests and the TestNode class will only need to interact with the NodeConnCB class.
  • NodeConn has been renamed P2PConnection. It's a low-level class that handles the TCP connection, reading and sending messages, and deserializing and serializing the P2P headers. It knows the names of P2P commands, but nothing about how to parse them.
  • NodeConnCB has been renamed P2PInterface. It's a high-level class that handles communicating with the Bitcoin node over the P2P interface. It's responsible for processing incoming P2P payloads and constructing/sending P2P payloads.

Note that the distinction above is somewhat analogous to net/net_processing in Bitcoin Core. The distinction already existed between NodeConn and NodeConnCB but was blurry and not well enforced. This PR makes the distinction more formal.

  • git stats show this as ~2000 lines of change. It's not. I've split mininode.py into two files (pulling out the Bitcoin struct and msg primitives into their own primitives.py file). That's mostly a move-only change, and accounts for ~1400 lines of the change.
  • this PR also culls a bunch of dead code from NodeConn and NodeConnCB, mostly around supporting very old versions of the P2P protocol. Our test framework can't support versions of Bitcoin Core further back than a couple of releases, but there's code in mininode to support P2P versions 209 and 60001. I've removed all of that, as well as support for alert messages.
  • I've also removed the ugly rpc property of the NodeConnCB class, which required a lot of lines of (completely mechanical) change to the p2p-segwit test. Conceptually it doesn't make sense for a NodeConnCB P2P interface to own an rpc interface.
  • This PR also tidies up the structure of the mininode.py module, so related functions are grouped together more logically.

@fanquake fanquake added the Tests label Oct 17, 2017
jnewbery added 14 commits October 18, 2017 08:34
@jnewbery
[tests] Add p2p connection to TestNode
82fc1bc 
p2p connections can now be added to TestNode instances.
@jnewbery
[tests] use TestNode p2p connection in tests
b1ae396
@jnewbery
[tests] Move test_framework Bitcoin primitives into separate module
64d947b 
mininode.py wildcard imports all names from primitives.py. This is
to avoid having to change all test scripts that import from mininode.py.
@jnewbery
[tests] Remove dead code from mininode.py
c7bf1fe 
Removes the dead deliver_sleep_time and EarlyDisconnectError code
@jnewbery
[tests] Remove support for bre-BIP31 ping messages
b8c6d95 
BIP31 support was added to Bitcoin Core in version 0.6.1. Our test
framework is incompatible with Bitcoin Core versions that old, so remove
all special logic for handling pre-BIP31 pings.
@jnewbery
[tests] Tidy up mininode.py module
c0caa7b 
Mostly move only. Adds a few extra comments.
@jnewbery
[tests] Explicitly disallow support for p2p versions below 60001
9991677 
The mininode module includes code to support p2p versions below
60001. However, the test_framework does not support versions
of Bitcoin Core before V0.13.0. Remove code supporting
p2p versions before 60001 (which has never been run).
@jnewbery
[tests] Remove support for p2p alert messages
f5a8c5d 
Alert messages were removed in p2p version 70013 (Bitcoin Core V0.13.0)
@jnewbery
[tests] Remove rpc property from NodeConn.
333012d 
It's only used in a helper method in p2p-segwit.py.

Change that helper method to a function which takes a node and a p2p
connection as arguments.
@jnewbery
[tests] use add_p2p_connection in p2p-fingerprint.py
dab9e22
@jnewbery
[tests] Better document the distinction between NodeConn and NodeConnCB
5765169 
This adds documentation explaining the separation between
NodeConn (representing the low-level connection and P2P
header parsing) from NodeConnCB (representing the higher
level P2P payload processing and convenience methods).

This commit also removes the ping keepalive logic. No tests
run for more than 30 minutes, so this logic is never
exercised.
@jnewbery
[tests] Make NodeConnCB a subclass of NodeConn
afa6fc1 
This makes NodeConnCB a subclass of NodeConn, and
removes the need for the client code to know
anything about the implementation details of NodeConnCB.

NodeConn can now be swapped out for any other implementation
of a low-level connection without changing client code.
@jnewbery
[tests] Move version message sending from NodeConn to NodeConnCB
cf236f4 
This commit moves the logic that sends a version message
on connection from NodeConn to NodeConnCB. NodeConn should
not be aware of the semantics or meaning of the P2P payloads.
@jnewbery
[tests] Rename NodeConn and NodeConnCB
293459a 
NodeConn -> P2PConnection
NodeConnCB -> P2PInterface
@jnewbery jnewbery force-pushed the node_conn_big_refactor branch from d28f36d to 293459a Compare October 18, 2017 12:35
@jnewbery jnewbery mentioned this pull request Oct 20, 2017
Merged
@jnewbery jnewbery changed the title [WIP] [tests] Simplify/clarify the NodeConn/NodeConnCB mininode classes [tests] [demonstration] Simplify/clarify the NodeConn/NodeConnCB mininode classes Nov 8, 2017
@jnewbery jnewbery mentioned this pull request Nov 8, 2017
Merged
jamesob
jamesob reviewed Nov 9, 2017
View reviewed changes
Copy link
Contributor

@jamesob jamesob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. Looks good!

test/functional/test_framework/mininode.py

MAX_INV_SZ = 50000
MAX_BLOCK_BASE_SIZE = 1000000
from test_framework.primitives import *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we want to avoid * imports or is this a case that merits exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's ugly. See the commit message for 64d947b:

mininode.py wildcard imports all names from primitives.py. This is
to avoid having to change all test scripts that import from mininode.py.

I don't like wildcard imports, but we're pretty lax about them in the functional tests, so this isn't making things much worse.

(I'd be happy to remove wildcard imports entirely and have tried to do that before in #9876 and #9943)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, thanks for the pointer. I'd be happy to help with this after the more important P2P* changes are merged (unless there's some good reason to do the two simultaneously).

maflcko pushed a commit that referenced this pull request Nov 9, 2017
Merge #11638: [tests] Dead mininode code
5e3f5e4 
fb00c45 [tests] Explicitly disallow support for p2p versions below 60001 (John Newbery)
3858aab [tests] Remove support for p2p alert messages (John Newbery)
c0b1274 [tests] Remove support for bre-BIP31 ping messages (John Newbery)
2904e30 [tests] Remove dead code from mininode.py (John Newbery)

Pull request description:

  This is the first part of #11518. It removes a ~150 lines of unused code from the mininode module:

  - remove unused `deliver_sleep_time` and `EarlyDisconnectError` code
  - remove support for pre-BIP31 ping messages
  - remove support for alert message
  - explicitly don't support p2p versions lower than 60001

  Should be an easy ACK for reviewers. If all extended tests pass, then this code really was dead :)

Tree-SHA512: 508e612ceb0b094250d18e75522d51e6b14cd069443050ba4af34d6f890c58721cb5653e8bc000b60635b9474d035b0dcd9c509c0dcdb3a7501df17b787f83b0
@jnewbery jnewbery mentioned this pull request Nov 9, 2017
Merged
@jnewbery
Copy link
Contributor Author

jnewbery commented Nov 9, 2017

Added #11648 for the next step.

@jnewbery jnewbery mentioned this pull request Nov 14, 2017
Merged
@jnewbery
Copy link
Contributor Author

jnewbery commented Nov 14, 2017 •
edited
Loading

#11677 covers removing the rpc property

laanwj added a commit that referenced this pull request Nov 17, 2017
@laanwj
Merge #11648: [tests] Add messages.py
ccc70a2 
1135c79 [tests] Tidy up mininode.py module (John Newbery)
f9cd9b1 [tests] Move test_framework Bitcoin primitives into separate module (John Newbery)

Pull request description:

  Second part of #11518.

  Moves the primitive Bitcoin datastructures and message classes into their own module, and tidies up the mininode.py module.

  - First commit is almost entirely move-only
  - Second commit is mostly move-only, but also does a little tidying.

Tree-SHA512: 5d74802677f1ab788e43188653106a96fffd9ab1fe3aa6a4eb94e5807de5dd5c8ee212296f45e8d16c7e3d95cfc4891677e812b7944bd3ab604e04b3b88aa06e
@jnewbery jnewbery mentioned this pull request Nov 17, 2017
Merged
@jnewbery
Copy link
Contributor Author

Opened #11712 for splitting NodeConn and NodeConnCB

maflcko pushed a commit that referenced this pull request Nov 29, 2017
Merge #11712: [tests] Split NodeConn from NodeConnCB
9f2c2db 
e9dfa9b [tests] Move version message sending from NodeConn to NodeConnCB (John Newbery)
dad596f [tests] Make NodeConnCB a subclass of NodeConn (John Newbery)
e30d404 [tests] Move only: move NodeConnCB below NodeConn (John Newbery)
4d50598 [tests] Tidy up mininode (John Newbery)
f2ae6f3 [tests] Remove mininode periodic (half-hour) ping messages (John Newbery)
ec59523 [tests] Remove rpc property from TestNode in p2p-segwit.py. (John Newbery)

Pull request description:

  This is the final step in #11518, except for possibly renaming - for motivation, please see that PR.

  If this is merged, then migrating the test framework from asyncore to asyncio should be easier (I say should because I haven't dug too deeply into what would be required).

  Requesting review from @ryanofsky , since he always has good feedback on these refactor PRs, and I'd appreciate his take on this refactor. Note particularly that I've reverted the change suggested here: #11182 (comment) . The idea, as always, is to present a simple interface to the test writer.

Tree-SHA512: 94dd467a13ec799b101108cf47d4dccb6f6240b601e375e3d785313333bbb389c26072a50759aca663bbf3d6c8b867b99e36ae8800ab8ea115e0496c151926ce
@jnewbery jnewbery mentioned this pull request Nov 29, 2017
Merged
@jnewbery
Copy link
Contributor Author

Opened #11791 to rename NodeConn and NodeConnCB.

Closing this PR since that's the last task here.

@jnewbery jnewbery closed this Nov 29, 2017
@jnewbery jnewbery deleted the node_conn_big_refactor branch November 29, 2017 22:18
maflcko pushed a commit that referenced this pull request Nov 30, 2017
Merge #11791: [tests] Rename NodeConn and NodeConnCB
13e31dd 
873beca [tests] Rename NodeConn and NodeConnCB (John Newbery)

Pull request description:

  Final step in #11518

  NodeConn -> P2PConnection
  NodeConnCB -> P2PInterface

  This is basically just a rename. Should be an easy review.

Tree-SHA512: fe1204b2b3d8182c5e324ffa7cb4099a47ef8536380e0bb9d37a5fccf76a24f548d1f1a7988ab8f830986a3058b670696de3fc891af5e5f75dbeb4e3273005d7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 11, 2019
@PastaPastaPasta
Merge bitcoin#11638: [tests] Dead mininode code
d775719 
fb00c45 [tests] Explicitly disallow support for p2p versions below 60001 (John Newbery)
3858aab [tests] Remove support for p2p alert messages (John Newbery)
c0b1274 [tests] Remove support for bre-BIP31 ping messages (John Newbery)
2904e30 [tests] Remove dead code from mininode.py (John Newbery)

Pull request description:

  This is the first part of bitcoin#11518. It removes a ~150 lines of unused code from the mininode module:

  - remove unused `deliver_sleep_time` and `EarlyDisconnectError` code
  - remove support for pre-BIP31 ping messages
  - remove support for alert message
  - explicitly don't support p2p versions lower than 60001

  Should be an easy ACK for reviewers. If all extended tests pass, then this code really was dead :)

Tree-SHA512: 508e612ceb0b094250d18e75522d51e6b14cd069443050ba4af34d6f890c58721cb5653e8bc000b60635b9474d035b0dcd9c509c0dcdb3a7501df17b787f83b0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 11, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#11648: [tests] Add messages.py
7bd9994 
1135c79 [tests] Tidy up mininode.py module (John Newbery)
f9cd9b1 [tests] Move test_framework Bitcoin primitives into separate module (John Newbery)

Pull request description:

  Second part of bitcoin#11518.

  Moves the primitive Bitcoin datastructures and message classes into their own module, and tidies up the mininode.py module.

  - First commit is almost entirely move-only
  - Second commit is mostly move-only, but also does a little tidying.

Tree-SHA512: 5d74802677f1ab788e43188653106a96fffd9ab1fe3aa6a4eb94e5807de5dd5c8ee212296f45e8d16c7e3d95cfc4891677e812b7944bd3ab604e04b3b88aa06e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 13, 2020
@PastaPastaPasta
Merge bitcoin#11638: [tests] Dead mininode code
3b53fa0 
fb00c45 [tests] Explicitly disallow support for p2p versions below 60001 (John Newbery)
3858aab [tests] Remove support for p2p alert messages (John Newbery)
c0b1274 [tests] Remove support for bre-BIP31 ping messages (John Newbery)
2904e30 [tests] Remove dead code from mininode.py (John Newbery)

Pull request description:

  This is the first part of bitcoin#11518. It removes a ~150 lines of unused code from the mininode module:

  - remove unused `deliver_sleep_time` and `EarlyDisconnectError` code
  - remove support for pre-BIP31 ping messages
  - remove support for alert message
  - explicitly don't support p2p versions lower than 60001

  Should be an easy ACK for reviewers. If all extended tests pass, then this code really was dead :)

Tree-SHA512: 508e612ceb0b094250d18e75522d51e6b14cd069443050ba4af34d6f890c58721cb5653e8bc000b60635b9474d035b0dcd9c509c0dcdb3a7501df17b787f83b0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 13, 2020
@PastaPastaPasta
Merge bitcoin#11638: [tests] Dead mininode code
fd43baa 
fb00c45 [tests] Explicitly disallow support for p2p versions below 60001 (John Newbery)
3858aab [tests] Remove support for p2p alert messages (John Newbery)
c0b1274 [tests] Remove support for bre-BIP31 ping messages (John Newbery)
2904e30 [tests] Remove dead code from mininode.py (John Newbery)

Pull request description:

  This is the first part of bitcoin#11518. It removes a ~150 lines of unused code from the mininode module:

  - remove unused `deliver_sleep_time` and `EarlyDisconnectError` code
  - remove support for pre-BIP31 ping messages
  - remove support for alert message
  - explicitly don't support p2p versions lower than 60001

  Should be an easy ACK for reviewers. If all extended tests pass, then this code really was dead :)

Tree-SHA512: 508e612ceb0b094250d18e75522d51e6b14cd069443050ba4af34d6f890c58721cb5653e8bc000b60635b9474d035b0dcd9c509c0dcdb3a7501df17b787f83b0

readd is None check

Signed-off-by: Pasta <pasta@dashboost.org>
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 13, 2020
@PastaPastaPasta
Merge bitcoin#11638: [tests] Dead mininode code
713e899 
fb00c45 [tests] Explicitly disallow support for p2p versions below 60001 (John Newbery)
3858aab [tests] Remove support for p2p alert messages (John Newbery)
c0b1274 [tests] Remove support for bre-BIP31 ping messages (John Newbery)
2904e30 [tests] Remove dead code from mininode.py (John Newbery)

Pull request description:

  This is the first part of bitcoin#11518. It removes a ~150 lines of unused code from the mininode module:

  - remove unused `deliver_sleep_time` and `EarlyDisconnectError` code
  - remove support for pre-BIP31 ping messages
  - remove support for alert message
  - explicitly don't support p2p versions lower than 60001

  Should be an easy ACK for reviewers. If all extended tests pass, then this code really was dead :)

Tree-SHA512: 508e612ceb0b094250d18e75522d51e6b14cd069443050ba4af34d6f890c58721cb5653e8bc000b60635b9474d035b0dcd9c509c0dcdb3a7501df17b787f83b0

readd is None check

Signed-off-by: Pasta <pasta@dashboost.org>
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 29, 2020
@PastaPastaPasta
Merge bitcoin#11638: [tests] Dead mininode code
a42d62d 
fb00c45 [tests] Explicitly disallow support for p2p versions below 60001 (John Newbery)
3858aab [tests] Remove support for p2p alert messages (John Newbery)
c0b1274 [tests] Remove support for bre-BIP31 ping messages (John Newbery)
2904e30 [tests] Remove dead code from mininode.py (John Newbery)

Pull request description:

  This is the first part of bitcoin#11518. It removes a ~150 lines of unused code from the mininode module:

  - remove unused `deliver_sleep_time` and `EarlyDisconnectError` code
  - remove support for pre-BIP31 ping messages
  - remove support for alert message
  - explicitly don't support p2p versions lower than 60001

  Should be an easy ACK for reviewers. If all extended tests pass, then this code really was dead :)

Tree-SHA512: 508e612ceb0b094250d18e75522d51e6b14cd069443050ba4af34d6f890c58721cb5653e8bc000b60635b9474d035b0dcd9c509c0dcdb3a7501df17b787f83b0

readd is None check

Signed-off-by: Pasta <pasta@dashboost.org>
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 6, 2020
@PastaPastaPasta
Merge bitcoin#11791: [tests] Rename NodeConn and NodeConnCB
745a7af 
873beca [tests] Rename NodeConn and NodeConnCB (John Newbery)

Pull request description:

  Final step in bitcoin#11518

  NodeConn -> P2PConnection
  NodeConnCB -> P2PInterface

  This is basically just a rename. Should be an easy review.

Tree-SHA512: fe1204b2b3d8182c5e324ffa7cb4099a47ef8536380e0bb9d37a5fccf76a24f548d1f1a7988ab8f830986a3058b670696de3fc891af5e5f75dbeb4e3273005d7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 8, 2020
@PastaPastaPasta
Merge bitcoin#11791: [tests] Rename NodeConn and NodeConnCB
1ab6447 
873beca [tests] Rename NodeConn and NodeConnCB (John Newbery)

Pull request description:

  Final step in bitcoin#11518

  NodeConn -> P2PConnection
  NodeConnCB -> P2PInterface

  This is basically just a rename. Should be an easy review.

Tree-SHA512: fe1204b2b3d8182c5e324ffa7cb4099a47ef8536380e0bb9d37a5fccf76a24f548d1f1a7988ab8f830986a3058b670696de3fc891af5e5f75dbeb4e3273005d7
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 30, 2021
@gades
Merge bitcoin#11638: [tests] Dead mininode code
c89c8f2 
fb00c45 [tests] Explicitly disallow support for p2p versions below 60001 (John Newbery)
3858aab [tests] Remove support for p2p alert messages (John Newbery)
c0b1274 [tests] Remove support for bre-BIP31 ping messages (John Newbery)
2904e30 [tests] Remove dead code from mininode.py (John Newbery)

Pull request description:

  This is the first part of bitcoin#11518. It removes a ~150 lines of unused code from the mininode module:

  - remove unused `deliver_sleep_time` and `EarlyDisconnectError` code
  - remove support for pre-BIP31 ping messages
  - remove support for alert message
  - explicitly don't support p2p versions lower than 60001

  Should be an easy ACK for reviewers. If all extended tests pass, then this code really was dead :)

Tree-SHA512: 508e612ceb0b094250d18e75522d51e6b14cd069443050ba4af34d6f890c58721cb5653e8bc000b60635b9474d035b0dcd9c509c0dcdb3a7501df17b787f83b0

readd is None check

Signed-off-by: Pasta <pasta@dashboost.org>
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 30, 2021
@gades
Merge bitcoin#11791: [tests] Rename NodeConn and NodeConnCB
c616178 
873beca [tests] Rename NodeConn and NodeConnCB (John Newbery)

Pull request description:

  Final step in bitcoin#11518

  NodeConn -> P2PConnection
  NodeConnCB -> P2PInterface

  This is basically just a rename. Should be an easy review.

Tree-SHA512: fe1204b2b3d8182c5e324ffa7cb4099a47ef8536380e0bb9d37a5fccf76a24f548d1f1a7988ab8f830986a3058b670696de3fc891af5e5f75dbeb4e3273005d7
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@jamesob jamesob jamesob left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jnewbery @jamesob @fanquake