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

1. 
   jonatack commented at 4:46 am on May 11, 2024: contributor

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

   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.

Contributors
jonatack DrahtBot mzumsande brunoerg pinheadmz fanquake

Labels
P2P