BIP324 integration #28331

sipa · 2023-08-24T00:25:16Z

Part of #27634.

This makes BIP324 support feature complete, through a (default off) -v2transport option for enabling V2 connections. If it is enabled:

The NODE_P2P_V2 service flag (1 << 11) is advertized.
Inbound connections can use V1 or V2 (automatically detected based on the protocol used by the peer)
V2 connections are used on outbound when the NODE_P2P_V2 service is available (or the new use_v2 parameter is set on the addnode RPC).
V2 outbound connections that instantly fail get retried as V1.

There are two new RPC fields, "transport_protocol_type" and "session_id", in getpeerinfo.

DrahtBot · 2023-08-24T00:25:18Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, mzumsande, Sjors
Approach ACK	jonatack
Stale ACK	ajtowns, vasild

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/754 (Add BIP324-specific labels to peer details by hebasto)
#28464 (net: improve max-connection limits code by mzumsande)
#28463 (p2p: Increase inbound capacity for block-relay only connections by mzumsande)
#28155 (net: improves addnode / m_added_nodes logic by sr-gi)
#27600 (net: Add new permission forceinbound to evict a random unprotected connection if all slots are otherwise full by pinheadmz)
#27581 (net: Continuous ASMap health check by fjahr)
#27114 (p2p: Allow whitelisting outgoing connections by brunoerg)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
#26863 (test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg)
#26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
#26114 (net: Make AddrFetch connections to fixed seeds by mzumsande)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#22341 (rpc: add getxpub by Sjors)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors · 2023-08-25T11:53:39Z

Not needed for testing here, but it would still be cool to have a lock icon in the peers tab in the GUI, and reveal the session id if you click on it. cc @hebasto

Tested a manual connection between macOS and Ubuntu, found matching session ids, pfew - no man in the middle attack on my local network :-)

My DNS seed hasn't discovered any peers yet announcing the service flag, that could take a while. sipa/bitcoin-seeder#102

sipa · 2023-08-29T14:07:03Z

@Sjors Re #28196 (comment):

I've changed a net debug-level log message to say "trying v2 connection ... lastseen=...hrs\n"

sipa · 2023-10-02T22:29:07Z

Addressed nits by @mzumsande and @theStack:

