Page MenuHomePhabricator
Differential D6654

Merge #18551: Do not clear validationinterface entries being executed
ClosedPublic
Actions

Authored by jasonbcox on Jun 19 2020, 01:24.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC1064bda6981c: Merge #18551: Do not clear validationinterface entries being executed
Summary

2276339a176f83ffe8ceefb3e41ecca8601aa13b Add test for UnregisterAllValidationInterfaces bug (Russell Yanofsky)
3c61abbbc847d725f30d169278d84655571407c1 Do not clear validationinterface entries being executed (Pieter Wuille)

Pull request description:

The previous code for MainSignalsInstance::Clear would decrement the reference
count of every interface, including ones that were already Unregister()ed but
still being executed.

This fixes the issue pointed out here: https://github.com/bitcoin/bitcoin/pull/18524/files#r404395685 . It's not currently observable.

ACKs for top commit:

jonasschnelli:
  utACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b - reviewed code and test (thanks @ryanofsky for adding the test).
MarcoFalke:
  ACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b 🎎
ryanofsky:
  Code review ACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b. No change to bugfix, just rebased and new test commit added since last review

Tree-SHA512: c1d68e7c681a45c6cadc84e407c2266bcb4b12d34264e1232a61c4eadb74b551231c5a3b1d041de39f507aef4dfa7d4589b8bfe1833f069c739c6270d2a05dbe

Backport of Core PR18551

Depends on D6653

Test Plan

ninja check

Diff Detail

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

Event Timeline

jasonbcox created this revision.Jun 19 2020, 01:24
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 19 2020, 01:24
jasonbcox requested review of this revision.Jun 19 2020, 01:24
teamcity edited the summary of this revision. (Show Details)Jun 19 2020, 01:25

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

jasonbcox added a child revision: D6655: Merge #18563: test: Fix unregister_all_during_call cleanup.Jun 19 2020, 01:26
teamcity added a comment.Jun 19 2020, 01:29
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Snippet of first build failure:

