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

jnewbery commented at 8:47 pm on October 17, 2017: member

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 #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:

#11638 - Remove dead code from mininode
#11648 - split mininode.py into mininode.py and primitives.py
#11648 - tidy up mininode module
#11677 - remove the rpc property from NodeConn
#11712 properly split NodeConn from NodeConnCB
#11791 (optional) rename NodeConn and NodeConnCB

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 added the label Tests on Oct 17, 2017

[tests] Add p2p connection to TestNode

p2p connections can now be added to TestNode instances.

82fc1bc05c

[tests] use TestNode p2p connection in tests b1ae3961b3

[tests] Move test_framework Bitcoin primitives into separate module

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

64d947b82d

[tests] Remove dead code from mininode.py

Removes the dead deliver_sleep_time and EarlyDisconnectError code

c7bf1febbb

[tests] Remove support for bre-BIP31 ping messages

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.

b8c6d95b52

[tests] Tidy up mininode.py module

Mostly move only. Adds a few extra comments.

c0caa7ba9e

[tests] Explicitly disallow support for p2p versions below 60001

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

999167764b

[tests] Remove support for p2p alert messages

Alert messages were removed in p2p version 70013 (Bitcoin Core V0.13.0)

f5a8c5d0a4

[tests] Remove rpc property from NodeConn.

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.

333012d548

[tests] use add_p2p_connection in p2p-fingerprint.py dab9e22b82

[tests] Better document the distinction between NodeConn and NodeConnCB

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.

5765169080

[tests] Make NodeConnCB a subclass of NodeConn

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.

afa6fc1eee

[tests] Move version message sending from NodeConn to NodeConnCB

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.

cf236f41e1

[tests] Rename NodeConn and NodeConnCB

NodeConn -> P2PConnection
NodeConnCB -> P2PInterface

293459aee9

jnewbery force-pushed on Oct 18, 2017

jnewbery renamed this:
~~[WIP] [tests] Simplify/clarify the NodeConn/NodeConnCB mininode classes~~
[tests] [demonstration] Simplify/clarify the NodeConn/NodeConnCB mininode classes
on Nov 8, 2017

in test/functional/test_framework/mininode.py:23 in 293459aee9

44-MY_SUBVERSION = b"/python-mininode-tester:0.0.3/"
45-MY_RELAY = 1 # from version 70001 onwards, fRelay should be appended to version messages (BIP37)
46-
47-MAX_INV_SZ = 50000
48-MAX_BLOCK_BASE_SIZE = 1000000
49+from test_framework.primitives import *

jamesob commented at 5:16 am on November 9, 2017:

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

jnewbery commented at 2:55 pm on November 9, 2017:

Yeah, it’s ugly. See the commit message for 64d947b82df924c101f0fd00f4891177a2d0ed44:

0mininode.py wildcard imports all names from primitives.py. This is
1to 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)

jamesob commented at 5:41 pm on November 9, 2017:

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

jamesob commented at 5:24 am on November 9, 2017: member

Concept ACK. Looks good!

MarcoFalke referenced this in commit 5e3f5e4f25 on Nov 9, 2017

jnewbery commented at 8:53 pm on November 9, 2017: member

Added #11648 for the next step.

jnewbery commented at 10:10 pm on November 14, 2017: member

#11677 covers removing the rpc property

laanwj referenced this in commit ccc70a295f on Nov 17, 2017

jnewbery commented at 9:48 pm on November 17, 2017: member

Opened #11712 for splitting NodeConn and NodeConnCB

MarcoFalke referenced this in commit 9f2c2dba21 on Nov 29, 2017

jnewbery commented at 10:18 pm on November 29, 2017: member

Opened #11791 to rename NodeConn and NodeConnCB.

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

jnewbery closed this on Nov 29, 2017

jnewbery deleted the branch on Nov 29, 2017

MarcoFalke referenced this in commit 13e31dd654 on Nov 30, 2017

PastaPastaPasta referenced this in commit d7757195e9 on Sep 11, 2019

PastaPastaPasta referenced this in commit 7bd99943ae on Sep 11, 2019

PastaPastaPasta referenced this in commit 3b53fa05d3 on Feb 13, 2020

PastaPastaPasta referenced this in commit fd43baaac7 on Feb 13, 2020

PastaPastaPasta referenced this in commit 713e899591 on Feb 13, 2020

PastaPastaPasta referenced this in commit a42d62d89e on Feb 29, 2020

PastaPastaPasta referenced this in commit 745a7afabd on Apr 6, 2020

PastaPastaPasta referenced this in commit 1ab6447f10 on Apr 8, 2020

ckti referenced this in commit da803f795c on Mar 28, 2021

ckti referenced this in commit 817fd8a827 on Mar 28, 2021

gades referenced this in commit c89c8f29e8 on Jun 30, 2021

gades referenced this in commit c616178060 on Jun 30, 2021

DrahtBot locked this on Sep 8, 2021

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