Do not clear validationinterface entries being executed

sipa commented at 2:17 am on April 7, 2020: member

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 added the label Validation on Apr 7, 2020

fanquake commented at 2:19 am on April 7, 2020: member

I take it this resolves this comment?

sipa commented at 2:20 am on April 7, 2020: member

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

fanquake requested review from ryanofsky on Apr 7, 2020

promag commented at 9:45 am on April 7, 2020: member

This change is now covered in #18471.

ryanofsky referenced this in commit 4ba1627fb9 on Apr 7, 2020

ryanofsky commented at 12:17 pm on April 7, 2020: member

Code review ACK c182fd517db2337f61b0038ca929fcfd01f18f07

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

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

ryanofsky approved

ryanofsky commented at 12:30 pm on April 7, 2020: member

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 commented at 6:23 pm on April 7, 2020: member

Code review ACK c182fd517db2337f61b0038ca929fcfd01f18f07.

Do not clear validationinterface entries being executed

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.

3c61abbbc8

Add test for UnregisterAllValidationInterfaces bug

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 https://github.com/bitcoin/bitcoin/pull/18524 and is
fixed by https://github.com/bitcoin/bitcoin/pull/18551

2276339a17

sipa force-pushed on Apr 7, 2020

sipa commented at 7:58 pm on April 7, 2020: member

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

ryanofsky approved

ryanofsky commented at 8:04 pm on April 7, 2020: member

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

jonasschnelli commented at 10:10 am on April 8, 2020: contributor

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

MarcoFalke commented at 12:42 pm on April 8, 2020: member

ACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b 🎎

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b 🎎
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh20Qv+N8PfCJlrF51u9zKcnkK1GBJis/ghaoibar1FJ1i1HSbPRRL+eKYVv0Sy
 81EtNyAVItG33UTdTRWO5ektfVHDweRO3XZZwknk0Jo/iUo+dLMhXoEvu4/izJ09V
 93K/ae/hZDnX5rARkOIW00NYtFdm3ynWtPXG6g5p5U9RJeZ31StmJoXp8FESDRhsl
10qx5vJSEhV7hG+oUl6seSJLFdMZ3F8E6/OPg7kBMCRPKJI9/CrKrbUMLUbiV8LZy3
11RJFrcCBWWa0Nw5T+mxoHl83iHJZKnPK5cKLzNYt1IkBC826KJKE1sN5lPz6zReug
12cMkr3ycS8TlXiNRo8fsQNaCV7xj0qAVcXgqqJurOyA0DadMSaddvj9NMD93BZDI+
13xO6fY8bVY6c4yTY+le0Ap0LhTdtvK4w//ld8K2lPMEVuhH3m5e8vjJHW87VWQQiG
14xL78hivW6rda/QqCKs4Ad5CupVLrCjVpDcCu2dZihoXmuqpXmH9Dekz2vdbR4/Il
15nKUooeqh
16=fz/C
17-----END PGP SIGNATURE-----

Timestamp of file with hash 85ad0a848e9b8fccb5f691da9f517f86522c78158bd9a4bd80a6403b8f07cf8b -

MarcoFalke merged this on Apr 8, 2020

MarcoFalke closed this on Apr 8, 2020

MarcoFalke commented at 12:45 pm on April 8, 2020: member

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

ryanofsky commented at 1:07 pm on April 8, 2020: member

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

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

MarcoFalke commented at 1:10 pm on April 8, 2020: member

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

ryanofsky commented at 1:16 pm on April 8, 2020: member

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 commented at 1:29 pm on April 8, 2020: member

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

 0--- a/src/test/validationinterface_tests.cpp
 1+++ b/src/test/validationinterface_tests.cpp
 2@@ -42,6 +42,9 @@ public:
 3 // [#18551](/bitcoin-bitcoin/18551/)
 4 BOOST_AUTO_TEST_CASE(unregister_all_during_call)
 5 {
 6+#ifdef WIN32
 7+    return;
 8+#endif
 9     bool destroyed = false;
10 
11     CScheduler scheduler;

MarcoFalke commented at 1:34 pm on April 8, 2020: member

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

fanquake commented at 1:54 pm on April 8, 2020: member

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:

0Running 370 test cases...
1Assertion failed!
2
3Program: C:\workspace\bitcoin\bin\test_bitcoin.exe
4File: validationinterface.cpp, Line 93
5
6Expression: !m_internals

ryanofsky commented at 2:06 pm on April 8, 2020: member

I think I see the problem. The failing assert is in RegisterBackgroundSignalScheduler https://github.com/bitcoin/bitcoin/blob/1f70185a809362117a8158e386fdead85728794f/src/validationinterface.cpp#L93

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

ryanofsky referenced this in commit 13d2a33537 on Apr 8, 2020

ryanofsky commented at 2:20 pm on April 8, 2020: member

#18563 should probably fix the test failures

MarcoFalke referenced this in commit 2392566284 on Apr 8, 2020

sidhujag referenced this in commit 93082b3112 on Apr 8, 2020

sidhujag referenced this in commit a032db8672 on Apr 8, 2020

glozow referenced this in commit d7bce8a0d7 on Apr 10, 2020

glozow referenced this in commit 9a83c0971e on Apr 10, 2020

HashUnlimited referenced this in commit c33572b2d3 on Apr 17, 2020

HashUnlimited referenced this in commit ab68ba7273 on Apr 17, 2020

deadalnix referenced this in commit 1064bda698 on Jun 20, 2020

deadalnix referenced this in commit 6f74431345 on Jun 20, 2020

metalicjames referenced this in commit 09af10dec5 on Aug 5, 2020

janus referenced this in commit d90950d5f8 on Nov 5, 2020

janus referenced this in commit c5ded8c219 on Nov 8, 2020

backpacker69 referenced this in commit aa49101317 on Mar 28, 2021

backpacker69 referenced this in commit 1a28b719bb on Mar 28, 2021

DrahtBot locked this on Feb 15, 2022

Do not clear validationinterface entries being executed #18551