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

pull jnewbery wants to merge 14 commits into bitcoin:master from jnewbery:node_conn_big_refactor changing 21 files +2098 −2293
  1. 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.
  2. fanquake added the label Tests on Oct 17, 2017
  3. [tests] Add p2p connection to TestNode
    p2p connections can now be added to TestNode instances.
    82fc1bc05c
  4. [tests] use TestNode p2p connection in tests b1ae3961b3
  5. [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
  6. [tests] Remove dead code from mininode.py
    Removes the dead deliver_sleep_time and EarlyDisconnectError code
    c7bf1febbb
  7. [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
  8. [tests] Tidy up mininode.py module
    Mostly move only. Adds a few extra comments.
    c0caa7ba9e
  9. [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
  10. [tests] Remove support for p2p alert messages
    Alert messages were removed in p2p version 70013 (Bitcoin Core V0.13.0)
    f5a8c5d0a4
  11. [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
  12. [tests] use add_p2p_connection in p2p-fingerprint.py dab9e22b82
  13. [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
  14. [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
  15. [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
  16. [tests] Rename NodeConn and NodeConnCB
    NodeConn -> P2PConnection
    NodeConnCB -> P2PInterface
    293459aee9
  17. jnewbery force-pushed on Oct 18, 2017
  18. 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
  19. 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).
  20. jamesob commented at 5:24 am on November 9, 2017: member
    Concept ACK. Looks good!
  21. MarcoFalke referenced this in commit 5e3f5e4f25 on Nov 9, 2017
  22. jnewbery commented at 8:53 pm on November 9, 2017: member
    Added #11648 for the next step.
  23. jnewbery commented at 10:10 pm on November 14, 2017: member
    #11677 covers removing the rpc property
  24. laanwj referenced this in commit ccc70a295f on Nov 17, 2017
  25. jnewbery commented at 9:48 pm on November 17, 2017: member
    Opened #11712 for splitting NodeConn and NodeConnCB
  26. MarcoFalke referenced this in commit 9f2c2dba21 on Nov 29, 2017
  27. 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.

  28. jnewbery closed this on Nov 29, 2017

  29. jnewbery deleted the branch on Nov 29, 2017
  30. MarcoFalke referenced this in commit 13e31dd654 on Nov 30, 2017
  31. PastaPastaPasta referenced this in commit d7757195e9 on Sep 11, 2019
  32. PastaPastaPasta referenced this in commit 7bd99943ae on Sep 11, 2019
  33. PastaPastaPasta referenced this in commit 3b53fa05d3 on Feb 13, 2020
  34. PastaPastaPasta referenced this in commit fd43baaac7 on Feb 13, 2020
  35. PastaPastaPasta referenced this in commit 713e899591 on Feb 13, 2020
  36. PastaPastaPasta referenced this in commit a42d62d89e on Feb 29, 2020
  37. PastaPastaPasta referenced this in commit 745a7afabd on Apr 6, 2020
  38. PastaPastaPasta referenced this in commit 1ab6447f10 on Apr 8, 2020
  39. ckti referenced this in commit da803f795c on Mar 28, 2021
  40. ckti referenced this in commit 817fd8a827 on Mar 28, 2021
  41. gades referenced this in commit c89c8f29e8 on Jun 30, 2021
  42. gades referenced this in commit c616178060 on Jun 30, 2021
  43. DrahtBot locked this on Sep 8, 2021


jnewbery jamesob

Labels
Tests


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-25 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me