Details

Reviewers

deadalnix
markblundeberg
jasonbcox

Group Reviewers

Restricted Project

Commits

rSTAGINGbaef60890b54: Call FinalizeBlockAndInvalidate without cs_main held
rABCbaef60890b54: Call FinalizeBlockAndInvalidate without cs_main held

Summary

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

Depends on D4802

Test Plan

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

Verify that the compiler has not emitted a thread-safety warning.
Run the node: ./src/bitcoind -regtest
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

fpelliccioni created this revision.Jan 14 2020, 15:52

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 14 2020, 15:52

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing failed.

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)

Harbormaster failed remote builds in B8931: Diff 15450!Jan 14 2020, 15:53

fpelliccioni planned changes to this revision.Jan 14 2020, 15:53

rebased from master

Build Bitcoin-ABC / Diffs / Diff Testing started.

fpelliccioni added a reviewer: markblundeberg.Jan 14 2020, 16:10

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8933: Diff 15453.Jan 14 2020, 16:27

fpelliccioni added a child revision: D4803: Prevent callback overruns in InvalidateBlock and RewindBlockIndex.Jan 14 2020, 17:12

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

fpelliccioni added inline comments.Jan 14 2020, 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.

lock frame reduced

Build Bitcoin-ABC / Diffs / Diff Testing started.

linting

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8940: Diff 15461.Jan 14 2020, 20:20

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8941: Diff 15462.Jan 14 2020, 20:22

Build Bitcoin-ABC / Diffs (Resource Intensive) / Resource Intensive Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Build Bitcoin-ABC / Diffs (Resource Intensive) / Resource Intensive Diff Testing passed.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Sanitizer tests look ok to me.

This revision is now accepted and ready to land.Jan 14 2020, 20:48

Closed by commit rABCbaef60890b54: Call FinalizeBlockAndInvalidate without cs_main held (authored by fpelliccioni). · Explain WhyJan 14 2020, 21:13

This revision was automatically updated to reflect the committed changes.

fpelliccioni added a commit: rABCbaef60890b54: Call FinalizeBlockAndInvalidate without cs_main held.

markblundeberg added inline comments.Jan 14 2020, 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.