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

kwvg · 2024-10-09T16:11:45Z

Additional Information

Dependent on backport: merge bitcoin#26519, #25880, #26982, #26584, #27577, #28419, #28196, #28433, #28489, #28525 (BIP324 backports) #6280
Dependent on backport: merge bitcoin#22817, #23042, #22777, #23774, #25443, #26138, #26854, #27128, #27761, #27863, #28287, #30118, partial bitcoin#22778 (auxiliary backports: part 16) #6276
Dependency for backport: merge bitcoin#20524, #26553, #27986, #28645, #28632, #28782, #28822, #29006, #29212, merge bitcoin-core/gui#754, partial bitcoin#23443, #26448 (BIP324 backports: part 3) #6329
bitcoin#28452 was backported as the behavior it introduces is required for backports like 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, 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 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

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas (note: N/A)
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

UdjinM6

LGTM in general and seems to be working on testnet, few notes:

28331: missing changes in src/rpc/util.cpp, should be partial
needs release notes I guess (for the addition of the protocol itself and for changes in cli output, cmd-line options and RPCs)
should we do a proto bump?

kwvg · 2024-10-14T22:06:49Z

@UdjinM6

28331: missing changes in src/rpc/util.cpp, should be partial

Resolved in latest push, missing change now noted in commit comment.

needs release notes I guess (for the addition of the protocol itself and for changes in cli output, cmd-line options and RPCs)

We can take reference from upstream v26 (source) release notes but would prefer to do that in a separate PR.

should we do a proto bump?

IMO that isn't necessary, BIP324/P2Pv2 is a change in the transport layer, the protocol layer is more-or-less unaffected and P2Pv2 can gracefully fallback to P2Pv1.

knst · 2024-10-15T08:18:07Z

src/bitcoin-cli.cpp

@@ -517,10 +518,11 @@ class NetinfoRequestHandler : public BaseRequestHandler
                const std::string addr{peer["addr"].get_str()};
                const std::string age{conn_time == 0 ? "" : ToString((m_time_now - conn_time) / 60)};
                const std::string sub_version{peer["subver"].get_str()};
+                const std::string transport{peer["transport_protocol_type"].get_str()};


bitcoin-29058: consider backport bitcoin-29212 to fix empty value if get_str doesn't exist

Good suggestion! Will add in next set of upcoming backports (its emphasis is specifically fleshing out tests, UI and dealing with intermittent failures and edge cases, would fit better there)

knst · 2024-10-15T08:24:03Z

test/functional/p2p_v2_transport.py

+            self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == num_peers + 1)
+            s.sendall(b'\x00' * (MAX_KEY_GARB_AND_GARBTERM_LEN - 1))
+            self.wait_until(lambda: self.nodes[0].getpeerinfo()[-1]["bytesrecv"] == MAX_KEY_GARB_AND_GARBTERM_LEN - 1)
+            with self.nodes[0].assert_debug_log("V2 transport error: missing garbage terminator"):


it doesn't works as expected because assert_debug_log is expecting an array of strings

This is a known bug that has been resolved in bitcoin#28645, which is a part of the next set of BIP324 backports

github-actions · 2024-10-15T14:21:22Z

This pull request has conflicts, please rebase.

Does not include changes to `headerssync.cpp` as it does not exist yet

Co-authored-by: UdjinM6 <[email protected]> excludes: - changes to `src/rpc/util.cpp`

…k magic detection Co-authored-by: UdjinM6 <[email protected]>

…-v2transport Co-authored-by: UdjinM6 <[email protected]>

…, add to -netinfo

…abled

UdjinM6

light ACK 5dd60c4

PastaPastaPasta

utACK 5dd60c4

, bitcoin#28645, bitcoin#28632, bitcoin#28782, bitcoin#28822, bitcoin#29006, bitcoin#29212, merge bitcoin-core/gui#754, partial bitcoin#23443, bitcoin#26448 (BIP324 backports: part 3) aa5311d merge bitcoin#29212: Fix -netinfo backward compat with getpeerinfo pre-v26 (Kittywhiskers Van Gogh) 1a293c7 merge bitcoin#29006: fix v2 transport intermittent test failure (Kittywhiskers Van Gogh) d0804d4 merge bitcoin#28822: Add missing wait for version to be sent in add_outbound_p2p_connection (Kittywhiskers Van Gogh) c0b3062 merge bitcoin#28782: Add missing sync on send_version in peer_connect (Kittywhiskers Van Gogh) 35253cd merge bitcoin#28632: make python p2p not send getaddr on incoming connections (Kittywhiskers Van Gogh) 6a4ca62 merge bitcoin#28645: fix `assert_debug_log` call-site bugs, add type checks (Kittywhiskers Van Gogh) deaee14 merge bitcoin-core/gui#754: Add BIP324-specific labels to peer details (Kittywhiskers Van Gogh) fffe6e7 merge bitcoin#27986: remove race in the user-agent reception check (Kittywhiskers Van Gogh) 1bf135b merge bitcoin#26553: Fix intermittent failure in rpc_net.py (Kittywhiskers Van Gogh) 5bf245b partial bitcoin#26448: fix intermittent failure in p2p_sendtxrcncl.py (Kittywhiskers Van Gogh) b7c0030 partial bitcoin#23443: Erlay support signaling (Kittywhiskers Van Gogh) c709df7 merge bitcoin#20524: Move MIN_VERSION_SUPPORTED to p2p.py (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6324 * Dependency for #6330 ## How Has This Been Tested? * Changes to Qt client were validated by running the client <details> <summary>Screenshot</summary> ![Transport reporting in Qt](https://github.com/user-attachments/assets/0d551e19-f3a2-4ce7-83d6-5cb3d03b1765) </details> * Changes to `dash-cli` were validated by running it with different node versions **Against a node built on this PR** <details> <summary>Screenshot</summary> ![getinfo with node running the latest build](https://github.com/user-attachments/assets/8cda68cc-727a-4cf3-a4d8-dd6a33331d78) </details> **Against a node built running the last release** <details> <summary>Screenshot</summary> ![getinfo with node running the latest release](https://github.com/user-attachments/assets/0c6ff476-7cc9-4297-bae5-35d423aba480) </details> ## 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 (note: N/A) - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: ACK aa5311d PastaPastaPasta: utACK aa5311d Tree-SHA512: 8cca324ac988a73c0590a4e9b318e81ce951ac55fb173cf507fa647cab01ab4981e6a06d4792376b4bfb44ff09d4811de05fadb9ba793dd00b4c7965b4b22654

