Page MenuHomePhabricator

[Avalanche] Gather INVs before entering critical section for cs_vNodes
ClosedPublic

Authored by Fabien on Apr 9 2020, 23:03.

Details

Summary

This fixes a lock order issue where ForNode() takes a lock on cs_vNodes
and locking cs_main to gather INVs within the callback trips deadlock detection,
since net_processing takes those same locks in the reverse order in another thread.

This does not suffer from the same flaw as D5692 in that it doesn't hold
an excessively long lock on cs_main. However it comes with a drawback of
gathering INVs before knowing if the peer node is still connected. I attempted
to mitigate this by making sure INVs are only gathered once. That way if one node
drops connection and another node is suitable, we don't need to take locks on cs_main twice.

Test Plan
cmake -GNinja -DCMAKE_BUILD_TYPE=Debug ..
for i in {1..20}; do ./test/functional/test_runner.py abc-p2p-ava* ; done

There's still some rarer flaky behavior in this test, but none of the failures should
indicate a deadlock in debug.log.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D5693
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 10165
Build 18156: Default Diff Build & Tests
Build 18155: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Apr 10 2020, 02:30
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche.cpp
568 ↗(On Diff #18718)

This is very inelegant.

This revision now requires changes to proceed.Apr 10 2020, 02:30
Fabien added a reviewer: jasonbcox.

Move the test to the loop condition.

This revision is now accepted and ready to land.Apr 10 2020, 15:48