Avoid lock order inversion in `Chainstate::ConnectTip` function #27684

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:230516-punish changing 2 files +7 −15

hebasto commented at 10:16 AM on May 17, 2023: member
Due to the synchronous call of CValidationInterface::BlockChecked a lock order inversion happens:
```
PeerManagerImpl::m_peer_mutex
  |
  V
Peer::TxRelay::m_tx_inventory_mutex
  |
  V
CTxMemPool::cs
  |
  V
PeerManagerImpl::m_peer_mutex
```
This PR breaks the last link.

The other possible solution is to move CValidationInterface::BlockChecked to a background thread (see #18963). But this PR is much simpler.

I cannot see any drawbacks of calling MaybePunishNodeForBlock asynchronously. Other opinions are welcome.
refactor: Make `PeerManagerImpl::MaybePunishNodeForBlock` return `void` 58a45ed2cb
Call `MaybePunishNodeForBlock` asynchronously 27aaf6a366
test: Drop `deadlock:Chainstate::ConnectTip` TSan suppression 10c620c233
DrahtBot commented at 10:16 AM on May 17, 2023: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.
DrahtBot renamed this:
~~Avoid lock order inversion in `Chainstate::ConnectTip` function~~
Avoid lock order inversion in `Chainstate::ConnectTip` function
on May 17, 2023
in src/net_processing.cpp:1981 in 10c620c233
```
1977 | @@ -1981,7 +1978,7 @@ void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationSta
1978 |      if (state.IsInvalid() &&
1979 |          it != mapBlockSource.end() &&
1980 |          State(it->second.first)) {
1981 | -            MaybePunishNodeForBlock(/*nodeid=*/ it->second.first, state, /*via_compact_block=*/ !it->second.second);
1982 | +            (void)std::async(std::launch::async, &PeerManagerImpl::MaybePunishNodeForBlock, this, /*nodeid=*/it->second.first, state, /*via_compact_block=*/!it->second.second, "");
```
maflcko commented at 10:27 AM on May 17, 2023:

Can you explain why this is fine in the context of logical races, as well as performance-wise in the context of new thread(pools) getting spawned?

hebasto commented at 10:29 AM on May 17, 2023:

Is MaybePunishNodeForBlock performance critical?

maflcko commented at 10:40 AM on May 17, 2023:

Bitcoin Core is performance critical and randomly (unspecified) spawning thread(pool)s may kill performance, cause OOM, etc ...

hebasto commented at 10:55 AM on May 17, 2023:

Then #18963 should be a better approach.
hebasto marked this as a draft on May 26, 2023
DrahtBot commented at 9:26 AM on August 8, 2023: contributor



There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
hebasto closed this on Aug 8, 2023
maflcko commented at 4:14 PM on August 8, 2023: member

Maybe m_tx_inventory_mutex can be released before pool.cs to break that link instead?
bitcoin locked this on Aug 7, 2024

Contributors

hebasto

DrahtBot

maflcko

Linked (view graph)

#18963 [WIP] rebase: Call ProcessNewBlock() asynchronously #19303 Replace all of the RecursiveMutex instances with the Mutex ones

Avoid lock order inversion in `Chainstate::ConnectTip` function #27684

Reviews