Page MenuHomePhabricator

Call FinalizeBlockAndInvalidate without cs_main held
ClosedPublic

Authored by fpelliccioni on Tue, Jan 14, 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fpelliccioni created this revision.Tue, Jan 14, 15:52
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Jan 14, 15:52

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)
fpelliccioni planned changes to this revision.Tue, Jan 14, 15:53
fpelliccioni updated this revision to Diff 15453.Tue, Jan 14, 16:09

rebased from master

jasonbcox requested changes to this revision.Tue, Jan 14, 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.Tue, Jan 14, 17:43
fpelliccioni added inline comments.Tue, Jan 14, 19:49
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.

fpelliccioni updated this revision to Diff 15461.Tue, Jan 14, 20:02

lock frame reduced

jasonbcox accepted this revision.Tue, Jan 14, 20:48

Sanitizer tests look ok to me.

This revision is now accepted and ready to land.Tue, Jan 14, 20:48
markblundeberg added inline comments.Tue, Jan 14, 21:31
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.