-
Notifications
You must be signed in to change notification settings - Fork 36.6k
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
p2p: detect addnode cjdns peers in GetAddedNodeInfo() #30085
Conversation
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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
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
Github-Pull: bitcoin#30085 Rebased-From: d0b0474
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.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK d0b0474
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK d0b0474
There was a problem hiding this 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
Confirmed this branch fixes the issue in production on mainnet as well:
|
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
Github-Pull: bitcoin#30085 Rebased-From: d0b0474
Backported to 27.x in #30092. |
Github-Pull: bitcoin#30085 Rebased-From: d0b0474
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
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
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
22701a4 doc: update manual pages for 27.1rc1 (fanquake) 9e91907 build: bump version to 27.1rc1 (fanquake) 9b4640c doc: update release-notes.md for 27.1 (fanquake) 80032d6 qt: 27.1rc1 translations update (Hennadii Stepanov) 423bd6d windeploy: Renew certificate (Ava Chow) 77b2321 depends: Fetch miniupnpc sources from an alternative website (Hennadii Stepanov) 31adcfa test: add GetAddedNodeInfo() CJDNS regression unit test (Jon Atack) 9cdb9ed p2p, bugfix: detect addnode cjdns peers in GetAddedNodeInfo() (Jon Atack) 3c26058 crypto: disable asan for sha256_sse4 with clang and -O0 (Cory Fields) 0ba11cf rpc: move UniValue in blockToJSON (willcl-ark) dedf319 gui: don't permit port in proxy IP option (willcl-ark) d1289a1 gui: fix create unsigned transaction fee bump (furszy) Pull request description: Backports: * bitcoin-core/gui#812 * bitcoin-core/gui#813 * #30085 * #30094 * #30097 * #30149 * #30151 Bump to 27.1rc1. ACKs for top commit: stickies-v: re-ACK 22701a4 willcl-ark: reACK 22701a4 hebasto: re-ACK 22701a4. Tree-SHA512: 6eca44ba7e6664eb4677646597dfdaf56a241c8c3e95e0ab8929ee2fc3671303fc6c2634d359b4523dbd452ac5e54fd1f4c7c2bf7e9c5209395f8cb3b4753fb3
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
Github-Pull: bitcoin#30085 Rebased-From: d0b0474
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 connectedCConnman::ThreadOpenAddedConnections()
continually retries to connect themFix 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