Page MenuHomePhabricator

Call FinalizeBlockAndInvalidate without cs_main held
ClosedPublic

Authored by fpelliccioni on Jan 14 2020, 15:52.

Details

Summary

Now InvalidateBlock needs cs_main to be unlocked.
Related to the latest comments on D4758.

Depends on D4802

Test Plan
  1. Build with Clang in Debug mode:
CXX=clang++ CC=clang cmake .. -D CMAKE_CXX_FLAGS="-Werror=thread-safety-analysis" -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check-all
  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.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Snippet of first build failure:

[15:52:59] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git config core.sparseCheckout true
[15:52:59] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git config http.sslCAInfo
[15:52:59] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git show-ref
[15:52:59] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git show-ref refs/tags/phabricator/diff/15450
[15:52:59] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git log -n1 --pretty=format:%H%x20%s 6d961c4702d84c51ee54e460249ecf61c0171a15 --
[15:52:59] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git -c credential.helper= checkout -q -f phabricator/diff/15450
[15:52:59] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git show-ref refs/tags/phabricator/diff/15450
[15:52:59] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] Cleaning Bitcoin ABC Staging in /home/teamcity/buildAgent/work/c4a5708f2bae7929 the file set ALL_UNTRACKED
[15:52:59] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git clean -f -d -x
[15:52:59] : Build preparation done
[15:52:59]E: Step 1/1: Command Line
[15:52:59] :	 [Step 1/1] Ant JUnit report watcher
[15:52:59] :		 [Ant JUnit report watcher] Watching paths:
[15:52:59] :		 [Ant JUnit report watcher] +:build/test_bitcoin.xml
[15:52:59] :		 [Ant JUnit report watcher] +:test/functional/junit_results.xml
[15:52:59] :		 [Ant JUnit report watcher] +:build/junit_results*.xml
[15:52:59] :	 [Step 1/1] Starting: /home/teamcity/buildAgent/temp/agentTmp/custom_script6736073977563878316
[15:52:59] :	 [Step 1/1] in directory: /home/teamcity/buildAgent/work/c4a5708f2bae7929
[15:52:59]W:	 [Step 1/1] + : build-diff
[15:52:59]W:	 [Step 1/1] + '[' -z build-diff ']'
[15:52:59] :	 [Step 1/1] Running build configuration 'build-diff'...
[15:52:59]W:	 [Step 1/1] + echo 'Running build configuration '\''build-diff'\''...'
[15:52:59]W:	 [Step 1/1] ++ git rev-parse --show-toplevel
[15:52:59]W:	 [Step 1/1] + TOPLEVEL=/home/teamcity/buildAgent/work/c4a5708f2bae7929
[15:52:59]W:	 [Step 1/1] + export TOPLEVEL
[15:52:59]W:	 [Step 1/1] + trap print_sanitizers_log ERR
[15:52:59]W:	 [Step 1/1] +++ dirname ./contrib/teamcity/build-configurations.sh
[15:52:59]W:	 [Step 1/1] ++ cd ./contrib/teamcity
[15:52:59]W:	 [Step 1/1] ++ pwd
[15:52:59]W:	 [Step 1/1] + CI_SCRIPTS_DIR=/home/teamcity/buildAgent/work/c4a5708f2bae7929/contrib/teamcity
[15:52:59]W:	 [Step 1/1] + setup
[15:52:59]W:	 [Step 1/1] + : /home/teamcity/buildAgent/work/c4a5708f2bae7929/build
[15:52:59]W:	 [Step 1/1] + mkdir -p /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/output
[15:52:59]W:	 [Step 1/1] ++ cd /home/teamcity/buildAgent/work/c4a5708f2bae7929/build
[15:52:59]W:	 [Step 1/1] ++ pwd
[15:52:59]W:	 [Step 1/1] + BUILD_DIR=/home/teamcity/buildAgent/work/c4a5708f2bae7929/build
[15:52:59]W:	 [Step 1/1] + export BUILD_DIR
[15:52:59]W:	 [Step 1/1] + TEST_RUNNER_FLAGS=--tmpdirprefix=output
[15:52:59]W:	 [Step 1/1] + cd /home/teamcity/buildAgent/work/c4a5708f2bae7929/build
[15:52:59]W:	 [Step 1/1] ++ nproc
[15:52:59]W:	 [Step 1/1] + THREADS=12
[15:52:59]W:	 [Step 1/1] + export THREADS
[15:52:59]W:	 [Step 1/1] + SAN_SUPP_DIR=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions
[15:52:59]W:	 [Step 1/1] + SAN_LOG_DIR=/tmp/sanitizer_logs
[15:52:59]W:	 [Step 1/1] + mkdir -p /tmp/sanitizer_logs
[15:52:59]W:	 [Step 1/1] + rm -rf '/tmp/sanitizer_logs/*'
[15:52:59] :	 [Step 1/1] Error: Invalid build name 'build-diff'
[15:52:59]W:	 [Step 1/1] + export ASAN_OPTIONS=malloc_context_size=0:log_path=/tmp/sanitizer_logs/asan.log
[15:52:59]W:	 [Step 1/1] + ASAN_OPTIONS=malloc_context_size=0:log_path=/tmp/sanitizer_logs/asan.log
[15:52:59]W:	 [Step 1/1] + export LSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/lsan:log_path=/tmp/sanitizer_logs/lsan.log
[15:52:59]W:	 [Step 1/1] + LSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/lsan:log_path=/tmp/sanitizer_logs/lsan.log
[15:52:59]W:	 [Step 1/1] + export TSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/tsan:log_path=/tmp/sanitizer_logs/tsan.log
[15:52:59]W:	 [Step 1/1] + TSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/tsan:log_path=/tmp/sanitizer_logs/tsan.log
[15:52:59]W:	 [Step 1/1] + export UBSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:log_path=/tmp/sanitizer_logs/ubsan.log
[15:52:59]W:	 [Step 1/1] + UBSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:log_path=/tmp/sanitizer_logs/ubsan.log
[15:52:59]W:	 [Step 1/1] + case "$ABC_BUILD_NAME" in
[15:52:59]W:	 [Step 1/1] + echo 'Error: Invalid build name '\''build-diff'\'''
[15:52:59]W:	 [Step 1/1] + exit 2
[15:52:59]W:	 [Step 1/1] Process exited with code 2
[15:53:00]E:	 [Step 1/1] Process exited with code 2 (Step: Command Line)
jasonbcox requested changes to this revision.Jan 14 2020, 17:43
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/validation.cpp
3141 ↗(On Diff #15453)

Does cs_main really need to be held past this point?

This revision now requires changes to proceed.Jan 14 2020, 17:43
src/validation.cpp
3141 ↗(On Diff #15453)

I ran the tests by putting the "unlock" just below that line and they were Ok.
So it seems that it is not necessary to held the lock beyond that line, unless some Thread-Safet-Attributes are missing.

Sanitizer tests look ok to me.

This revision is now accepted and ready to land.Jan 14 2020, 20:48
src/validation.cpp
3147 ↗(On Diff #15462)

I believe these accesses to chainActive do actually need a lock -- something else might call chainActive.setTip in another thread which would be a data race.