[01:28:11] :	 [Step 1/1] [94/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/sighash_tests.cpp.o
[01:28:11] :	 [Step 1/1] [95/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/sighashtype_tests.cpp.o
[01:28:11] :	 [Step 1/1] [96/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/sigcheckcount_tests.cpp.o
[01:28:11] :	 [Step 1/1] [97/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/skiplist_tests.cpp.o
[01:28:11] :	 [Step 1/1] [98/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/streams_tests.cpp.o
[01:28:11] :	 [Step 1/1] [99/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/sync_tests.cpp.o
[01:28:11] :	 [Step 1/1] [100/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/timedata_tests.cpp.o
[01:28:11] :	 [Step 1/1] [101/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/torcontrol_tests.cpp.o
[01:28:11] :	 [Step 1/1] [102/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/transaction_tests.cpp.o
[01:28:11] :	 [Step 1/1] [103/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txvalidation_tests.cpp.o
[01:28:11] :	 [Step 1/1] [104/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/uint256_tests.cpp.o
[01:28:11] :	 [Step 1/1] [105/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txvalidationcache_tests.cpp.o
[01:28:11] :	 [Step 1/1] [106/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/undo_tests.cpp.o
[01:28:11] :	 [Step 1/1] [107/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/util_threadnames_tests.cpp.o
[01:28:11] :	 [Step 1/1] [108/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/util_tests.cpp.o
[01:28:11] :	 [Step 1/1] [109/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/work_comparator_tests.cpp.o
[01:28:11] :	 [Step 1/1] [110/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/__/rpc/test/server_tests.cpp.o
[01:28:11] :	 [Step 1/1] [111/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_tests.cpp.o
[01:28:11] :	 [Step 1/1] [112/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/versionbits_tests.cpp.o
[01:28:13] :	 [Step 1/1] [113/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validationinterface_tests.cpp.o
[01:28:17] :	 [Step 1/1] [114/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/txindex_tests.cpp.o
[01:28:18] :	 [Step 1/1] [115/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/util/setup_common.cpp.o
[01:28:18] :	 [Step 1/1] [116/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/denialofservice_tests.cpp.o
[01:28:18] :	 [Step 1/1] [117/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/blockfilter_index_tests.cpp.o
[01:28:18] :	 [Step 1/1] [118/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/validation_block_tests.cpp.o
[01:28:20] :	 [Step 1/1] [119/120] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/avalanche_tests.cpp.o
[01:28:22] :	 [Step 1/1] [120/120] Linking CXX executable src/test/test_bitcoin
[01:28:22]W:	 [Step 1/1] + TEST_BITCOIN_JUNIT=junit_results_unit_tests_without_wallet.xml
[01:28:22]W:	 [Step 1/1] + TEST_BITCOIN_SUITE_NAME='Bitcoin ABC unit tests without wallet'
[01:28:22]W:	 [Step 1/1] + ASAN_OPTIONS=malloc_context_size=0:log_path=/tmp/sanitizer_logs/asan.log
[01:28:22]W:	 [Step 1/1] + LSAN_OPTIONS=suppressions=/work/test/sanitizer_suppressions/lsan:log_path=/tmp/sanitizer_logs/lsan.log
[01:28:22]W:	 [Step 1/1] + TSAN_OPTIONS=suppressions=/work/test/sanitizer_suppressions/tsan:log_path=/tmp/sanitizer_logs/tsan.log
[01:28:22]W:	 [Step 1/1] + UBSAN_OPTIONS=suppressions=/work/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:log_path=/tmp/sanitizer_logs/ubsan.log
[01:28:22]W:	 [Step 1/1] + ./src/test/test_bitcoin --logger=HRF:JUNIT,message,junit_results_unit_tests_without_wallet.xml -- '-testsuitename=Bitcoin ABC unit tests without wallet'
[01:28:58] :	 [Step 1/1] Running 440 test cases...
[01:28:58] :	 [Step 1/1] 
[01:28:58] :	 [Step 1/1] Assertion occurred in a following context:
[01:28:58] :	 [Step 1/1]     ../src/test/sigcheckcount_tests.cpp:335
[01:28:58] :	 [Step 1/1] Assertion occurred in a following context:
[01:28:58] :	 [Step 1/1]     ../src/test/sigcheckcount_tests.cpp:335
[01:28:58] :	 [Step 1/1] Assertion occurred in a following context:
[01:28:58] :	 [Step 1/1]     ../src/test/sigcheckcount_tests.cpp:339
[01:28:58] :	 [Step 1/1] Assertion occurred in a following context:
[01:28:58] :	 [Step 1/1]     ../src/test/sigcheckcount_tests.cpp:339
[01:28:58] :	 [Step 1/1] Assertion occurred in a following context:
[01:28:58] :	 [Step 1/1]     ../src/test/sigcheckcount_tests.cpp:349
[01:28:58] :	 [Step 1/1] Assertion occurred in a following context:
[01:28:58] :	 [Step 1/1]     ../src/test/sigcheckcount_tests.cpp:349
[01:28:58] :	 [Step 1/1] Assertion occurred in a following context:
[01:28:58] :	 [Step 1/1]     ../src/test/sigcheckcount_tests.cpp:359
[01:28:58] :	 [Step 1/1] Assertion occurred in a following context:
[01:29:06]W:	 [Step 1/1] test_bitcoin: ../src/validationinterface.cpp:97: void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler&): Assertion `!m_internals' failed.
[01:29:06] :	 [Step 1/1]     ../src/test/sigcheckcount_tests.cpp:359unknown location(0):  [4;31;49mfatal error: in "versionbits_tests/versionbits_test": signal: SIGSEGV, si_code: -432025520 (memory access violation at address: 0x00000040) [0;39;49m
[01:29:06]W:	 [Step 1/1] 
[01:29:06] :	 [Step 1/1] ../src/test/versionbits_tests.cpp(226):  [1;36;49mlast checkpoint: "versionbits_test" fixture ctor [0;39;49m
[01:29:06]W:	 [Step 1/1]  [1;31;49m*** 1 failure is detected in the test module "Bitcoin ABC unit tests without wallet"
[01:29:06]W:	 [Step 1/1]  [0;39;49mcp: cannot stat '/work/build': No such file or directory
[01:29:06]W:	 [Step 1/1] cp: cannot stat '/work/ibd/debug.log': No such file or directory
[01:29:10]W:	 [Step 1/1] Process exited with code 201
[01:29:10]E:	 [Step 1/1] Process exited with code 201 (Step: Command Line)
Harbormaster completed remote builds in B11585: Diff 21561.Jun 19 2020, 01:29
teamcity added a comment.Jun 19 2020, 06:25
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Snippet of first build failure:

Build 'Bitcoin ABC Diffs / Diff Testing' #11895, branch 'phabricator/diff/21561'
Started 2020-06-19 01:25:33 on 'highperf1' by 'Phabricator Staging (phabricator-staging)'
Finished 2020-06-19 06:25:39 with status FAILURE 'Execution timeout (new)'
Fabien accepted this revision.Jun 19 2020, 07:15
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/validationinterface.cpp
76 ↗(On Diff #21561)

Braces

This revision is now accepted and ready to land.Jun 19 2020, 07:15
Fabien added a comment.Jun 19 2020, 07:17

Build failures are fixed in D6655

deadalnix added a subscriber: deadalnix.Jun 20 2020, 00:22

It would have been wiser to merge both.

This revision was landed with ongoing or failed builds.Jun 20 2020, 00:36
Closed by commit rABC1064bda6981c: Merge #18551: Do not clear validationinterface entries being executed (authored by MarcoFalke <falke.marco@gmail.com>, committed by deadalnix). · Explain Why
This revision was automatically updated to reflect the committed changes.
deadalnix added a commit: rABC1064bda6981c: Merge #18551: Do not clear validationinterface entries being executed.

Revision Contents
Changeset List

Diff 21573

View Options

src/Makefile.test.include

Loading...
View Options

src/test/CMakeLists.txt

Loading...
View Options

src/test/validationinterface_tests.cpp

Loading...
View Options

src/validationinterface.cpp

Loading...