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 aNodeConn
connection to thatNodeConnCB
, while passing theNodeConnCB
into theNodeConn
as an argument. The internal interface between theNodeConn
andNodeConnCB
is equally tortuous. Note that #11182 hides that complexity from the user behindTestNode
, 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 fromNodeConn
- #11712 properly split
NodeConn
fromNodeConnCB
- #11791 (optional) rename
NodeConn
andNodeConnCB
some notes on the implementation:
- The main structural change in this PR is that the old
NodeConnCB
class is now a subclass ofNodeConn
. Individual tests and theTestNode
class will only need to interact with theNodeConnCB
class. NodeConn
has been renamedP2PConnection
. 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 renamedP2PInterface
. 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 theNodeConnCB
class, which required a lot of lines of (completely mechanical) change to the p2p-segwit test. Conceptually it doesn’t make sense for aNodeConnCB
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.