Fix: Dandelion mutex LOCK usages #84
Conversation
|
Will fire this up now with |
| { | ||
| AssertLockHeld(cs_main); | ||
| if (m_mempool) AssertLockHeld(m_mempool->cs); | ||
| if (m_stempool) AssertLockHeld(m_stempool->cs); |
There was a problem hiding this comment.
not sure if this is needed, 'MaybeUpdateMempoolForReorg' contains an assert for stempool already
There was a problem hiding this comment.
Yeah agree with @barrystyle here, seems redundant.
There was a problem hiding this comment.
Are you able to remove the redundant asserts?
There was a problem hiding this comment.
I see where you are coming from. The signature of MaybeUpdateMempoolForReorgrequires callers to assert that locks are held already. Super useful for static analysis.
This is also the reason the Bitcoin Developers added a check for the mempool mutex in the first place (L2647) although it is checked again in MaybeUpdateMempoolForReorg.
|
Was any of this backed with 'deadlocked thread' output from debug? |
JaredTate
left a comment
There was a problem hiding this comment.
ACK. Client compiles, runs & sends tx's fine.
Side note: Still seeing the "Blockpolicy error mempool tx *** already being tracked" quite often.
gto90
left a comment
There was a problem hiding this comment.
cACK. I would like to get @barrystyle 's questions answered before we merge.
|
cACK for now. |
This is seriously an issue we have to fix. @barrystyle Maybe you can take a look into this? |
|
Would love to merge this if no one is opposed to that |
This PR fixes the
stempoolmutex usage inActivateBestChain().In some places the mutex wasn't acquired which could lead to race conditions, so it was added where needed.
And in some locations the mutex wasn't required where
EXCLUSIVE_LOCKS_REQUIREDas modified accordingly.