Page MenuHomePhabricator

refactor: Avoid locking tx pool cs thrice
ClosedPublic

Authored by fpelliccioni on Oct 1 2019, 13:32.

Details

Summary

addUnchecked is (outside the tests) only called by ATMP, which already takes the tx pool read lock. So locking it twice more in both addUnchecked methods seems redundant.

Backport of Bitcoin Core PR13786
https://github.com/bitcoin/bitcoin/pull/13786

Test Plan
  1. Build with Clang in Debug mode:
CXX=clang++ CC=clang cmake .. -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check
  1. Verify that the compiler has not emitted a thread-safety warning.
  2. Run the node: ./src/bitcoind -regtest
  3. Verify that text similar to "Assertion failed: lock ... not held ..." is not printed on stderr.

Update:
To mitigate the risk of ignoring the search for warnings I suggest executing the subsequent test plans as follows:

CXX=clang++ CC=clang cmake .. -D CMAKE_CXX_FLAGS="-Werror=thread-safety-analysis" -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
feature-backport-84d5a6210
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7676
Build 13391: Bitcoin ABC Buildbot (legacy)
Build 13390: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Oct 1 2019, 14:11

Build with Clang to check for -Wthread-safety-warnings.

This revision now requires changes to proceed.Oct 1 2019, 14:11
This revision is now accepted and ready to land.Oct 3 2019, 13:26
fpelliccioni edited the test plan for this revision. (Show Details)
fpelliccioni edited the test plan for this revision. (Show Details)