diff --git a/doc/bips.md b/doc/bips.md
index 87b5918c72..952d289daa 100644
--- a/doc/bips.md
+++ b/doc/bips.md
@@ -49,7 +49,7 @@ BIPs that are implemented by Bitcoin Core:
 * [`BIP 173`](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki): Bech32 addresses for native Segregated Witness outputs are supported as of **v0.16.0** ([PR 11167](https://github.com/bitcoin/bitcoin/pull/11167)). Bech32 addresses are generated by default as of **v0.20.0** ([PR 16884](https://github.com/bitcoin/bitcoin/pull/16884)).
 * [`BIP 174`](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki): RPCs to operate on Partially Signed Bitcoin Transactions (PSBT) are present as of **v0.17.0** ([PR 13557](https://github.com/bitcoin/bitcoin/pull/13557)).
 * [`BIP 176`](https://github.com/bitcoin/bips/blob/master/bip-0176.mediawiki): Bits Denomination [QT only] is supported as of **v0.16.0** ([PR 12035](https://github.com/bitcoin/bitcoin/pull/12035)).
-* [`BIP 324`](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki): Support for the BIP324 v2 transport protocol and the associated `NODE_P2P_V2` service bit is supported as of **v26.0**, but off by default ([PR 28331](https://github.com/bitcoin/bitcoin/pull/28331)).
+* [`BIP 324`](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki): The v2 transport protocol specified by BIP324 and the associated `NODE_P2P_V2` service bit are supported as of **v26.0**, but off by default ([PR 28331](https://github.com/bitcoin/bitcoin/pull/28331)).
 * [`BIP 325`](https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki): Signet test network is supported as of **v0.21.0** ([PR 18267](https://github.com/bitcoin/bitcoin/pull/18267)).
 * [`BIP 339`](https://github.com/bitcoin/bips/blob/master/bip-0339.mediawiki): Relay of transactions by wtxid is supported as of **v0.21.0** ([PR 18044](https://github.com/bitcoin/bitcoin/pull/18044)).
 * [`BIP 340`](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki)
diff --git a/src/net.cpp b/src/net.cpp
index c4c2d55c77..6b2ef5f43d 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -469,7 +469,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
         const std::vector<CService> resolved{Lookup(pszDest, default_port, fNameLookup && !HaveNameProxy(), 256)};
         if (!resolved.empty()) {
             const CService& rnd{resolved[GetRand(resolved.size())]};
-            addrConnect = CAddress{MaybeFlipIPv6toCJDNS(rnd), addrConnect.nServices};
+            addrConnect = CAddress{MaybeFlipIPv6toCJDNS(rnd), NODE_NONE};
             if (!addrConnect.IsValid()) {
                 LogPrint(BCLog::NET, "Resolver returned invalid address %s for %s\n", addrConnect.ToStringAddrPort(), pszDest);
                 return nullptr;

theStack

re-ACK 75a3291

mzumsande · 2023-10-02T23:59:15Z

re-ACK 75a3291

Sjors · 2023-10-03T09:12:40Z

re-ACK 75a3291

vasild · 2023-10-03T09:36:47Z

ACK 75a3291

rsantacroce · 2023-10-04T08:13:17Z

any clear reason why noise protocol hasn't been considered here ?

Sjors · 2023-10-04T08:22:16Z

@rsantacroce the BIP explains this under "Why not use a general-purpose transport encryption protocol?": https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki

rsantacroce · 2023-10-04T08:35:20Z

Thank you, i missed that one! very good explanation now it's the other way around why we don't use it on stratumv2 .... this is for the other repo. thank you and thank you for the great work.

d9c4e34 qt: Add "Session id" label to peer details (Hennadii Stepanov) f08adec qt: Add "Transport" label to peer details (Hennadii Stepanov) Pull request description: This PR adds BIP324-specific labels to the peer details widget: - a transport version - a session id See: bitcoin/bitcoin#28331 (comment). ![image](https://github.com/bitcoin-core/gui/assets/32963518/0e0b4c92-dde0-4b2e-b285-a2c69ef09efc) ACKs for top commit: achow101: ACK d9c4e34 RandyMcMillan: ACK d9c4e34 theStack: Tested re-ACK d9c4e34 MarnixCroes: tACK d9c4e34 Tree-SHA512: 524e634b1eedcae535d5c2a10dd8dea90d33d65d6790614f86ab6387a0ec9ee4bbc7646b8c20a5b3f1bccbb69bc52a46514e2b28cb4588979834d945b1f89b3a

kashifs · 2023-11-10T16:04:29Z

test/functional/p2p_v2_transport.py

+        assert_equal(self.nodes[1].getblockcount(), 5)
+        # verify there is a v2 connection between node 0 and 1
+        node_0_info = self.nodes[0].getpeerinfo()
+        node_1_info = self.nodes[0].getpeerinfo()


@sipa, am I missing something or should this line read:

node_1_info = self.nodes[1].getpeerinfo()

This indeed looks like a bug.

cc @theStack

Good catch @kashifs. Do you want to open a PR for the fix? If not (for whatever reason), I'm happy to do it and add the commit with your authorship.

I've opened a PR here. Please let me know if it's done correctly and if there is more that I should do.

…patible with --v2transport 35fb993 test: enable v2 transport for p2p_timeouts.py (Martin Zumsande) 2c1669c test: enable v2 transport for rpc_net.py (Sebastian Falbesoner) cc961c2 test: enable v2 transport for p2p_node_network_limited.py (Sebastian Falbesoner) 3598a1b test: enable --v2transport in combination with --usecli (Martin Zumsande) 68a9001 test: persist -v2transport over restarts and respect -v2transport=0 (Martin Zumsande) Pull request description: This makes the functional test suite compatible with BIP324, so that `python3 test_runner.py --v2transport` should succeed (currently, 12 tests fail for me on master). Includes two commits by TheStack I found in an old discussion bitcoin/bitcoin#28331 (comment) Note that even though all tests should pass, the python `p2p.py` module will do v2 connections only after the merge of #24748, so that for now only connections between two full nodes will actually run v2. Some of the fixed tests were added with `--v2transport` to the test runner. Though after #24748 we might also want to consider running the entire suite with `--v2transport` in some CI. ACKs for top commit: sipa: utACK 35fb993. Thanks for taking care of this. achow101: ACK 35fb993 theStack: ACK 35fb993 stratospher: ACK 35fb993. Tree-SHA512: 80dc0bf211fa525ff1d092043aea9f222f14c02e5832a548fb8b83b9ede1fcee03c5e8ade0d05c331bdaa492af9c1cf3d0f0b15b846673c6eacea82dd4cefbc3

fanquake · 2023-12-07T13:22:20Z

Removing label, as this got a release note.

This was referenced Aug 24, 2023

BIP324 connection support #28196

Merged

BIP324 tracking issue #27634

Closed

sipa force-pushed the 202308_bip324_integration branch from 9454cbd to 06d98ea Compare August 24, 2023 01:17

DrahtBot added the CI failed label Aug 24, 2023

DrahtBot removed the CI failed label Aug 24, 2023

sipa force-pushed the 202308_bip324_integration branch from 06d98ea to 5e071a4 Compare August 24, 2023 13:03

glozow added the P2P label Aug 24, 2023

sipa force-pushed the 202308_bip324_integration branch 2 times, most recently from 06a00c4 to 35f0384 Compare August 28, 2023 01:17

DrahtBot added the CI failed label Aug 28, 2023

sipa force-pushed the 202308_bip324_integration branch from 35f0384 to bdb7f73 Compare August 29, 2023 02:58

DrahtBot removed the CI failed label Aug 29, 2023

sipa force-pushed the 202308_bip324_integration branch from bdb7f73 to 3f959b9 Compare August 29, 2023 20:08

sipa mentioned this pull request Aug 29, 2023

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

DrahtBot added the CI failed label Aug 29, 2023

sipa force-pushed the 202308_bip324_integration branch from 3f959b9 to 1f13994 Compare August 29, 2023 22:14

sipa added 2 commits October 2, 2023 18:11

test: enable v2 transport between nodes in some functional tests

64ca721

doc: mention BIP324 support in bips.md

75a3291

sipa force-pushed the 202308_bip324_integration branch from 7b255b5 to 75a3291 Compare October 2, 2023 22:26

theStack approved these changes Oct 2, 2023

View reviewed changes

DrahtBot requested review from Sjors and mzumsande October 2, 2023 23:28

DrahtBot removed the request for review from mzumsande October 2, 2023 23:59

DrahtBot removed the request for review from Sjors October 3, 2023 09:12

fanquake merged commit 6f882e6 into bitcoin:master Oct 3, 2023
16 checks passed

benthecarman mentioned this pull request Oct 3, 2023

October topics austinbitdevs/austinbitdevs.com#26

Closed

sipa added the Needs release note label Oct 4, 2023

reez mentioned this pull request Oct 8, 2023

Socratic Seminar #12 Topics NashBitDevs/nashbitdevs.org#14

Closed

lorenzolfm mentioned this pull request Oct 10, 2023

Tópicos Outubro saopaulobitdevs/saopaulobitdevs.org#14

Closed

Sjors mentioned this pull request Oct 11, 2023

Test calling addnode more than once (and MAX_ADDNODE_CONNECTIONS) #28635

Open

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023

Merge bitcoin#28331: BIP324 integration

95e4527

ajtowns mentioned this pull request Oct 20, 2023

refactor: Split per-peer parts of net module into new node/connection module #28686

Closed

mzumsande mentioned this pull request Nov 6, 2023

test: Make existing functional tests compatible with --v2transport #28805

Merged

kashifs reviewed Nov 10, 2023

View reviewed changes

stratospher mentioned this pull request Nov 27, 2023

[Part 2/2] Functional tests for v2 P2P encryption #24748 Bitshala/BitcoinCore-PR-Review-Club#48

Closed

fanquake removed the Needs release note label Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BIP324 integration #28331

BIP324 integration #28331

sipa commented Aug 24, 2023 •

edited by fanquake

DrahtBot commented Aug 24, 2023 •

edited

Sjors commented Aug 25, 2023 •

edited

sipa commented Aug 29, 2023

sipa commented Oct 2, 2023

theStack left a comment

mzumsande commented Oct 2, 2023

Sjors commented Oct 3, 2023

vasild commented Oct 3, 2023

rsantacroce commented Oct 4, 2023

Sjors commented Oct 4, 2023

rsantacroce commented Oct 4, 2023 •

edited

kashifs Nov 10, 2023

sipa Nov 10, 2023

fanquake Nov 10, 2023

theStack Nov 10, 2023

kashifs Nov 10, 2023 •

edited

fanquake commented Dec 7, 2023

BIP324 integration #28331

BIP324 integration #28331

Conversation

sipa commented Aug 24, 2023 • edited by fanquake

DrahtBot commented Aug 24, 2023 • edited

Reviews

Conflicts

Sjors commented Aug 25, 2023 • edited

sipa commented Aug 29, 2023

sipa commented Oct 2, 2023

theStack left a comment

Choose a reason for hiding this comment

mzumsande commented Oct 2, 2023

Sjors commented Oct 3, 2023

vasild commented Oct 3, 2023

rsantacroce commented Oct 4, 2023

Sjors commented Oct 4, 2023

rsantacroce commented Oct 4, 2023 • edited

kashifs Nov 10, 2023

Choose a reason for hiding this comment

sipa Nov 10, 2023

Choose a reason for hiding this comment

fanquake Nov 10, 2023

Choose a reason for hiding this comment

theStack Nov 10, 2023

Choose a reason for hiding this comment

kashifs Nov 10, 2023 • edited

Choose a reason for hiding this comment

fanquake commented Dec 7, 2023

sipa commented Aug 24, 2023 •

edited by fanquake

DrahtBot commented Aug 24, 2023 •

edited

Sjors commented Aug 25, 2023 •

edited

rsantacroce commented Oct 4, 2023 •

edited

kashifs Nov 10, 2023 •

edited