Skip to content
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#26519, #25880, #26982, #26584, #27577, #28419, #28196, #28433, #28489, #28525 (BIP324 backports) #6280

Merged
merged 10 commits into from
Sep 24, 2024

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Sep 18, 2024

Additional Information

  • When backporting bitcoin#25880, changes needed to be made to p2p_ibd_stalling.py to help it pass.

    • DIP3 activation had to be delayed to a block beyond the range of the test. This is to prevent block rejection arising from a missing DIP3-compliant coinbase (done with -dip3params=2000:2000)
    • Mock time was disabled to ensure nodes in the test do not resort to direct fetching (with mock time enabled, nodes would be considered close apart in time, which would prevent the primary node from fetching in parallel, which is behavior this test relies on) (done with self.disable_mocktime = True)
    • The nodes connected do not report compressed headers support (the test relies on sending headers messages and reworking it to use compressed headers has little benefit) (done with services = NODE_NETWORK | NODE_BLOOM)
  • When backporting bitcoin#28196, in the v2transport_test unit test, references to 4000000 were substituted with MAX_PROTOCOL_MESSAGE_LENGTH as Dash messages have a protocol limit of 3MiB (source) while Bitcoin messages have a protocol limit of 4MB (source).

    • Additionally note that short message IDs as defined in the BIP324 spec (source) have not been changed to include Dash-specific messages, meaning, Dash-specific messages will always take 13 bytes.
    • As FEEFILTER is not supported by Dash, it has been replaced with a blank string in the short IDs table. It was not removed as doing so would disturb the table's arrangement as specified in spec and require readjustment of tests to account for the change in layout.

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 (note: N/A)
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@kwvg kwvg added this to the 21.2 milestone Sep 18, 2024
@kwvg kwvg marked this pull request as ready for review September 18, 2024 22:46
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 397f34a

@UdjinM6
Copy link

UdjinM6 commented Sep 19, 2024

Additionally note that short message IDs as defined in the BIP324 spec (source) have not been changed to include Dash-specific messages, meaning, Dash-specific messages will always take 13 bytes

message_type ... a one byte ID in range 1..255

only 28 IDs are used in Bitcoin so why don't we use IDs 128+ for Dash-specific messages? Doubt they'll get to these numbers anytime soon. @PastaPastaPasta ?

also pls see da5b56c

@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Sep 20, 2024

only 28 IDs are used in Bitcoin so why don't we use IDs 128+ for Dash-specific messages? Doubt they'll get to these numbers anytime soon. @PastaPastaPasta ?

@UdjinM6
This is actually the goal in the not so distant future, probably before we actually merge in stuff where it's actually usable. If not, we'll do that via a new protocol version. Excluding that for now was just to expedite the process, as the code doesn't have a trivial way to just start at 128 for example if I remember correctly

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 63b13aa

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 63b13aa

NetMsgType::BLOCK,
NetMsgType::BLOCKTXN,
NetMsgType::CMPCTBLOCK,
"", /* FEEFILTER is not implemented in Dash */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"FEEFILTER_NOT_SUPPORTED" is better than empty string here IMO, to avoid mixing with other possible errors

Suggested change
"", /* FEEFILTER is not implemented in Dash */
"FEEFILTER_NOT_SUPPORTED",

@PastaPastaPasta PastaPastaPasta merged commit 3b8f244 into dashpay:develop Sep 24, 2024
35 checks passed
PastaPastaPasta added a commit that referenced this pull request Oct 16, 2024
, 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
PastaPastaPasta added a commit that referenced this pull request Oct 24, 2024
, bitcoin#29372, bitcoin#29460, bitcoin#29358, bitcoin#29511, bitcoin#29390, bitcoin#29431, bitcoin-core/gui#788 (BIP324 backports: part 4)

