Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

jonatack
Copy link
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

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
@DrahtBot
Copy link
Contributor

DrahtBot commented May 11, 2024

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.

@DrahtBot DrahtBot added the P2P label May 11, 2024
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request May 13, 2024
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

Github-Pull: bitcoin#30085
Rebased-From: 684da97
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request May 13, 2024
@jonatack
Copy link
Contributor Author

jonatack commented May 13, 2024

This straightforward fix (5f53e6c) was previously ACKed by @vasild (#28248 (review) and Concept ACKed by @brunoerg (#28248 (comment)) 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!)

Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK d0b0474

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crACK d0b0474

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d0b0474

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.

Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK d0b047494c28381942c09d0cca45baa323bfcffc
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmZE+NUACgkQ5+KYS2KJ
yTqRLA/+PTzgnOENcfPRDjRTqOrwzxdHPDW8xHu+m0VcLZO0Uv4HwIW8ArZk7DJB
va3z0Y4gIWYvpLwLR+pUlSFqsLKmgsi+gYSVkGK3sGBAGvTH8ibNfKWKdSxzQaAs
PHkutSbYFchel2aO7g/cVzNsD4IfbWvGMmxj2f4kNX0RcWG49IHi3xlMh7ppqn8x
NjS+Xl8kzCgz8oYPQVh7KMKOLEeu+FcoaKRrWhgkhWn6A+CNRbyxwcZYXWrx1oY5
uK+tgnnbvRoYaTg7BTluFfitOqUGPnsMWw52a+vPbtq0lMbtLGby6nPrDwlr1jCn
HPMxz4klxHoqYXJa138Exywyck0Ziu4eqeq/jzi1vQO4KBm6MaiHWrBpfjr/i5uS
Vc/eWPzlTHXktT4MDOb2Mtgfrcu35zQlaU4HMCkLnJGhtFDRhiab99kGHbq+h/LF
kVrePOsVStPNg0s5UwxQIYdEBw4UXePCCPRwe9xxi7UoGGq8mmX6W7AFFn90bepQ
n6a579L6y9olxEr0+Xh863B0esen6wh7znCIm8YJfBHoNJZmOe3tTiWNn0+HQF9e
lqat+HDHY6H24jWSSlqpIvYtHMMe/FeP/5rNldWIdLxIAl3Nc3RPnfmYOTnnZv7F
djr3j+Tk1sIl7hKNqYg9eFcoIzRyLqecJOPDxVrK3eCPVdM12OA=
=u4ll
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

@pinheadmz
Copy link
Member

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

$ bccli getaddednodeinfo
[
  {
    "addednode": "fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa",
    "connected": false,
    "addresses": [
    ]
  },
  {
    "addednode": "fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce",
    "connected": true,
    "addresses": [
      {
        "address": "[fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce]:8333",
        "connected": "outbound"
      }
    ]
  }
]

@fanquake fanquake merged commit dd42a5d into bitcoin:master May 16, 2024
16 checks passed
@jonatack jonatack deleted the 2024-05-fix-cjdns-detection-in-GetAddedNodeInfo branch May 16, 2024 04:07
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 22, 2024
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

Github-Pull: bitcoin#30085
Rebased-From: 684da97
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 22, 2024
@fanquake
Copy link
Member

Backported to 27.x in #30092.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 23, 2024
glozow pushed a commit to glozow/bitcoin that referenced this pull request May 23, 2024
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

Github-Pull: bitcoin#30085
Rebased-From: 684da97
glozow pushed a commit to glozow/bitcoin that referenced this pull request May 23, 2024
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

Github-Pull: bitcoin#30085
Rebased-From: 684da97
glozow added a commit that referenced this pull request May 24, 2024
aa7e876 [doc] add draft release notes for 26.2rc1 (glozow)
21d9aaa p2p, bugfix: detect addnode cjdns peers in GetAddedNodeInfo() (Jon Atack)
ec5ce2f windeploy: Renew certificate (Ava Chow)
96d0e81 rpc: Reword SighashFromStr error message (MarcoFalke)
6685aff rpc: move UniValue in blockToJSON (willcl-ark)
7f45e00 depends: Fix build of Qt for 32-bit platforms (laanwj)
f9b76ba ci: Pull in qtbase5-dev instead of seperate low-level libraries (laanwj)
c587753 doc: Suggest installing dev packages for debian/ubuntu qt5 build (laanwj)
7ecdb08 ci: Bump s390x to ubuntu:24.04 (MarcoFalke)
d9ef6cf sign: don't assume we are parsing a sane Miniscript (Antoine Poinsot)
e4859c8 depends: fix mingw-w64 Qt DEBUG=1 build (fanquake)
bb46b90 Fix #29767, set m_synced = true after Commit() (nanlour)
bf5b6fc Throw error if invalid parameters passed to getnetworkhashps RPC endpoint (Jameson Lopp)
a81a922 [rpc, bugfix] Enforce maximum value for setmocktime (dergoegge)
d39ea51 Change Luke Dashjr seed to dashjr-list-of-p2p-nodes.us (Luke Dashjr)
c21bbcc [doc] archive 26.1 release notes (glozow)

Pull request description:

  Archives 26.1 release notes and adds draft release notes for 26.2rc1

  Also backports:
  - #29691
  - #29869
  - #28554
  - #29747
  - #29853
  - #29856
  - #29764
  - #29776
  - #29985
  - #30094
  - #29870
  - #30149
  - #30085

ACKs for top commit:
  stickies-v:
    re-ACK aa7e876, only changes are fixing commit msg and transifex reference
  willcl-ark:
    ACK aa7e876

Tree-SHA512: b81ba6092640de696d782114cdf43e7ed1d63ea0a3231cade30653c2743d87700e0f852a1b1fcc42ae313b2d4f004e6026ddbad87d58c2fde0a660e90026ed98
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants