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

Fix: Dandelion mutex LOCK usages #84

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Fix: Dandelion mutex LOCK usages #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

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

not sure if this is needed, 'MaybeUpdateMempoolForReorg' contains an assert for stempool already

Copy link

Choose a reason for hiding this comment

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

Yeah agree with @barrystyle here, seems redundant.

Copy link
Member

Choose a reason for hiding this comment

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

@SmartArray ,

Are you able to remove the redundant asserts?

Copy link
Author

@SmartArray SmartArray Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.


const CBlockIndex* pindexOldTip = m_chain.Tip();
const CBlockIndex* pindexFork = m_chain.FindFork(pindexMostWork);
Expand Down Expand Up @@ -2793,6 +2794,7 @@ bool CChainState::ActivateBestChain(BlockValidationState& state, std::shared_ptr
LOCK(cs_main);
// Lock transaction pool for at least as long as it takes for connectTrace to be consumed
LOCK(MempoolMutex());
LOCK(StempoolMutex());
CBlockIndex* starting_tip = m_chain.Tip();
bool blocks_connected = false;
do {
Expand Down Expand Up @@ -2945,6 +2947,7 @@ bool CChainState::InvalidateBlock(BlockValidationState& state, CBlockIndex* pind
// Lock for as long as disconnectpool is in scope to make sure MaybeUpdateMempoolForReorg is
// called after DisconnectTip without unlocking in between
LOCK(MempoolMutex());
LOCK(StempoolMutex());
if (!m_chain.Contains(pindex)) break;
pindex_was_in_chain = true;
CBlockIndex *invalid_walk_tip = m_chain.Tip();
Expand Down
4 changes: 2 additions & 2 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,7 @@ class CChainState
CCoinsViewCache& view, bool fJustCheck = false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

// Apply the effects of a block disconnection on the UTXO set.
bool DisconnectTip(BlockValidationState& state, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs, m_stempool->cs);
bool DisconnectTip(BlockValidationState& state, DisconnectedBlockTransactions* disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);

// Manual block validity manipulation:
/** Mark a block as precious and reorganize.
Expand Down Expand Up @@ -803,7 +803,7 @@ class CChainState

private:
bool ActivateBestChainStep(BlockValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs, m_stempool->cs);
bool ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs, m_stempool->cs);
bool ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool->cs);

void InvalidBlockFound(CBlockIndex* pindex, const BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
CBlockIndex* FindMostWorkChain() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
Expand Down