4735b82 merge bitcoin#29431: disconnection scenarios during v2 handshake (Kittywhiskers Van Gogh)
cc6b88e merge bitcoin-core/gui#788: update session ID tooltip (Kittywhiskers Van Gogh)
2455862 merge bitcoin#29390: speedup bip324_cipher.py unit test (Kittywhiskers Van Gogh)
062aaf1 merge bitcoin#29511: Fix intermittent failure in rpc_net.py --v2transport (Kittywhiskers Van Gogh)
54972e8 merge bitcoin#29358: use v2 everywhere for P2PConnection if --v2transport is enabled (Kittywhiskers Van Gogh)
4cce72f test: add missing debug log assertion in `p2p_invalid_messages.py` (Kittywhiskers Van Gogh)
bd2fe61 merge bitcoin#29460: assert rpc error for addnode v2transport not enabled (Kittywhiskers Van Gogh)
5ee15fa merge bitcoin#29372: fix intermittent failure in `rpc_setban.py --v2transport` (Kittywhiskers Van Gogh)
e278818 merge bitcoin#29352: fix intermittent failure in p2p_v2_earlykeyresponse (Kittywhiskers Van Gogh)
6b2a8b5 merge bitcoin#24748: functional tests for v2 P2P encryption (Kittywhiskers Van Gogh)
32500f2 merge bitcoin#27653: add unit test coverage for Python ECDSA implementation (Kittywhiskers Van Gogh)
9f476c6 net: add Dash network message short IDs, allocate range 128 onwards (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6329

  * Dash-specific P2P messages have been allocated short IDs after 128 based on a prior suggestion ([source](#6280 (comment))) as there are 255 potential short IDs (ID `0` is reserved for V1 fallback, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1019)) and upstream uses 28 short IDs (though Dash has left ID `5` blank as we do not implement the `FEEFILTER` message, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1024)).

    As it is unlikely that upstream will utilize more than 127 short IDs (and the spec refers to IDs after 32 as "undefined", [source](https://github.com/bitcoin/bips/blob/52894e1aa7af27f4ae7836a391643124c71e2639/bip-0324.mediawiki#v2-bitcoin-p2p-message-structure)), there shouldn't be an adverse effect to utilizing IDs >=128. The unified array of short IDs are accessible through `V2ShortIDs()`.

    * As there are checks to see if an ID *can* be valid by checking against the array size (which wouldn't work here as we create an array of 256 entries combining both upstream and Dash's allocated short IDs, filling the rest with blank values and we cannot ignore blank values to know if a value is unallocated as the blanking could also signal a reservation, [source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.cpp#L1048-L1052)), such a check needs to be done by using `IsValidV2ShortID()`.
    * `V2ShortIDs()` isn't as elegant as desired as `std::fill` and `std::copy` are not `constexpr` until C++20 ([source](https://en.cppreference.com/w/cpp/algorithm/fill), [source](https://en.cppreference.com/w/cpp/algorithm/copy)) 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#24748](bitcoin#24748)

  * The oversized payload test in `p2p_invalid_messages.py` will expect an excessively large message of size of `3145729` bytes (and in P2Pv2, `3145742` bytes), as opposed to upstream's `4000001` and `4000014` bytes respectively as Dash has a lower protocol limit of 3MiB ([source](https://github.com/dashpay/dash/blob/a7bbcc823d800b293e9ec54dff518fa9929c763c/src/net.h#L80-L81)) vs Bitcoin's 4MB ([source](https://github.com/bitcoin/bitcoin/blob/225718eda89d65a7041c33e1994d3bf7bd71bbdd/src/net.h#L62-L63))

  ## 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
  - [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:
  PastaPastaPasta:
    utACK 4735b82
  UdjinM6:
    light ACK 4735b82

Tree-SHA512: 4a9d586133633c109e6a8f20a6c6c5e4b24185fb616bcd8568e546b6e9f886e7a8707e811fd070bbe32c40df269f89a56343a24b67242c6147f9df27275af599
@UdjinM6 UdjinM6 removed this from the 21.2 milestone Oct 29, 2024
@UdjinM6 UdjinM6 added this to the 22 milestone Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants