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

fix: deadlock over cs_main and contributionsCacheCs in dkssessionmgr #5962

Merged
merged 1 commit into from
Apr 2, 2024
Merged

fix: deadlock over cs_main and contributionsCacheCs in dkssessionmgr #5962

merged 1 commit into from
Apr 2, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Mar 28, 2024 •
edited
Loading

Issue being fixed or feature implemented

It fixes rpc failure: "Work queue depth exceeded"

As I checked on running dashd in deadlock condition:
Thread 78 is a thread that keep cs_main:

#14 0x0000aaaad1f8d604 in BuildSimplifiedMNListDiff () at evo/simplifiedmns.cpp:364

but it is locked by contributionsCacheCs

#8  llmq::CDKGSessionManager::GetVerifiedContributions () at llmq/dkgsessionmgr.cpp:392

On other hand, contributionsCacheCs is blocked by Thread 59

#17 0x0000aaaad1ba1940 in llmq::CDKGSessionManager::GetVerifiedContributions () at llmq/dkgsessionmgr.cpp:393

and it makes circuit lock by waiting cs_main in

#9  ReadBlockFromDisk () at node/blockstorage.cpp:75

See https://github.com/dashpay/dash-issues/issues/69 for more details

Seems introduced there: #3911

What was done?

Deadlock is removed by reducing scope of mutex

How Has This Been Tested?

I reviewed 2 different servers which have status work queue exceeded, both have same deadlock, so, this patch should fix this issue. Once this fix is merged, we can test it on testnet.

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

@knst
fix: deadlock over cs_main and contributionsCacheCs in dkssessionmgr
ded1b5a 
It fixes rpc failure: "Work queue depth exceeded"
@knst knst added this to the 21 milestone Mar 28, 2024
@knst knst requested review from UdjinM6 and PastaPastaPasta March 28, 2024 19:22
@DashCoreAutoGuix
Copy link

Guix Automation has began to build this PR tagged as v21.0.0-devpr5962.ded1b5a3. A new comment will be made when the image is pushed.

UdjinM6
UdjinM6 approved these changes Mar 29, 2024
View reviewed changes
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

@DashCoreAutoGuix
Copy link

Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.0.0-devpr5962.ded1b5a3. The image should be on dockerhub soon.

@knst
Copy link
Collaborator Author

knst commented Mar 30, 2024 •
edited
Loading

@PastaPastaPasta whatever this PR fix "rpc queue" or it won't, there's a fix for known deadlock that I discovered. Even if it is not full fix for "rpc queue issue", that PR is worth to get merged.

Later one being merged it can be backported to 20.1.1 and we will have an extra release with it (without waiting version 21), or will not have an extra release: changes are trivial to backport.

@PastaPastaPasta PastaPastaPasta merged commit fcee7a5 into dashpay:develop Apr 2, 2024
10 of 11 checks passed
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Apr 3, 2024
@PastaPastaPasta
Merge dashpay#5962: fix: deadlock over cs_main and contributionsCache…
fd46c4c 
…Cs in dkssessionmgr

ded1b5a fix: deadlock over cs_main and contributionsCacheCs in dkssessionmgr (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  **It fixes rpc failure: "Work queue depth exceeded"**

  As I checked on running `dashd` in deadlock condition:
  Thread 78 is a thread that keep `cs_main`:
  ```
  #14 0x0000aaaad1f8d604 in BuildSimplifiedMNListDiff () at evo/simplifiedmns.cpp:364
  ```
  but it is locked by `contributionsCacheCs`
  ```
  #8  llmq::CDKGSessionManager::GetVerifiedContributions () at llmq/dkgsessionmgr.cpp:392
  ```

  On other hand, `contributionsCacheCs` is blocked by Thread 59
  ```
  #17 0x0000aaaad1ba1940 in llmq::CDKGSessionManager::GetVerifiedContributions () at llmq/dkgsessionmgr.cpp:393
  ```
  and it makes circuit lock by waiting `cs_main` in
  ```
  #9  ReadBlockFromDisk () at node/blockstorage.cpp:75
  ```

  See dashpay/dash-issues#69 for more details

  Seems introduced there: dashpay#3911

  ## What was done?
  Deadlock is removed by reducing scope of mutex

  ## How Has This Been Tested?
  I reviewed 2 different servers which have status `work queue exceeded`, both have same deadlock, so, this patch should fix this issue. Once this fix is merged, we can test it on testnet.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] 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

Top commit has no ACKs.

Tree-SHA512: 4fe5c03c464ee6934fb927b897f007b65a8995723196edaffdae067edee7067da151130d4c4bac47d3418fdad5c8e130682f42d7ef9c044380a8c8fff78ee008
@UdjinM6 UdjinM6 modified the milestones: 21, 20.1.1 Apr 3, 2024
PastaPastaPasta added a commit that referenced this pull request Apr 3, 2024
@PastaPastaPasta
Merge #5969: chore: backport items to 20.1.x and release v20.1.1
1951298 
b96b202 chore: bump version to 20.1.1 (pasta)
83cac77 docs: add v20.1.1 release notes (pasta)
e58c7c4 Merge #5970: guix: exclude debug symbols for apple from list of hashes due to its non-determinism (pasta)
2bde1dd Merge #5927: fix: check if message can be handled before attempting to deserialize (pasta)
1637fa5 Merge #5968: docs: add v20.0.4 release notes (pasta)
fd46c4c Merge #5962: fix: deadlock over cs_main and contributionsCacheCs in dkssessionmgr (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  Backport to v20.1.x and release v20.1.1

  ## What was done?
  Backports and release

  ## How Has This Been Tested?

  ## Breaking Changes
  None

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] 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 _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: 40574df3349bf1c653a50b4efbf78d1654eb048eeeb483eb657eec5f2af102f95cff8f978a98c174925d2672d56426238a1b7ec3e8cd2a53ec28c0fca42c1293
PastaPastaPasta added a commit that referenced this pull request Apr 9, 2024
@PastaPastaPasta
Merge #5972: chore: merge master 20.1.1 back into develop
ce992f7 
b96b202 chore: bump version to 20.1.1 (pasta)
83cac77 docs: add v20.1.1 release notes (pasta)
e58c7c4 Merge #5970: guix: exclude debug symbols for apple from list of hashes due to its non-determinism (pasta)
2bde1dd Merge #5927: fix: check if message can be handled before attempting to deserialize (pasta)
1637fa5 Merge #5968: docs: add v20.0.4 release notes (pasta)
fd46c4c Merge #5962: fix: deadlock over cs_main and contributionsCacheCs in dkssessionmgr (pasta)

Pull request description:

  Merge master into develop

Top commit has no ACKs.

Tree-SHA512: e65ffefcb7d0e92367ff322cab533f8505f0e9d00ab136bccb1dbf86ad64d6b710b5c9f9943594c544827d2eb69f5d0ba185962a2ac0b16c6b776a94f44cfb42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees
No one assigned
Labels
guix-build
Projects
None yet
Milestone
20.1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@knst @DashCoreAutoGuix @UdjinM6 @PastaPastaPasta