-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
backport: merge bitcoin#27653, #24748, #29352, #29372, #29460, #29358, #29511, #29390, #29431, bitcoin-core/gui#788 (BIP324 backports: part 4) #6330
Conversation
This pull request has conflicts, please rebase. |
, 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
This pull request has conflicts, please rebase. |
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.
Looks good overall, will test + see few notes below
Also, add add `getqrinfo` and `qrinfo` to `allNetMessageTypes[]` and `netMessageTypesViolateBlocksOnly[]`
…port is enabled
changes looks good but needs rebase on latest develop now |
also, I tested it a bit and it seems to be working with no issues on testnet 👍 |
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 4735b82
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.
light ACK 4735b82
Additional Information
Depends on 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
Dash-specific P2P messages have been allocated short IDs after 128 based on a prior suggestion (source) as there are 255 potential short IDs (ID
0
is reserved for V1 fallback, source) and upstream uses 28 short IDs (though Dash has left ID5
blank as we do not implement theFEEFILTER
message, source).As it is unlikely that upstream will utilize more than 127 short IDs (and the spec refers to IDs after 32 as "undefined", source), there shouldn't be an adverse effect to utilizing IDs >=128. The unified array of short IDs are accessible through
V2ShortIDs()
.IsValidV2ShortID()
.V2ShortIDs()
isn't as elegant as desired asstd::fill
andstd::copy
are notconstexpr
until C++20 (source, source) and until we drop C++17 support, we have to be mindful of that.masternode connect
will now attempt to establish a P2Pv2 connection if the node initiating the connection has opted-in using the new argument (v2transport
) and the node was started with P2Pv2 enabled (using the launch argument,-v2transport
).This mirrors changes to behavior to
addconnection
introduced in bitcoin#24748The oversized payload test in
p2p_invalid_messages.py
will expect an excessively large message of size of3145729
bytes (and in P2Pv2,3145742
bytes), as opposed to upstream's4000001
and4000014
bytes respectively as Dash has a lower protocol limit of 3MiB (source) vs Bitcoin's 4MB (source)Breaking Changes
None expected.
Checklist: