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#22817, #23042, #22777, #23774, #25443, #26138, #26854, #27128, #27761, #27863, #28287, #30118, partial bitcoin#22778 (auxiliary backports: part 16) #6276

Merged
merged 14 commits into from
Oct 4, 2024

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Sep 17, 2024

Additional Information

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)

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.

should you just skip 25443 because

test: soften connect_nodes assertions
....
Effectively reverts https://github.com/bitcoin/bitcoin/pull/25443

?

@kwvg
Copy link
Collaborator Author

kwvg commented Sep 17, 2024

There are changes introduced to connect_nodes() after bitcoin#25443 and for ease of review, bitcoin#25443 was retained to keep the diffs intact.

Making a separate "effective revert" commit after connect_nodes() changes have been made, allows for one commit to be reverted when a potential future PR resolves the underlying cause instead of modifying every commit that changes connect_nodes() to account for the absence of bitcoin#25443.

@knst
Copy link
Collaborator

knst commented Sep 17, 2024

we don't have signet as well:

# Setup the three signets, which are incompatible with each other

allows for one commit to be reverted when a potential future PR

Do 20445 "partial" at least... No one going to remember, that we need to revert here something, maybe add TODO here?

@kwvg
Copy link
Collaborator Author

kwvg commented Sep 17, 2024

we don't have signet as well

I've kept the signet comments in as it's unclear if signet will be implemented soon. If it will be soon, I think it's fine to keep them in. If it will be in the foreseeable future, then I can remove them in a separate commit so it is easier to revert when signet is implemented. If it won't be, then I can remove them from the commit and also remove the signet references I kept around in prior backports.

Do 20445 "partial" at least... No one going to remember, that we need to revert here something, maybe add TODO here?

Can do that. Will wait for GitLab CI result and will mark as partial and add TODO to connect_blocks().

@kwvg kwvg changed the title backport: merge bitcoin#22817, #25443, #26519, #26854, #25880, #26982, #26584, #27577, #28287, #28419 (auxiliary backports: part 16) backport: merge bitcoin#22741, #22817, #25443, #26138, #26854, #27128, #28287, partial bitcoin#22550 (auxiliary backports: part 16) Sep 22, 2024
@kwvg kwvg added this to the 21.2 milestone Sep 22, 2024
@kwvg kwvg requested a review from UdjinM6 September 22, 2024 18:40
Copy link

This pull request has conflicts, please rebase.

Copy link

This pull request has conflicts, please rebase.

@UdjinM6
Copy link

UdjinM6 commented Sep 25, 2024

let's extract fd82bdc and 927dd6f into their own PR maybe?

@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Sep 25, 2024
PastaPastaPasta added a commit that referenced this pull request Sep 25, 2024
…t-only)

