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"

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.

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> excludes: - changes to `src/rpc/util.cpp`

, bitcoin#28849, bitcoin#28805, bitcoin#28951, bitcoin#29058, bitcoin#29239, partial bitcoin#28331, bitcoin#28452 (BIP324 backports: part 2) 5dd60c4 merge bitcoin#29239: Make v2transport default for addnode RPC when enabled (Kittywhiskers Van Gogh) b2ac426 merge bitcoin#29058: use v2transport for manual/addrfetch connections, add to -netinfo (Kittywhiskers Van Gogh) 92e862a merge bitcoin#28951: damage ciphertext/aad in full byte range (Kittywhiskers Van Gogh) 4e96e26 merge bitcoin#28805: Make existing functional tests compatible with --v2transport (Kittywhiskers Van Gogh) 9371e2e merge bitcoin#28849: fix node index bug when comparing peerinfo (Kittywhiskers Van Gogh) 400c9dd merge bitcoin#28634: add check for missing garbage terminator detection (Kittywhiskers Van Gogh) 65eb194 merge bitcoin#28588: add checks for v1 prefix matching / wrong network magic detection (Kittywhiskers Van Gogh) 6074974 merge bitcoin#28577: raise V1_PREFIX_LEN from 12 to 16 (Kittywhiskers Van Gogh) ff92d1a partial bitcoin#28331: BIP324 integration (Kittywhiskers Van Gogh) f9f8805 fix: drop `CConnman::mapNodesWithDataToSend`, use transport data (UdjinM6) d39d8a4 partial bitcoin#28452: Do not use std::vector = {} to release memory (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6280 * Dependent on #6276 * Dependency for #6329 * [bitcoin#28452](bitcoin#28452) was backported as the behavior it introduces is required for backports like [bitcoin#28331](bitcoin#28331). As it pre-dates earlier BIP324 backports, the `ClearShrink` usage from those backports have also been incorporated into this backport. * When backporting [bitcoin#28331](bitcoin#28331), in TestFramework's `add_nodes()`, `extra_args[i]` is not converted to a `list` like it is done upstream and avoiding duplication of `-v2transport=1` is instead done by an additional check. This is done to prevent test failures in `feature_mnehf.py`. * The local variable `args` is removed in [bitcoin#28805](bitcoin#28805) as it does nothing (the argument appending logic is removed as part of the backport) and upstream removes it anyways. Special thanks to UdjinM6 for significant contributions to this and dependent PRs! 🎉 ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: light ACK 5dd60c4 PastaPastaPasta: utACK 5dd60c4 Tree-SHA512: 7f3d0e54e1c96fc99b2d6b1e64485110aa73aeec0e4e4245ed4e6dc0529a7608e9c39103af636d5945d289775c40d3d15d7d4a75bea82462194dbf355fca48dc

…ct logging 06443b8 net: clarify if we ever sent or received from peer (Sjors Provoost) 1d01ad4 net: add LogIP() helper, use in net_processing (Sjors Provoost) 937ef9e net_processing: use CNode::DisconnectMsg helper (Sjors Provoost) ad22442 net: additional disconnection logging (Sjors Provoost) Pull request description: While debugging unexpected disconnections, possibly related to #28331, I found some additional [net] logging to be useful. All cases where we disconnect now come with a log message that has the word `disconnecting`: * all calls to `CloseSocketDisconnect()` log `disconnecting peer=…` * wherever we set `pnode->fDisconnect = true;` * for all `InactivityCheck` cases (which in turn sets `fDisconnect`) * replaces "dropping" with "disconnecting" in `Network not active, dropping peer=…` A few exceptions are listed here: #28521 (comment) I changed `CloseSocketDisconnect()` to no longer log `disconnecting`, and instead have all the call sites do so. This PR introduces two helper functions on `CNode`: `DisconnectMsg` and `LogIP`. The second and third commit use these helpers in `net_processing.cpp` so these disconnect messages are more consistent now (e.g. some didn't log the IP). No new messages are added there though. The `LogIP()` helper is rarely used outside of a disconnect event, but it's available for future use. Any `LogPrint` this PR touches is replaced with `LogDebug` (superseded by #30750), and every `LogPrintf ` with `LogInfo`. ACKs for top commit: davidgumberg: reACK 06443b8 vasild: ACK 06443b8 danielabrozzoni: ACK 06443b8 hodlinator: ACK 06443b8 Tree-SHA512: 525f4c11568616e1d48455a3fcab9e923da7432377fe9230468c15403d2e9b7ce712112df8fbd547cfec01dce0d1f26107cfc1b90f78cfc1fe13e08d57b08464

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

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

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

kwvg added a commit to kwvg/dash that referenced this pull request Oct 9, 2024

merge bitcoin#28331: BIP324 integration

4917be4

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

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

kwvg added a commit to kwvg/dash that referenced this pull request Oct 14, 2024

partial bitcoin#28331: BIP324 integration

bb75a29

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> excludes: - changes to `src/rpc/util.cpp`

kwvg added a commit to kwvg/dash that referenced this pull request Oct 15, 2024

partial bitcoin#28331: BIP324 integration

a4dcbee

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> excludes: - changes to `src/rpc/util.cpp`

kwvg added a commit to kwvg/dash that referenced this pull request Oct 15, 2024

partial bitcoin#28331: BIP324 integration

ff92d1a

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> excludes: - changes to `src/rpc/util.cpp`

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

Loading

DrahtBot commented Aug 24, 2023 •

edited

Loading

Sjors commented Aug 25, 2023 •

edited

Loading

sipa commented Aug 29, 2023

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

Loading

kashifs Nov 10, 2023

sipa Nov 10, 2023

fanquake Nov 10, 2023

theStack Nov 10, 2023

kashifs Nov 10, 2023 •

edited

Loading

fanquake commented Dec 7, 2023

BIP324 integration #28331

BIP324 integration #28331

Conversation

sipa commented Aug 24, 2023 • edited by fanquake Loading

DrahtBot commented Aug 24, 2023 • edited Loading

Reviews

Conflicts

Sjors commented Aug 25, 2023 • edited Loading

sipa commented Aug 29, 2023

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 Loading

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 Loading

Choose a reason for hiding this comment

fanquake commented Dec 7, 2023

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