BIP324 integration by sipa · Pull Request #28331 · bitcoin/bitcoin

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"

theStack

ACK 7b255b5

doc/bips.md

mzumsande

ACK 7b255b5

src/net.cpp

Co-authored-by: Dhruv Mehta <[email protected]>

…tics This matches the behavior for per-type received bytes.

Co-authored-by: Dhruv Mehta <[email protected]>

Co-authored-by: Pieter Wuille <[email protected]>

When an outbound v2 connection is disconnected without receiving anything, but at least 24 bytes of our pubkey were sent out (enough to constitute an invalid v1 header), add them to a queue of reconnections to be tried. The reconnections are in a queue rather than performed immediately, because we should not block the socket handler thread with connection creation (a blocking operation that can take multiple seconds).

Co-authored-by: Dhruv Mehta <[email protected]>

Co-authored-by: Pieter Wuille <[email protected]>

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.

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.

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

theStack approved these changes Oct 2, 2023

View reviewed changes

doc/bips.md Outdated Show resolved Hide resolved

mzumsande reviewed Oct 2, 2023

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

sipa and others added 10 commits October 2, 2023 18:09

net: advertise NODE_P2P_V2 if CLI arg -v2transport is on

abf343b

Co-authored-by: Dhruv Mehta <[email protected]>

rpc: don't report v2 handshake bytes in the per-type sent byte statis…

a4706bc

…tics This matches the behavior for per-type received bytes.

net: use V2Transport when NODE_P2P_V2 service flag is present

62d21ee

Co-authored-by: Dhruv Mehta <[email protected]>

rpc: addnode arg to use BIP324 v2 p2p

c73cd42

Co-authored-by: Pieter Wuille <[email protected]>

sync: modernize CSemaphore / CSemaphoreGrant

4d265d0

net: expose transport types/session IDs of connections in RPC and logs

b815cce

Co-authored-by: Dhruv Mehta <[email protected]>

test: Functional test for opportunistic encryption

05d19fb

Co-authored-by: Pieter Wuille <[email protected]>

test: enable v2 transport between nodes in some functional tests

64ca721

doc: mention BIP324 support in bips.md

75a3291

theStack approved these changes Oct 2, 2023

View reviewed changes

benthecarman mentioned this pull request Oct 3, 2023

October topics austinbitdevs/austinbitdevs.com#26

Closed

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

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

kwvg mentioned this pull request Oct 9, 2024

backport: merge bitcoin#28577, #28588, #28634, #28849, #28805, #28951, #29058, #29239, partial bitcoin#28331, #28452 (BIP324 backports: part 2) dashpay/dash#6324

Merged

5 tasks

Conversation

sipa commented Aug 24, 2023 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Sjors commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Aug 29, 2023

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sipa commented Oct 2, 2023

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Oct 2, 2023

Uh oh!

Sjors commented Oct 3, 2023

Uh oh!

vasild commented Oct 3, 2023

Uh oh!

rsantacroce commented Oct 4, 2023

Uh oh!

Sjors commented Oct 4, 2023

Uh oh!

rsantacroce commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kashifs Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

sipa Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

theStack Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

kashifs Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Dec 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants

sipa commented Aug 24, 2023 •

edited by fanquake

Loading

DrahtBot commented Aug 24, 2023 •

edited

Loading

Sjors commented Aug 25, 2023 •

edited

Loading

rsantacroce commented Oct 4, 2023 •

edited

Loading

kashifs Nov 10, 2023 •

edited

Loading