1e17b74 test: no longer connect nodes in parallel in `start_masternodes` (UdjinM6)
be72ef5 test: use `setmnthreadactive` to get controlable `connect_nodes` behaviour (UdjinM6)
e2ed82a feat(rpc): introduce `setmnthreadactive` (regtest-only) (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  This adds a new rpc command to enable/disable automatic masternode connections creation. We need this for #6276. 1e17b74 is extracted from ede1833 to avoid multiple jobs calling `setmnthreadactive` on the same node in parallel.

  ## What was done?
  Add `setmnthreadactive` rpc and use it

  ## How Has This Been Tested?
  run tests

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  kwvg:
    LGTM, ACK 1e17b74
  PastaPastaPasta:
    utACK 1e17b74

Tree-SHA512: 83c1c07d0066e26202fd21942a09e41c3560c4d32229b44390946c4acb22319b32aa61a13b9106d20fc8cc197dd2a8ab5fdfcfdeaf3da76af062fc0fd7646972
knst added a commit that referenced this pull request Sep 26, 2024
…e.py`

a656d2f feat: more logging (UdjinM6)
cedd3d5 refactor: make expected_connections optional (UdjinM6)
fd2fbe0 fix: check mn state after each mined quorum (UdjinM6)
cce87a6 fix: should have at least 2 connections when testing isolate_mn (UdjinM6)
793f4b7 fix: connect repaired mns only (UdjinM6)
8597acd fix: remember mns that don't listen and avoid them (UdjinM6)
2069625 fix: calculate expected_complaints correctly (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  Fix some general mistakes and also `connect_nodes` related issues discovered while debugging #6276. Add some logging to make debugging a bit easier.

  ~NOTE: builds on top of #6278 to avoid conflicts, will rebase~ done

  ## What was done?
  pls see individual commits

  ## How Has This Been Tested?
  run tests

  ## Breaking Changes
  n/a

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] 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)_

ACKs for top commit:
  kwvg:
    LGTM, ACK a656d2f
  knst:
    ACK a656d2f
  PastaPastaPasta:
    utACK a656d2f

Tree-SHA512: 30f657218ce0338f9a5a09d9a839cca9c4605740265d2182a1e143ec6ece739fecf748f7b60ccec065c17d9f6d893c0c47893be05c44bb8d34624fb4bf3c2a58
@kwvg kwvg changed the title backport: merge bitcoin#22741, #22817, #25443, #26138, #26854, #27128, #28287, partial bitcoin#22550 (auxiliary backports: part 16) backport: merge bitcoin#22817, #25443, #26138, #26854, #27128, #28287 (auxiliary backports: part 16) Sep 26, 2024
@kwvg kwvg changed the title backport: merge bitcoin#22817, #25443, #26138, #26854, #27128, #28287 (auxiliary backports: part 16) backport: merge bitcoin#22817, #23042, #22777, #25443, #26138, #26854, #27128, #27761, #27863, #28287, partial bitcoin#22778 (auxiliary backports: part 16) Sep 30, 2024
PastaPastaPasta added a commit that referenced this pull request Oct 1, 2024
…at are invalid or likely to fail

40f2ab9 test: don't attempt to reconnect already connected nodes (Kittywhiskers Van Gogh)
4a0fc8b test: don't attempt to (dis)connect nodes to/from themselves (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #6276

  ## 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:
  UdjinM6:
    utACK 40f2ab9

Tree-SHA512: aaaeedabeb6b8ef77187fc14db1888c39863daf66afda93b8c8bc1dbbdf3ff6734445fd296d5b1034da6104e2d7cfcacf26b97b7be0a697b7a99f3671b6cb9a2
Copy link

github-actions bot commented Oct 1, 2024

This pull request has conflicts, please rebase.

Due to stricter checks, we can no longer start masternodes in parallel,
as entities used to process `to_connection` checks are reused before the
previous check is completed, resulting in an exception. Since we're
now validating the establishment of a two-way connection, we have to do
it one at a time.
@kwvg kwvg changed the title backport: merge bitcoin#22817, #23042, #22777, #25443, #26138, #26854, #27128, #27761, #27863, #28287, partial bitcoin#22778 (auxiliary backports: part 16) backport: merge bitcoin#22817, #23042, #22777, #23774, #25443, #26138, #26854, #27128, #27761, #27863, #28287, partial bitcoin#22778 (auxiliary backports: part 16) Oct 2, 2024
@kwvg kwvg marked this pull request as ready for review October 2, 2024 11:23
@kwvg kwvg requested review from knst and PastaPastaPasta October 2, 2024 11:23
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.

p2p_node_network_limited.py fails. Backporting 30118 (2c825b4) should help.

@kwvg kwvg changed the title backport: merge bitcoin#22817, #23042, #22777, #23774, #25443, #26138, #26854, #27128, #27761, #27863, #28287, partial bitcoin#22778 (auxiliary backports: part 16) backport: merge bitcoin#22817, #23042, #22777, #23774, #25443, #26138, #26854, #27128, #27761, #27863, #28287, #30118, partial bitcoin#22778 (auxiliary backports: part 16) Oct 3, 2024
@kwvg kwvg requested a review from UdjinM6 October 3, 2024 21:41
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 e458adb

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 e458adb

@PastaPastaPasta PastaPastaPasta merged commit 8cef87d into dashpay:develop Oct 4, 2024
35 of 36 checks passed
PastaPastaPasta added a commit that referenced this pull request Oct 5, 2024
…ollow-up dash#6276)

9b0c506 test: call `self.generate()` in `p2p_net_deadlock.py` (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  `develop` is currently borked ([build](https://gitlab.com/dashpay/dash/-/jobs/7999893074#L266)) because [dash#6288](#6288) changed the expected syntax for `generate*` calls and [dash#6276](#6276) introduced a new test (`p2p_net_deadlock.py`) that was based on a version of `develop` that used the older syntax.

  No conflicts were reported because it was a new file introduced in the latter PR that the former PR did not have knowledge of.

  ## 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:
    utACK 9b0c506
  PastaPastaPasta:
    utACK 9b0c506

Tree-SHA512: 3b49f0ff4fc5beea8bd48019c581577ebb47cf50c5526e60367740e5deea7fc92451a480abd49b2b3b2a7be7fb9ca2a4aca99c565ef7aaee1ba8cce43e0be776
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
@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants