p2p: detect addnode cjdns peers in GetAddedNodeInfo() #30085

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:2024-05-fix-cjdns-detection-in-GetAddedNodeInfo changing 2 files +7 −1
  1. jonatack commented at 4:46 am on May 11, 2024: member

    Addnode peers connected to us via the cjdns network are currently not detected by CConnman::GetAddedNodeInfo(), i.e. fConnected is always false. This causes the following issues:

    • RPC getaddednodeinfo incorrectly shows them as not connected

    • CConnman::ThreadOpenAddedConnections() continually retries to connect them

    Fix the issue and add a unit regression test. Extracted from #28248. Suggest running the test with:

    ./src/test/test_bitcoin -t net_peer_connection_tests -l test_suite

  2. p2p, bugfix: detect addnode cjdns peers in GetAddedNodeInfo()
    Addnode (manual) peers connected to us via the cjdns network are currently not
    detected by CConnman::GetAddedNodeInfo(), i.e. fConnected is always false.
    
    This causes the following issues:
    
    - RPC `getaddednodeinfo` incorrectly shows them as not connected
    
    - CConnman::ThreadOpenAddedConnections() continually retries to connect them
    684da97070
  3. test: add GetAddedNodeInfo() CJDNS regression unit test d0b047494c
  4. DrahtBot commented at 4:46 am on May 11, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK mzumsande, brunoerg, pinheadmz

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  5. DrahtBot added the label P2P on May 11, 2024
  6. luke-jr referenced this in commit 197c837b19 on May 13, 2024
  7. luke-jr referenced this in commit 7ab607330d on May 13, 2024
  8. jonatack commented at 7:01 pm on May 13, 2024: member

    This straightforward fix (5f53e6c073d089598a444436d74e1f5c1615030b) was previously ACKed by @vasild (https://github.com/bitcoin/bitcoin/pull/28248#pullrequestreview-1695032063 and Concept ACKed by @brunoerg (https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1777344970) with a request to add test coverage, which I’ve added here.

    (Thank you @luke-jr for updating bitcoin knots.) @vasild @brunoerg mind having a look (thanks!)

  9. mzumsande commented at 9:41 pm on May 14, 2024: contributor
    utACK d0b047494c28381942c09d0cca45baa323bfcffc
  10. brunoerg approved
  11. brunoerg commented at 9:34 am on May 15, 2024: contributor
    crACK d0b047494c28381942c09d0cca45baa323bfcffc
  12. pinheadmz approved
  13. pinheadmz commented at 6:05 pm on May 15, 2024: member

    ACK d0b047494c28381942c09d0cca45baa323bfcffc

    Built and tested on arm/macOS. It’s a simple fix to recognize CJDNS addresses in GetAddedNodeInfo(). Otherwise CService::IsValid() will fail because the CJDNS prefix is in a reserved range. Confirmed the test fails on master and passes with the branch.

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA256
     2
     3ACK d0b047494c28381942c09d0cca45baa323bfcffc
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmZE+NUACgkQ5+KYS2KJ
     7yTqRLA/+PTzgnOENcfPRDjRTqOrwzxdHPDW8xHu+m0VcLZO0Uv4HwIW8ArZk7DJB
     8va3z0Y4gIWYvpLwLR+pUlSFqsLKmgsi+gYSVkGK3sGBAGvTH8ibNfKWKdSxzQaAs
     9PHkutSbYFchel2aO7g/cVzNsD4IfbWvGMmxj2f4kNX0RcWG49IHi3xlMh7ppqn8x
    10NjS+Xl8kzCgz8oYPQVh7KMKOLEeu+FcoaKRrWhgkhWn6A+CNRbyxwcZYXWrx1oY5
    11uK+tgnnbvRoYaTg7BTluFfitOqUGPnsMWw52a+vPbtq0lMbtLGby6nPrDwlr1jCn
    12HPMxz4klxHoqYXJa138Exywyck0Ziu4eqeq/jzi1vQO4KBm6MaiHWrBpfjr/i5uS
    13Vc/eWPzlTHXktT4MDOb2Mtgfrcu35zQlaU4HMCkLnJGhtFDRhiab99kGHbq+h/LF
    14kVrePOsVStPNg0s5UwxQIYdEBw4UXePCCPRwe9xxi7UoGGq8mmX6W7AFFn90bepQ
    15n6a579L6y9olxEr0+Xh863B0esen6wh7znCIm8YJfBHoNJZmOe3tTiWNn0+HQF9e
    16lqat+HDHY6H24jWSSlqpIvYtHMMe/FeP/5rNldWIdLxIAl3Nc3RPnfmYOTnnZv7F
    17djr3j+Tk1sIl7hKNqYg9eFcoIzRyLqecJOPDxVrK3eCPVdM12OA=
    18=u4ll
    19-----END PGP SIGNATURE-----
    

    pinheadmz’s public key is on keybase

  14. pinheadmz commented at 7:43 pm on May 15, 2024: member

    Confirmed this branch fixes the issue in production on mainnet as well:

     0$ bccli getaddednodeinfo
     1[
     2  {
     3    "addednode": "fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa",
     4    "connected": false,
     5    "addresses": [
     6    ]
     7  },
     8  {
     9    "addednode": "fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce",
    10    "connected": true,
    11    "addresses": [
    12      {
    13        "address": "[fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce]:8333",
    14        "connected": "outbound"
    15      }
    16    ]
    17  }
    18]
    
  15. fanquake merged this on May 16, 2024
  16. fanquake closed this on May 16, 2024

  17. jonatack deleted the branch on May 16, 2024
  18. fanquake referenced this in commit 9cdb9edfb8 on May 22, 2024
  19. fanquake referenced this in commit e90635f21e on May 22, 2024
  20. fanquake commented at 9:59 am on May 22, 2024: member
    Backported to 27.x in #30092.
  21. fanquake referenced this in commit 31adcfa171 on May 23, 2024
  22. glozow referenced this in commit a2efacb6fc on May 23, 2024
  23. glozow referenced this in commit 21d9aaae32 on May 23, 2024
  24. glozow referenced this in commit 6d7a1e3670 on May 24, 2024
  25. fanquake referenced this in commit fccd32efe6 on May 29, 2024
  26. luke-jr referenced this in commit 4421251d7d on Jun 13, 2024
  27. luke-jr referenced this in commit ac5c317924 on Jun 13, 2024
  28. bitcoin deleted a comment on Sep 5, 2024

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-11 06:12 UTC

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