Page MenuHomePhabricator
Differential D12373

[avalanche] Add compile and runtime assertions that cs_main is not held for some avalanche Processor functions
ClosedPublic
Actions

Authored by sdulfari on Oct 26 2022, 16:54.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC1cfb3559c159: [avalanche] Add compile and runtime assertions that cs_main is not held for…
Summary

Since PeerManager takes a lock on cs_main in order to verify proofs, there is a potential deadlock if cs_main
is taken (commonly done in validation code) and then cs_peerManager is taken (commonly done via Processor).

PeerManager takes a lock on cs_main in order to verify proofs, making the typical lock order:
cs_peerManager (processor.cpp) -> cs_main. As avalanche post consensus develops, its easy to accidentally
introduce an inverted lock order by accessing the avalanche API in validation code:
cs_main (validation.cpp) -> cs_peerManager (processor.cpp).

This patch introduces additional low-hanging-fruit compile and runtime assertions to help catch these as quickly as possible.

Test Plan
ninja check check-functional

Diff Detail

Event Timeline

sdulfari created this revision.Oct 26 2022, 16:54
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 26 2022, 16:54
sdulfari requested review of this revision.Oct 26 2022, 16:54
Harbormaster completed remote builds in B20723: Diff 36012.Oct 26 2022, 16:57
teamcity added a comment.Oct 26 2022, 16:57
Build Bitcoin ABC Diffs / Diff Testing (build-clang) failed.

Tail of the build log:

[352/524] Linking C executable src/secp256k1/verify-bench
[353/524] Linking C executable src/secp256k1/sign-bench
[354/524] Installing component secp256k1
-- Install configuration: "RelWithDebInfo"
-- Install component: "secp256k1"
-- Installing: /results/artifacts/lib/libsecp256k1.a
-- Installing: /results/artifacts/include/secp256k1.h
-- Installing: /results/artifacts/include/secp256k1_preallocated.h
-- Installing: /results/artifacts/include/secp256k1_recovery.h
-- Installing: /results/artifacts/include/secp256k1_schnorr.h
[355/524] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[356/524] Building CXX object src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o
[357/524] Linking CXX executable src/bitcoin-cli
[358/524] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[359/524] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[360/524] Linking C executable src/secp256k1/ecmult-bench
[361/524] Linking C executable src/secp256k1/internal-bench
[362/524] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[363/524] Building CXX object src/CMakeFiles/server.dir/torcontrol.cpp.o
[364/524] Building CXX object src/CMakeFiles/server.dir/txmempool.cpp.o
[365/524] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[366/524] Building CXX object src/CMakeFiles/server.dir/wallet/init.cpp.o
[367/524] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[368/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[369/524] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[370/524] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[371/524] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[372/524] Linking CXX executable src/bitcoin-tx
[373/524] Building CXX object src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o
[374/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[375/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[376/524] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[377/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[378/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[379/524] Building CXX object src/CMakeFiles/server.dir/rpc/blockchain.cpp.o
[380/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[381/524] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[382/524] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[383/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[384/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[385/524] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[386/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[387/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[388/524] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[389/524] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[390/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[391/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[392/524] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[393/524] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[394/524] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[395/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[396/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[397/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[398/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[399/524] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[400/524] Linking CXX static library src/wallet/libwallet.a
[401/524] Linking CXX static library src/wallet/libwallet-tool.a
[402/524] Linking CXX executable src/bitcoin-wallet
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1
sdulfari planned changes to this revision.Oct 26 2022, 17:27
sdulfari updated this revision to Diff 36036.Oct 27 2022, 16:30

Use LOCKS_EXCLUDED instead of EXCLUSIVE_LOCKS_REQUIRED because cs_main locks are taken in too many places to exclude cs_main from everywhere it should not be taken. For more info see https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative

Harbormaster completed remote builds in B20743: Diff 36036.Oct 27 2022, 17:01
Fabien added a subscriber: Fabien.Oct 27 2022, 19:47
Fabien added inline comments.
src/net_processing.cpp
4999 ↗(On Diff #36036)

Is that change intentional ? I don't see what it does

sdulfari added inline comments.Oct 27 2022, 20:25
src/net_processing.cpp
4999 ↗(On Diff #36036)

It moves the call before LOCK(cs_main). The thread safety analysis isn't smart enough to know that cs_main is unlocked after that section goes out of scope.

Fabien requested changes to this revision.Oct 28 2022, 08:32
Fabien added inline comments.
src/net_processing.cpp
4999 ↗(On Diff #36036)

Please double check, this is very dubious and would mean that the whole feature is broken. Also I can't reproduce.

This revision now requires changes to proceed.Oct 28 2022, 08:32
sdulfari updated this revision to Diff 36067.Oct 28 2022, 15:52

Revert moving quorum check in net code. I'm not able to reproduce this for some reason.

Harbormaster completed remote builds in B20759: Diff 36067.Oct 28 2022, 16:08
Fabien accepted this revision.Oct 29 2022, 09:57
This revision is now accepted and ready to land.Oct 29 2022, 09:57
Closed by commit rABC1cfb3559c159: [avalanche] Add compile and runtime assertions that cs_main is not held for… (authored by sdulfari). · Explain WhyOct 31 2022, 16:21
This revision was automatically updated to reflect the committed changes.
sdulfari added a commit: rABC1cfb3559c159: [avalanche] Add compile and runtime assertions that cs_main is not held for….

Revision Contents
Changeset List

PathSize
src/
avalanche/
7 lines
6 lines

Diff 36012

View Options

src/avalanche/processor.h

Loading...
View Options

src/avalanche/processor.cpp

Loading...