Page MenuHomePhabricator

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

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

Details

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

Event Timeline

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

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)

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 added a subscriber: Fabien.
Fabien added inline comments.
src/validationinterface.cpp
76

Braces

This revision is now accepted and ready to land.Jun 19 2020, 07:15

It would have been wiser to merge both.

This revision was landed with ongoing or failed builds.Jun 20 2020, 00:36
This revision was automatically updated to reflect the committed changes.