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.

Sign up for GitHub

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

Jump to bottom

backport: merge bitcoin#22817, #25443, #26138, #26854, #27128, #28287 (auxiliary backports: part 16) #6276

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from
Draft

backport: merge bitcoin#22817, #25443, #26138, #26854, #27128, #28287 (auxiliary backports: part 16) #6276

wants to merge 9 commits into from

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Sep 17, 2024 •
edited
Loading

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)

knst
knst reviewed Sep 17, 2024
View reviewed changes
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 •
edited
Loading

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
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please rebase.

This was referenced Sep 25, 2024
Merged
Merged
@github-actions GitHub Actions
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
@PastaPastaPasta
Merge #6286: feat(rpc): introduce and use setmnthreadactive (regtes…
85764c4 
…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
@knst
Merge #6287: test: fixes and improvements for `feature_llmq_simplepos…
67b5d86 
…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 mentioned this pull request Sep 26, 2024
Open
5 tasks
@kwvg
merge bitcoin#22817: Avoid race after connect_nodes
4d8f1fd 
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
merge bitcoin#25443: Fail if connect_nodes fails
bfcecd5
@kwvg
merge bitcoin#26138: Avoid race in disconnect_nodes helper
b49654e
@kwvg
merge bitcoin#26854: Fix intermittent timeout in p2p_permissions.py
c9d7223
@kwvg
merge bitcoin#27128: fix intermittent issue in p2p_disconnect_ban
78e3246
@kwvg
fix: ensure that deadlocks are actually resolved
b48822b
@kwvg
merge bitcoin#28287: add sendmsgtopeer rpc and a test for net-level…
e64d254 
… deadlock situation

`random_bytes()` is introduced in bitcoin#25625 but the function def
alone doesn't warrant a full backport, so we'll only implement the
section relevant to this PR.
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knst knst knst left review comments

@UdjinM6 UdjinM6 Awaiting requested review from UdjinM6

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Milestone
21.2
Development

Successfully merging this pull request may close these issues.
4 participants
@kwvg @knst @UdjinM6 @thephez