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.
I take it this resolves this comment?
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.
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
Code review ACK c182fd517db2337f61b0038ca929fcfd01f18f07.
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 https://github.com/bitcoin/bitcoin/pull/18524 and is
fixed by https://github.com/bitcoin/bitcoin/pull/18551
@ryanofsky Cherry-picked. Also updated the PR description to say it's not currently observable.
Code review ACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b. No change to bugfix, just rebased and new test commit added since last review
utACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b - reviewed code and test (thanks @ryanofsky for adding the test).
ACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b 🎎
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 2276339a176f83ffe8ceefb3e41ecca8601aa13b 🎎
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh20Qv+N8PfCJlrF51u9zKcnkK1GBJis/ghaoibar1FJ1i1HSbPRRL+eKYVv0Sy
1EtNyAVItG33UTdTRWO5ektfVHDweRO3XZZwknk0Jo/iUo+dLMhXoEvu4/izJ09V
3K/ae/hZDnX5rARkOIW00NYtFdm3ynWtPXG6g5p5U9RJeZ31StmJoXp8FESDRhsl
qx5vJSEhV7hG+oUl6seSJLFdMZ3F8E6/OPg7kBMCRPKJI9/CrKrbUMLUbiV8LZy3
RJFrcCBWWa0Nw5T+mxoHl83iHJZKnPK5cKLzNYt1IkBC826KJKE1sN5lPz6zReug
cMkr3ycS8TlXiNRo8fsQNaCV7xj0qAVcXgqqJurOyA0DadMSaddvj9NMD93BZDI+
xO6fY8bVY6c4yTY+le0Ap0LhTdtvK4w//ld8K2lPMEVuhH3m5e8vjJHW87VWQQiG
xL78hivW6rda/QqCKs4Ad5CupVLrCjVpDcCu2dZihoXmuqpXmH9Dekz2vdbR4/Il
nKUooeqh
=fz/C
-----END PGP SIGNATURE-----
Timestamp of file with hash 85ad0a848e9b8fccb5f691da9f517f86522c78158bd9a4bd80a6403b8f07cf8b -
</details>
cc @sipsorcery (or anyone else on windows) any idea why this fails appveyor?
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
Yeah, stuff like this #16976 makes it hard to debug
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
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:
// [#18551](/github-metadata-backup-bitcoin-bitcoin/18551/)
BOOST_AUTO_TEST_CASE(unregister_all_during_call)
{
+#ifdef WIN32
+ return;
+#endif
bool destroyed = false;
CScheduler scheduler;
appveyor was already failing on pretty much every pull, so I think this is not an issue
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
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