kwvg added this to the 22 milestone Oct 9, 2024

kwvg requested review from knst, UdjinM6 and PastaPastaPasta October 10, 2024 06:17

kwvg marked this pull request as ready for review October 10, 2024 06:17

UdjinM6 reviewed Oct 10, 2024

View reviewed changes

UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Oct 10, 2024

kwvg force-pushed the bip324_bps2 branch from 3caaa7a to f8b3594 Compare October 14, 2024 22:04

kwvg requested a review from UdjinM6 October 14, 2024 22:06

knst reviewed Oct 15, 2024

View reviewed changes

kwvg mentioned this pull request Oct 15, 2024

backport: merge bitcoin#20524, #26553, #27986, #28645, #28632, #28782, #28822, #29006, #29212, merge bitcoin-core/gui#754, partial bitcoin#23443, #26448 (BIP324 backports: part 3) #6329

Merged

5 tasks

github-actions bot added the needs rebase label Oct 15, 2024

kwvg and others added 11 commits October 15, 2024 15:33

partial bitcoin#28452: Do not use std::vector = {} to release memory

d39d8a4

Does not include changes to `headerssync.cpp` as it does not exist yet

fix: drop CConnman::mapNodesWithDataToSend, use transport data

f9f8805

partial bitcoin#28331: BIP324 integration

ff92d1a

Co-authored-by: UdjinM6 <[email protected]> excludes: - changes to `src/rpc/util.cpp`

merge bitcoin#28577: raise V1_PREFIX_LEN from 12 to 16

6074974

merge bitcoin#28588: add checks for v1 prefix matching / wrong networ…

65eb194

…k magic detection Co-authored-by: UdjinM6 <[email protected]>

merge bitcoin#28634: add check for missing garbage terminator detection

400c9dd

merge bitcoin#28849: fix node index bug when comparing peerinfo

9371e2e

merge bitcoin#28805: Make existing functional tests compatible with -…

4e96e26

…-v2transport Co-authored-by: UdjinM6 <[email protected]>

merge bitcoin#28951: damage ciphertext/aad in full byte range

92e862a

merge bitcoin#29058: use v2transport for manual/addrfetch connections…

b2ac426

…, add to -netinfo

merge bitcoin#29239: Make v2transport default for addnode RPC when en…

5dd60c4

…abled

kwvg force-pushed the bip324_bps2 branch from f8b3594 to 5dd60c4 Compare October 15, 2024 15:34

github-actions bot removed the needs rebase label Oct 15, 2024

UdjinM6 approved these changes Oct 16, 2024

View reviewed changes

PastaPastaPasta approved these changes Oct 16, 2024

View reviewed changes

PastaPastaPasta merged commit 5903fb7 into dashpay:develop Oct 16, 2024
34 of 35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kwvg commented Oct 9, 2024 •

edited

Loading

UdjinM6 left a comment

kwvg commented Oct 14, 2024

knst Oct 15, 2024

kwvg Oct 15, 2024

knst Oct 15, 2024

kwvg Oct 15, 2024

github-actions bot commented Oct 15, 2024

UdjinM6 left a comment

PastaPastaPasta left a comment

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

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

Conversation

kwvg commented Oct 9, 2024 • edited Loading

Additional Information

Breaking Changes

Checklist

UdjinM6 left a comment

Choose a reason for hiding this comment

kwvg commented Oct 14, 2024

knst Oct 15, 2024

Choose a reason for hiding this comment

kwvg Oct 15, 2024

Choose a reason for hiding this comment

knst Oct 15, 2024

Choose a reason for hiding this comment

kwvg Oct 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 15, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

kwvg commented Oct 9, 2024 •

edited

Loading