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

Fix potential deadlock #506

Merged
merged 1 commit into from Jun 10, 2019
Merged

Fix potential deadlock #506

merged 1 commit into from Jun 10, 2019

Conversation

aguycalled
Copy link
Member

This PR fixes a potential deadlock moving IsColdStakingEnabled() out of CheckTransaction().

2019-06-04 20:36:10  (1) cs_main  wallet/wallet.cpp:1965
2019-06-04 20:36:10  (2) cs_wallet  wallet/wallet.cpp:1965
2019-06-04 20:36:10 Current lock order is:
2019-06-04 20:36:10  (2) pwallet->cs_wallet  wallet/walletdb.cpp:633
2019-06-04 20:36:10  (1) cs_main  ./main.cpp:4589

@mxaddict
Copy link
Contributor

mxaddict commented Jun 5, 2019

utACK

@red010b37 red010b37 self-requested a review June 5, 2019 10:04
@proletesseract
Copy link
Member

compiling and testing now.

Copy link
Member

@proletesseract proletesseract left a comment

Choose a reason for hiding this comment

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

Compiles and runs on ubuntu 18.04. Test suite runs. Not entirely sure exactly how to test this specific change. Code looks fine. Can you perhaps just explain in a bit more detail what was wrong and what you did to fix it? If I understand correctly, having the cold staking check where it was before could cause a conflict in how the thread locking occurs resulting in this algorithm being unable to proceed?

@aguycalled
Copy link
Member Author

IsColdStakingEnabled() locked cs_main while CheckTransaction() locks cs_wallet, so every call to CheckTransaction() was locking in the order cs_wallet,cs_main conflicting with the lock order from wallet/wallet.cpp:1965 which runs in parallel in other thread

Copy link
Contributor

@mxaddict mxaddict left a comment

Choose a reason for hiding this comment

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

Tested Ubuntu 18.04 ran node for 26 hours

@mxaddict mxaddict merged commit de6e167 into navcoin:master Jun 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants