Do not clear validationinterface entries being executed #18551

sipa · 2020-04-07T02:17:50Z

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.

fanquake · 2020-04-07T02:19:13Z

I take it this resolves this comment?

sipa · 2020-04-07T02:20:51Z

@fanquake You're too fast. I was just looking to add a link where I made that comment.

promag · 2020-04-07T09:45:47Z

This change is now covered in #18471.

Bug in MainSignalsInstance::Clear could cause validation interface callbacks to be deleted during execution if UnregisterAllValidationInterfaces was called more than once. Bug was introduced in bitcoin#18524 and is fixed by bitcoin#18551

ryanofsky · 2020-04-07T12:17:53Z

Code review ACK c182fd5

I wrote a test for the bug that fails without the fix and passes with it: 4ba1627 (branch)

Feel free to use the test here and squash into the fix commit.

ryanofsky · 2020-04-07T12:30:37Z

Note: This PR fixes a bug in an internal function. The fix doesn't affect behavior of bitcoind, the gui, or any utilities because the function isn't currently called in a way that would trigger the bug

promag · 2020-04-07T18:23:42Z

Code review ACK c182fd5.

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.

Bug in MainSignalsInstance::Clear could cause validation interface callbacks to be deleted during execution if UnregisterAllValidationInterfaces was called more than once. Bug was introduced in bitcoin#18524 and is fixed by bitcoin#18551

sipa · 2020-04-07T19:58:58Z

@ryanofsky Cherry-picked. Also updated the PR description to say it's not currently observable.

ryanofsky

Code review ACK 2276339. No change to bugfix, just rebased and new test commit added since last review

jonasschnelli · 2020-04-08T10:10:04Z

utACK 2276339 - reviewed code and test (thanks @ryanofsky for adding the test).

maflcko · 2020-04-08T12:42:04Z

ACK 2276339 🎎

Show signature and timestamp

Signature:

Timestamp of file with hash 85ad0a848e9b8fccb5f691da9f517f86522c78158bd9a4bd80a6403b8f07cf8b -

maflcko · 2020-04-08T12:45:51Z

cc @sipsorcery (or anyone else on windows) any idea why this fails appveyor?

ryanofsky · 2020-04-08T13:07:52Z

cc @sipsorcery (or anyone else on windows) any idea why this fails appveyor?

Appveyor links

https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/32000822 (passed)
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/32021955 (failed)
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/32023789 (failed)
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/32039241 (pending)

I think this is the error message

src\test_bitcoin.exe -k stdout -e stdout 2> NUL
Command exited with code -1073740791

maflcko · 2020-04-08T13:10:30Z

Yeah, stuff like this #16976 makes it hard to debug

ryanofsky · 2020-04-08T13:16:58Z

The PR was rebased between the passing build and failing build https://github.com/bitcoin/bitcoin/compare/c182fd51..2276339a, but I'd guess probably the new unregister_all_during_call unit test is crashing and causing the failure

EDIT: Actually that diff link is misleading because appveyor builds branches merged with master. Still it seems pretty likely the new unregister_all_during_call test is causing this

ryanofsky · 2020-04-08T13:29:40Z

Could disable this if appveyor failures cause problems for other PRs:

