Details

Reviewers

deadalnix
markblundeberg
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGINGbd6d5140b5c1: Make last disconnected block BLOCK_FAILED_VALID, even when aborted
rABCbd6d5140b5c1: Make last disconnected block BLOCK_FAILED_VALID, even when aborted

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

Branch

arcpatch-D4929

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9077
Build 16116: Default Diff Build & Tests
Build 16115: arc lint + arc unit

Event Timeline

fpelliccioni created this revision.Jan 13 2020, 18:01

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 13 2020, 18:01

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

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 added a parent revision: D4804: Optimization: don't add txn back to mempool after 10 invalidates.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8906: Diff 15410.Jan 13 2020, 18:09

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

fixes

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing failed.

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)

Harbormaster failed remote builds in B8962: Diff 15494!Jan 15 2020, 18:08

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 mentioned this in D5013: CheckBlockIndex before unlocking cs_main.Jan 18 2020, 03:06

markblundeberg added a child revision: D5013: CheckBlockIndex before unlocking cs_main.Jan 18 2020, 03:22

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.