Fix: Dandelion mutex LOCK usages #84
Conversation
SmartArray
requested review from
digibyte,
JaredTate,
digicontributer and
gto90
SmartArray
changed the title
SmartArray
changed the title
|
Will fire this up now with |
| @@ -2645,6 +2645,7 @@ bool CChainState::ActivateBestChainStep(BlockValidationState& state, CBlockIndex | |||
| { | |||
| 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? |
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.