--- a/src/test/validationinterface_tests.cpp
+++ b/src/test/validationinterface_tests.cpp
@@ -42,6 +42,9 @@ public:
 // https://github.com/bitcoin/bitcoin/pull/18551
 BOOST_AUTO_TEST_CASE(unregister_all_during_call)
 {
+#ifdef WIN32
+    return;
+#endif
     bool destroyed = false;
 
     CScheduler scheduler;

maflcko · 2020-04-08T13:34:15Z

appveyor was already failing on pretty much every pull, so I think this is not an issue

fanquake · 2020-04-08T13:54:45Z

The tests are now failing, and it was caused by this pull. Maybe we can disable as @ryanofsky suggested. I haven't been able to investigate further:

Running 370 test cases...
Assertion failed!

Program: C:\workspace\bitcoin\bin\test_bitcoin.exe
File: validationinterface.cpp, Line 93

Expression: !m_internals

ryanofsky · 2020-04-08T14:06:39Z

I think I see the problem. The failing assert is in RegisterBackgroundSignalScheduler

bitcoin/src/validationinterface.cpp

Line 93 in 1f70185

assert(!m_internals);

and is probably failing because the test isn't calling UnregisterBackgroundSignalScheduler to clean up after itself

Use TestingSetup fixture to fix unregister_all_during_call test not calling UnregisterBackgroundSignalScheduler, which could trigger an assert in RegisterBackgroundSignalScheduler when called in later tests Failure reported by fanquake <[email protected]> bitcoin#18551 (comment)

ryanofsky · 2020-04-08T14:20:46Z

#18563 should probably fix the test failures

13d2a33 Fix unregister_all_during_call cleanup (Russell Yanofsky) Pull request description: Use `TestingSetup` fixture to fix `unregister_all_during_call` test not calling `UnregisterBackgroundSignalScheduler`, which could trigger an assert in `RegisterBackgroundSignalScheduler` when called in later tests Failure reported by fanquake bitcoin#18551 (comment) ACKs for top commit: MarcoFalke: ACK 13d2a33 if appveyor unit tests pass Tree-SHA512: d2ec8ff14c54d97903af50031abfac1f38ec1c3aabc90371cfd5b79481fa69d3d77f339bfdf7d2178fd85e83402f72eda7cf4d339e5bbfa7e6e1a68836643b93

Bug in MainSignalsInstance::Clear could cause validation interface callbacks to be deleted during execution if UnregisterAllValidationInterfaces was called more than once. Bug was introduced in bitcoin#18524 and is fixed by bitcoin#18551

Use TestingSetup fixture to fix unregister_all_during_call test not calling UnregisterBackgroundSignalScheduler, which could trigger an assert in RegisterBackgroundSignalScheduler when called in later tests Failure reported by fanquake <[email protected]> bitcoin#18551 (comment)

Bug in MainSignalsInstance::Clear could cause validation interface callbacks to be deleted during execution if UnregisterAllValidationInterfaces was called more than once. Bug was introduced in bitcoin#18524 and is fixed by bitcoin#18551

Use TestingSetup fixture to fix unregister_all_during_call test not calling UnregisterBackgroundSignalScheduler, which could trigger an assert in RegisterBackgroundSignalScheduler when called in later tests Failure reported by fanquake <[email protected]> bitcoin#18551 (comment)

@ryanofsky

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 [[bitcoin/bitcoin#18551 | PR18551]] Depends on D6653 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: deadalnix, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6654

Summary: 13d2a33537a403ac47a989be92109d3214375b6a Fix unregister_all_during_call cleanup (Russell Yanofsky) Pull request description: Use `TestingSetup` fixture to fix `unregister_all_during_call` test not calling `UnregisterBackgroundSignalScheduler`, which could trigger an assert in `RegisterBackgroundSignalScheduler` when called in later tests Failure reported by fanquake bitcoin/bitcoin#18551 (comment) ACKs for top commit: MarcoFalke: ACK 13d2a33537a403ac47a989be92109d3214375b6a if appveyor unit tests pass Tree-SHA512: d2ec8ff14c54d97903af50031abfac1f38ec1c3aabc90371cfd5b79481fa69d3d77f339bfdf7d2178fd85e83402f72eda7cf4d339e5bbfa7e6e1a68836643b93 Backport of Core [[bitcoin/bitcoin#18563 | PR18563]] Depends on D6654 Test Plan: `ninja check` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6655

Bug in MainSignalsInstance::Clear could cause validation interface callbacks to be deleted during execution if UnregisterAllValidationInterfaces was called more than once. Bug was introduced in bitcoin/bitcoin#18524 and is fixed by bitcoin/bitcoin#18551

Use TestingSetup fixture to fix unregister_all_during_call test not calling UnregisterBackgroundSignalScheduler, which could trigger an assert in RegisterBackgroundSignalScheduler when called in later tests Failure reported by fanquake <[email protected]> bitcoin/bitcoin#18551 (comment)

Bug in MainSignalsInstance::Clear could cause validation interface callbacks to be deleted during execution if UnregisterAllValidationInterfaces was called more than once. Bug was introduced in bitcoin/bitcoin#18524 and is fixed by bitcoin/bitcoin#18551

Use TestingSetup fixture to fix unregister_all_during_call test not calling UnregisterBackgroundSignalScheduler, which could trigger an assert in RegisterBackgroundSignalScheduler when called in later tests Failure reported by fanquake <[email protected]> bitcoin/bitcoin#18551 (comment)

fanquake added the Validation label Apr 7, 2020

sipa mentioned this pull request Apr 7, 2020

refactor: drop boost::signals2 in validationinterface #18524

Merged

fanquake requested a review from ryanofsky April 7, 2020 02:21

promag mentioned this pull request Apr 7, 2020

qa: Test shared validation interface #18471

Closed

ryanofsky approved these changes Apr 7, 2020

View reviewed changes

sipa and others added 2 commits April 7, 2020 12:53

Do not clear validationinterface entries being executed

3c61abb

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.

sipa force-pushed the 202004_fix_validation_notify_clear branch from c182fd5 to 2276339 Compare April 7, 2020 19:57

ryanofsky approved these changes Apr 7, 2020

View reviewed changes

maflcko merged commit 1f70185 into bitcoin:master Apr 8, 2020

ryanofsky mentioned this pull request Apr 8, 2020

test: Fix unregister_all_during_call cleanup #18563

Merged

promag mentioned this pull request Apr 9, 2020

boost::signals2 disconnections don't block for slots to finish executing (& CValidationInterface lifecycle races) #18065

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Do not clear validationinterface entries being executed #18551

Do not clear validationinterface entries being executed #18551

Uh oh!

Conversation

sipa commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Apr 7, 2020

Uh oh!

sipa commented Apr 7, 2020

Uh oh!

promag commented Apr 7, 2020

Uh oh!

ryanofsky commented Apr 7, 2020

Uh oh!

ryanofsky commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Apr 7, 2020

Uh oh!

sipa commented Apr 7, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Apr 8, 2020

Uh oh!

maflcko commented Apr 8, 2020

Uh oh!

maflcko commented Apr 8, 2020

Uh oh!

ryanofsky commented Apr 8, 2020

Uh oh!

maflcko commented Apr 8, 2020

Uh oh!

ryanofsky commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Apr 8, 2020

Uh oh!

maflcko commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Apr 8, 2020

Uh oh!

ryanofsky commented Apr 8, 2020

Uh oh!

ryanofsky commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sipa commented Apr 7, 2020 •

edited

Loading

ryanofsky commented Apr 7, 2020 •

edited

Loading

ryanofsky commented Apr 8, 2020 •

edited

Loading

maflcko commented Apr 8, 2020 •

edited

Loading