-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 deadlock in CSigSharesManager::SendMessages #2757
Conversation
Why not just set |
@UdjinM6 I thought ToInvStr() was more clear in the use of LLMQ_400_60, which is actually kind of a hack (we're passing the wrong type here...). |
True, it's hack-ish :) But the funny thing is that |
Yeah that's true...that was different in the past but now it's a leftover that's actually not needed. I'll remove ToInv completely now. |
Locking "cs" at this location caused a (potential) deadlock due to changed order of cs and cs_vNodes locking. This changes the method to not require the session object anymore which removes the need for locking.
c75d534
to
2bdb856
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
This allows use of sizes which are not supported in chainparams.
Turned out that the last changes I did caused a crash due to LLMQ_400_60 not known on regtest. Changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
Locking "cs" at this location caused a (potential) deadlock due to changed
order of cs and cs_vNodes locking. This changes the method to not require
the session object anymore which removes the need for locking.