Page MenuHomePhabricator

Make last disconnected block BLOCK_FAILED_VALID, even when aborted
ClosedPublic

Authored by fpelliccioni on Jan 13 2020, 18:01.

Details

Summary

This PR makes a number of improvements to the InvalidateBlock (invalidateblock RPC) and RewindBlockIndex functions, primarily around breaking up their long-term cs_main holding. In addition:

  • They're made safely interruptible (bitcoind can be shutdown, and no progress in either will be lost, though if incomplete, invalidateblock won't continue after restart and will need to be called again)
  • The validation queue is prevented from overflowing (meaning invalidateblock on a very old block will not drive bitcoind OOM) (see #14289).
  • invalidateblock won't bother to move transactions back into the mempool after 10 blocks (optimization).

Partial Backport of Bitcoin Core PR15402 (commit 519b0bc):
https://github.com/bitcoin/bitcoin/pull/15402/commits/519b0bc5dc5155b6f7e2362c2105552bb7618ad0

Make last disconnected block BLOCK_FAILED_VALID, even when aborted

Depends on D4804

Test Plan
ninja check-all

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.Jan 13 2020, 18:01
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 13 2020, 18:01
fpelliccioni planned changes to this revision.Jan 13 2020, 18:02
fpelliccioni retitled this revision from Granular invalidateblock and RewindBlockIndex to Make last disconnected block BLOCK_FAILED_VALID, even when aborted.Jan 13 2020, 18:04
fpelliccioni edited the summary of this revision. (Show Details)
fpelliccioni edited the test plan for this revision. (Show Details)
fpelliccioni edited reviewers, added: markblundeberg; removed: Fabien.
fpelliccioni requested review of this revision.Jan 13 2020, 18:15
fpelliccioni edited the summary of this revision. (Show Details)Jan 14 2020, 21:27
fpelliccioni added a reviewer: Fabien.
deadalnix requested changes to this revision.Jan 15 2020, 15:45
deadalnix added inline comments.
src/validation.cpp
3097 ↗(On Diff #15410)

I don't think that this is correct.

This revision now requires changes to proceed.Jan 15 2020, 15:45

Snippet of first build failure:

[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git config lfs.storage /home/teamcity/buildAgent/system/git/git-48AA3180.git/lfs
[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git config core.sparseCheckout true
[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git config http.sslCAInfo
[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git show-ref
[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git show-ref refs/tags/phabricator/diff/15494
[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git log -n1 --pretty=format:%H%x20%s 6c498e20bb715b3a87f5bfc02b32716e8b6d0b6a --
[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git -c credential.helper= checkout -q -f phabricator/diff/15494
[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git show-ref refs/tags/phabricator/diff/15494
[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] Cleaning Bitcoin ABC Staging in /home/teamcity/buildAgent/work/c4a5708f2bae7929 the file set ALL_UNTRACKED
[18:08:06] :			 [Update checkout directory (/home/teamcity/buildAgent/work/c4a5708f2bae7929)] /usr/bin/git clean -f -d -x
[18:08:06] : Build preparation done
[18:08:06]E: Step 1/1: Command Line
[18:08:06] :	 [Step 1/1] Ant JUnit report watcher
[18:08:06] :		 [Ant JUnit report watcher] Watching paths:
[18:08:06] :		 [Ant JUnit report watcher] +:build/test_bitcoin.xml
[18:08:06] :		 [Ant JUnit report watcher] +:build/**/junit_results*.xml
[18:08:06] :	 [Step 1/1] Starting: /home/teamcity/buildAgent/temp/agentTmp/custom_script4094997215296407092
[18:08:06] :	 [Step 1/1] in directory: /home/teamcity/buildAgent/work/c4a5708f2bae7929
[18:08:06]W:	 [Step 1/1] + : build-diff
[18:08:06] :	 [Step 1/1] Running build configuration 'build-diff'...
[18:08:06]W:	 [Step 1/1] + '[' -z build-diff ']'
[18:08:06]W:	 [Step 1/1] + echo 'Running build configuration '\''build-diff'\''...'
[18:08:06]W:	 [Step 1/1] ++ git rev-parse --show-toplevel
[18:08:06]W:	 [Step 1/1] + TOPLEVEL=/home/teamcity/buildAgent/work/c4a5708f2bae7929
[18:08:06]W:	 [Step 1/1] + export TOPLEVEL
[18:08:06]W:	 [Step 1/1] + trap print_sanitizers_log ERR
[18:08:06]W:	 [Step 1/1] +++ dirname ./contrib/teamcity/build-configurations.sh
[18:08:06]W:	 [Step 1/1] ++ cd ./contrib/teamcity
[18:08:06]W:	 [Step 1/1] ++ pwd
[18:08:06]W:	 [Step 1/1] + CI_SCRIPTS_DIR=/home/teamcity/buildAgent/work/c4a5708f2bae7929/contrib/teamcity
[18:08:06]W:	 [Step 1/1] + setup
[18:08:06]W:	 [Step 1/1] + : /home/teamcity/buildAgent/work/c4a5708f2bae7929/build
[18:08:06]W:	 [Step 1/1] + mkdir -p /home/teamcity/buildAgent/work/c4a5708f2bae7929/build/output
[18:08:06]W:	 [Step 1/1] ++ cd /home/teamcity/buildAgent/work/c4a5708f2bae7929/build
[18:08:06]W:	 [Step 1/1] ++ pwd
[18:08:06]W:	 [Step 1/1] + BUILD_DIR=/home/teamcity/buildAgent/work/c4a5708f2bae7929/build
[18:08:06]W:	 [Step 1/1] + export BUILD_DIR
[18:08:06]W:	 [Step 1/1] + TEST_RUNNER_FLAGS=--tmpdirprefix=output
[18:08:06]W:	 [Step 1/1] + cd /home/teamcity/buildAgent/work/c4a5708f2bae7929/build
[18:08:06]W:	 [Step 1/1] ++ nproc
[18:08:06]W:	 [Step 1/1] + THREADS=4
[18:08:06]W:	 [Step 1/1] + export THREADS
[18:08:06]W:	 [Step 1/1] + SAN_SUPP_DIR=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions
[18:08:06]W:	 [Step 1/1] + SAN_LOG_DIR=/tmp/sanitizer_logs
[18:08:06]W:	 [Step 1/1] + mkdir -p /tmp/sanitizer_logs
[18:08:06]W:	 [Step 1/1] + rm -rf '/tmp/sanitizer_logs/*'
[18:08:06]W:	 [Step 1/1] + export ASAN_OPTIONS=malloc_context_size=0:log_path=/tmp/sanitizer_logs/asan.log
[18:08:06]W:	 [Step 1/1] + ASAN_OPTIONS=malloc_context_size=0:log_path=/tmp/sanitizer_logs/asan.log
[18:08:06]W:	 [Step 1/1] + export LSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/lsan:log_path=/tmp/sanitizer_logs/lsan.log
[18:08:06]W:	 [Step 1/1] + LSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/lsan:log_path=/tmp/sanitizer_logs/lsan.log
[18:08:06]W:	 [Step 1/1] + export TSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/tsan:log_path=/tmp/sanitizer_logs/tsan.log
[18:08:06]W:	 [Step 1/1] + TSAN_OPTIONS=suppressions=/home/teamcity/buildAgent/work/c4a5708f2bae7929/test/sanitizer_suppressions/tsan:log_path=/tmp/sanitizer_logs/tsan.log
[18:08:06]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
[18:08:06]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
[18:08:06] :	 [Step 1/1] Error: Invalid build name 'build-diff'
[18:08:06]W:	 [Step 1/1] + case "$ABC_BUILD_NAME" in
[18:08:06]W:	 [Step 1/1] + echo 'Error: Invalid build name '\''build-diff'\'''
[18:08:06]W:	 [Step 1/1] + exit 2
[18:08:06]W:	 [Step 1/1] Process exited with code 2
[18:08:06]E:	 [Step 1/1] Process exited with code 2 (Step: Command Line)
deadalnix requested changes to this revision.Jan 17 2020, 01:51

See build failure.

This revision now requires changes to proceed.Jan 17 2020, 01:51

From what I can tell, this change is actually a bugfix and should have really been combined with D4802. Please get this landed soon.

markblundeberg requested changes to this revision.Jan 18 2020, 03:32
markblundeberg added inline comments.
src/validation.cpp
3105 ↗(On Diff #15494)

Note: we don't have BLOCK_FAILED_CHILD (instead FailedParent), and also these comments need to mention parking with equal weight.

fpelliccioni updated this revision to Diff 15690.Jan 20 2020, 15:31

rebase from master

fpelliccioni planned changes to this revision.Jan 20 2020, 15:34
fpelliccioni updated this revision to Diff 15691.Jan 20 2020, 17:26

remove reverted code

fpelliccioni planned changes to this revision.Jan 20 2020, 17:27
fpelliccioni updated this revision to Diff 15692.Jan 20 2020, 17:29

reverted code

fpelliccioni updated this revision to Diff 15693.Jan 20 2020, 17:39

fixes code comments

markblundeberg accepted this revision.Jan 21 2020, 00:05

OK, looks good. It looks like there are more backports to do, at least https://github.com/bitcoin/bitcoin/pull/16849 (possibly others too).

deadalnix accepted this revision.Jan 21 2020, 21:21
This revision is now accepted and ready to land.Jan 21 2020, 21:21