This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.
It adds a MockForward method to the scheduler class that iterates through the task queue and reschedules them to be delta_seconds sooner.
This is currently used to support functional testing of the “unbroadcast” set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698.
Some relevant info about the analysis of the cost (maintenance burden) vs benefit (supporting a better tested & simpler codebase) of these changes
pros:
cons:
explanation of the two use cases & workarounds.
REATTEMPT_BROADCAST_INTERVAL constant that indicates scheduler frequency could be defined as a chain param and made more frequent for regtest (thanks @MarcoFalke for the suggestion).MaybeResendWalletTxs which calls ResubmitWalletTransactionsToMempool which uses nNextResend to track time. nNextResend implements a mockable time so can be tested from the functional tests. If the scheduler mock is supported, the scheduler could directly invoke the resubmit function & drop the member var to maintain time tracking.
utACK 58c4c19.
This seems like a generally useful test harness, and the code looks basically correct to me. I think the pointer to the scheduler is a little crass, but it isn’t a bad option considering the scheduler is created as a static in init.cpp (someone can clean it up later if they make the schedule a non static).
The time delta technically could underflow, but I don’t think that’s the base time being subtracted from is a time point (and we’re far from the unix epoch time). If this weren’t just testing harness, it might be worth it to check for underflow, but since we’d basically be having to actively try to shoot ourselves in the foot during a test to get an underflow this usage is fine.
Great work!
Concept ACK.
Looks like there’s an issue with the new mockforward unit test, as most of the Travis instances are failing:
0test/scheduler_tests.cpp(158): Entering test case "mockforward"
1test/scheduler_tests.cpp(188): error: in "scheduler_tests/mockforward": check num_tasks == 1 has failed [3 != 1]
2test/scheduler_tests.cpp(191): error: in "scheduler_tests/mockforward": check counter == 2 has failed [0 != 2]
3test/scheduler_tests.cpp(197): error: in "scheduler_tests/mockforward": check delta > 2*60 && delta < 3*60 has failed
4test/scheduler_tests.cpp(158): Leaving test case "mockforward"; testing time: 6321us
5test/scheduler_tests.cpp(13): Leaving test suite "scheduler_tests"; testing time: 146946us
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
No objection, but some observations:
In light of the test failures on some platforms, we obviously need to make sure this is doing the right thing. Timestamp handling isn’t obviously trivial especially in combination with condition variables. See also the numerous scheduler crashed that no one can explain: #16307, #16027, #14200, …
“Mocking” code so that tests run the mocked code and production runs a different code might defeat the whole point of having the tests in the first place. Tests (especially functional tests) are supposed to test the production code (paths) as closely as possible.
So my personal preference would be to go with some minimally invasive and simple approach like the chainparam. But if everyone else likes this mock approach because it could be used for other testing needs as well, then I won’t object having this merged.
379+ RPCResults{},
380+ RPCExamples{""},
381+ }.Check(request);
382+
383+ if (!Params().MineBlocksOnDemand())
384+ throw std::runtime_error("mockscheduler for regression testing (-regtest mode) only");
120+ boost::unique_lock<boost::mutex> lock(newTaskMutex);
121+
122+ // use temp_queue to maintain updated schedule
123+ std::multimap<boost::chrono::system_clock::time_point, Function> temp_queue;
124+
125+ for (const auto& element : taskQueue){
5c9d5bed7205161d6870d199d223af37efe66179
nit, space before {.
10@@ -11,6 +11,7 @@
11 class BanMan;
12 class CConnman;
13 class CTxMemPool;
14+class CScheduler;
58c4c1943e082060f38054ed4738478635ab9c31
nit, keep sorted.
9 #include <outputtype.h>
10 #include <rpc/blockchain.h>
11 #include <rpc/server.h>
12 #include <rpc/util.h>
13 #include <script/descriptor.h>
14+#include <scheduler.h>
58c4c1943e082060f38054ed4738478635ab9c31
nit, keep sorted.
14+#include <scheduler.h>
15 #include <util/check.h>
16 #include <util/strencodings.h>
17 #include <util/system.h>
18 #include <util/validation.h>
19+#include <init.h>
58c4c1943e082060f38054ed4738478635ab9c31
Why is this included?
383+ if (!Params().MineBlocksOnDemand())
384+ throw std::runtime_error("mockscheduler for regression testing (-regtest mode) only");
385+
386+ RPCTypeCheck(request.params, {UniValue::VNUM});
387+
388+ g_rpc_node->scheduler->MockForward(boost::chrono::seconds(request.params[0].get_int64()));
58c4c1943e082060f38054ed4738478635ab9c31
Maybe check delta_time > 0?
Concept ACK.
Some minor comments otherwise change LGTM.
178+ // bump the scheduler forward 5 minutes
179+ scheduler.MockForward(boost::chrono::seconds(5*60));
180+
181+ // finish up
182+ MicroSleep(600);
183+ scheduler.stop(false);
ca57312e5b0dc9d28766369b963395f1690e0b4f
Maybe change this to:
0scheduler.scheduleFromNow([&]{ scheduler.stop(false); }, 1);
and remove the sleep above.
clever. thank you!
lets see if travis likes it 🤞🏽
I addressed review comments & tests are now passing CI. Added checks in RPC function & scheduler function to ensure reasonable values / no underflow.
This PR has received 3 ACKs - utACK from @JeremyRubin & concept ACKs from @promag @fanquake. Would appreciate re-review 🙏🏽
599@@ -570,6 +600,7 @@ static const CRPCCommand commands[] =
600
601 /* Not shown in help */
602 { "hidden", "setmocktime", &setmocktime, {"timestamp"}},
603+ { "hidden", "mockscheduler", &mockscheduler, {"delta_time"}},
mockscheduler would be assert(Params().MineBlocksOnDemand()).
static const commands[]= will get done prior to working out what chain is in use, so you’d have to do a much more serious rework to remove it from the list of rpcs entirely.
121+
122+ // use temp_queue to maintain updated schedule
123+ std::multimap<boost::chrono::system_clock::time_point, Function> temp_queue;
124+
125+ for (const auto& element : taskQueue) {
126+ boost::chrono::system_clock::time_point new_time = std::max(boost::chrono::system_clock::now() + delta_seconds, element.first) - delta_seconds;
What’s the problem of doing
0temp_queue.emplace_hint(temp_queue.cend(), element.first - delta_seconds, element.second);
Looks fine to me since wait_until returns when system time is later than that absolute time.
Didn’t check the case for boost < 1.50.
if element.first - delta_seconds < now everything will process fine (as in the previous version you reviewed)
but if delta_seconds > element.first then there could be an underflow. seems highly unlikely because element.first is a time point, but doesn’t hurt to have this guarantee
This looks equivalent to
0std::max(boost::chrono::system_clock::now(), element.first - delta_seconds);
Also perhaps you only want to invoke now() once and cache it rather than querying it for every queue element.
@sipa that misses the point above. if delta_seconds > element.first, it underflows and then it can be greater than now().
I guess equivalently, if delta_seconds is too large, now() + delta_seconds may overflow… but that seems less likely because we don’t know if this system_clock::now() which feeds into the queue is set to start at time 0 on startup.
Note also that caching now(), were emplace hint not used, would mean that the tmp queue would potentially be unstable ordered w.r.t. jobs with the same timeout.
if delta_seconds > element.first, it underflows and then it can be greater than now().
According to https://en.cppreference.com/w/cpp/chrono/duration, the representation type for the standard clock values is signed, so underflow should not be any issue (or at least not more than we need to worry about the clock overflowing in the first place).
Note also that caching now(), were emplace hint not used, would mean that the tmp queue would potentially be unstable ordered w.r.t. jobs with the same timeout.
If that’s a concern, it already is. There is no guarantee that subsequent calls to now() will be strictly increasing.
delta_seconds is reasonable. I’m thinking of asserting the value is between 0 & 3600 from both this MockForward method & the mockscheduler rpc method.
and reverting to the original temp_queue.emplace_hint(temp_queue.cend(), element.first - delta_seconds, element.second)
now() are strictly increasing (they’ll be monotonically increasing, but that’s not enough to guarantee a stable sorting order absent hints).
reverted to original (element.first - delta_seconds), but added checks in MockForward and mockscheduler that delta_seconds is between 0 and 3600.
which means we no longer invoke now() =P
lmk if there are any further concerns
154@@ -155,4 +155,45 @@ BOOST_AUTO_TEST_CASE(singlethreadedscheduler_ordered)
155 BOOST_CHECK_EQUAL(counter2, 100);
156 }
157
158+BOOST_AUTO_TEST_CASE(mockforward)
159+{
160+ CScheduler scheduler;
161+
162+ int counter{0};
163+ CScheduler::Function dummy = [&counter](){counter++;};
().
I think this is a style preference (plus it’s in tests) so it’s whatever.
For whatever reason, I prefer this form when it’s a function that should be called multiple times (as dummy is) and the no () form when it’s more akin to a std::bind for a call-once thing.
177+
178+ // bump the scheduler forward 5 minutes
179+ scheduler.MockForward(boost::chrono::seconds(5*60));
180+
181+ // ensure scheduler has chance to process all tasks queued for before 1 ms from now.
182+ scheduler.scheduleFromNow([&]{ scheduler.stop(false); }, 1);
&scheduler.
I think & is perfectly clear, it’s just for style.
I agree in principle if a lambda is multiple lines and has arguments it’s usually better to explicitly capture, but I think implicit capture is totally reasonable here.
&counter so I though you would like to do the same here.
mockscheduler RPC, just a simple call in the test framework would be nice.
thanks for review @promag!
RE testing mockscheduler RPC- as @JeremyRubin mentioned, my next PR uses it here , so the happy path will be tested. But if you prefer separate explicit test coverage, I can add.
I’ll incorporate style nits if I push & invalidate current ACKs
377+ },
378+ RPCResults{},
379+ RPCExamples{""},
380+ }.Check(request);
381+
382+ if (!Params().MineBlocksOnDemand()) {
setmocktime, but it feels very strange to use MineBlocksOnDemand to gate time mocking. Perhaps it’s worth adding a separate chainparams property for time mocking, and then using it for both setmocktime and mockscheduler.
AllowMockableTime() and having it call MineBlocksOnDemand – that way someone can clean up the logic later, I’m sure we’re doing other weird things like this that could also be fixed.
MineBlocksOnDemand felt weird. Added m_is_mockable_chain to chainparams & implemented for mockscheduler. If it looks okay I’ll do a follow up PR to use that for mocktime.
changes:
mockscheduler rpc updated to use new m_is_mockable_chain chainparams property instead of MineBlocksOnDemandmockscheduler and MockForward methods to ensure delta_seconds is within acceptable rangeother comments:
mockscheduler rpc, but testing the success case would require actually invoking something scheduled from the c++ code, making it look extremely similar to the PR that provides context for & builds on this one. link to test. So I don’t think it makes sense to test independently.So I believe all existing review comments are addressed.
This PR is ready for review :)
s390x travis build is failing with “Disk space is too low!” error. everything else is green
appveyor wallet_resendwallettransactions.py failed, seems unlikely that its related but not impossible since MaybeResendWalletTxs is called by the scheduler. investigating.
113@@ -114,6 +114,28 @@ void CScheduler::scheduleFromNow(CScheduler::Function f, int64_t deltaMilliSecon
114 schedule(f, boost::chrono::system_clock::now() + boost::chrono::milliseconds(deltaMilliSeconds));
115 }
116
117+void CScheduler::MockForward(boost::chrono::seconds delta_seconds)
118+{
119+ assert(delta_seconds.count() > 0 && delta_seconds.count() < 3600);
In commit 69468a93737ed9d4a38e04bdce20778ce8191810:
Could add a comment why 3600? Also, why not use a named compile time constant like boost::chrono::hours{1}?
54@@ -55,6 +55,11 @@ class CScheduler
55 // need more accurate scheduling, don't use this method.
56 void scheduleEvery(Function f, int64_t deltaMilliSeconds);
57
58+ // Regtest only- mock the scheduler to fast forward in time
0 /**
1 * Mock the scheduler to fast forward in time
This is not limited to regtest. It can be called in unit tests as well, or in #17037 testchains.
171+ // check taskQueue
172+ boost::chrono::system_clock::time_point first, last;
173+ size_t num_tasks = scheduler.getQueueInfo(first, last);
174+ BOOST_CHECK(num_tasks == 3);
175+
176+ boost::thread scheduler_thread(std::bind(&CScheduler::serviceQueue, &scheduler));
in commit 3679f096c2ef52fa8673e5cd4064154a1889fa12
No features of boost::thread are needed here, so you might as well use a plain std thread
0 std::thread scheduler_thread([&]() { scheduler.serviceQueue(); });
182+ scheduler.scheduleFromNow([&scheduler]{ scheduler.stop(false); }, 1);
183+ scheduler_thread.join();
184+
185+ // check that the queue only has one job remaining
186+ num_tasks = scheduler.getQueueInfo(first, last);
187+ BOOST_CHECK(num_tasks == 1);
in commit 3679f096c2ef52fa8673e5cd4064154a1889fa12:
0 BOOST_CHECK_EQUAL(num_tasks, 1);
boost_check_equal was causing a warning I didn’t understand, but after pushing I realized I just need to cast to size_t.
231@@ -231,7 +232,7 @@ class CTestNetParams : public CChainParams {
232 fDefaultConsistencyChecks = false;
233 fRequireStandard = false;
234 m_is_test_chain = true;
235-
236+ m_is_mockable_chain = false;
In commit 3392d690cfb338e1a9a9d7347710c0744e4486d5:
this should be true, otherwise one has to change this in the source code when wanting to test on testnet. While there is an argument for test params to be as close to main params as possible, I think this does not apply to params that have been introduced for primarily testing purposes. (See also my m_is_test_chain, which turned out to be too general and people prefer a per-parameter toggle).
So I think this should be true to not hinder testing on testnet.
m_is_mockable_chain to be true on testnet, wouldn’t it be better not to introduce the param and use the m_is_test_chain instead?
367@@ -366,6 +368,32 @@ static UniValue setmocktime(const JSONRPCRequest& request)
368 return NullUniValue;
369 }
370
371+static UniValue mockscheduler(const JSONRPCRequest& request)
372+{
373+ RPCHelpMan{"mockscheduler",
in commit cf04e02bbe14257a1513b029231cb6a21c9766ac:
0 RPCHelpMan{"mockscheduler",
Excessive whitespace?
RPCHelpMan calls in rpc/misc & rpc/net & wallet/rpcwallet. Are you seeing inconsistency? Is there something I’m missing?
387+ int64_t delta_seconds = request.params[0].get_int64();
388+ if ((delta_seconds <= 0) || (delta_seconds > 3600)) {
389+ throw std::runtime_error("delta_time must be between 0 and 3600 seconds (1 hr)");
390+ }
391+
392+ g_rpc_node->scheduler->MockForward(boost::chrono::seconds(delta_seconds));
in commit cf04e02bbe14257a1513b029231cb6a21c9766ac:
Would be nice to protect against null pointer dereference. See EnsureMemPool
ACK cf04e02bbe14257a1513b029231cb6a21c9766ac only some style nits 🔵
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK cf04e02bbe14257a1513b029231cb6a21c9766ac only some style nits 🔵
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUgrVAwAzQPXXwQxaHhiRYtQO0W21qQLUyCF92Bvjtq2RMMQnmkrQZ8sIeyFkka9
8Mm0FDE2e0eU4en2Xw4CK/7yFsWUxlOMx8vrOlzYttO9htkmVT95Rnm7HOuAShvav
9lcuEloyfjMe0bKAR+X/X51/roJhf+WSGrKsEFI8C+FV/bu/AbOOxlLEFiW9v6Wo3
1083aqh74lXb5B3Bf4aZJRSTAJhlUE9XNlMH664laqefks2HXyseZpvomRd/w90NX/
11VC9UzgeTUnSqUAGd278nFFAf1J5tqIfnE37USY1EokaZw4Yc8zsdYOYHUK1ZklEw
12ZHIF3WPRln9OaR7kLfWtHslQUnCo1797qP2ouWAVpgRVXl63cNmjMqRGJErGZtXK
1323bKPE3D1CgEdb+dO422dKlbiI4Nsdg0pkwnAR/z+KLBwCu+oov2KF3kPhwVdQfq
14pUNQMn/cS7X9QP7bHxQOlAnpl4ijuBJfbV+wqU+0fY/WfclKyM/20u7IFecoZFOz
15CYu4fn+Z
16=NQgO
17-----END PGP SIGNATURE-----
Timestamp of file with hash 6c8ec4d2d3fd87097e17c0ce53d45761554026de152c054c527f36f2fdc7f39b -
1268@@ -1269,6 +1269,9 @@ bool AppInitMain(NodeContext& node)
1269 CScheduler::Function serviceLoop = std::bind(&CScheduler::serviceQueue, &scheduler);
1270 threadGroup.create_thread(std::bind(&TraceThread<CScheduler::Function>, "scheduler", serviceLoop));
1271
1272+ assert(!node.scheduler);
1273+ node.scheduler = &scheduler;
src/test/util/setup_common.cpp, no?
Thank you for the review @MarcoFalke ! I addressed your concerns in a commit (with one open question), currently opened as a PR on my fork: https://github.com/amitiuttarwar/bitcoin/pull/2, will make as a follow up PR if this one gets accepted.
Tried to investigate the appveyor failure and cannot see how it could possibly be related to my changes.
I’d prefer to fixup the style fixes you want to make into the commits in this pull or not do them at all. Splitting them into a separate pull only causes noise in the git history and on GitHub.
Rationale is that we generally discourage style-only pull requests.
incorporated style updates & made NodeContext.scheduler a unique_ptr instead of raw pointer.
update: Travis build failing on my new test. Working on debugging.
Looks like travis fails with the boost exception “invalid argument” (https://github.com/bitcoin/bitcoin/pull/18037#issuecomment-580650220)
0Running 3 test cases...
1
2Test cases order is shuffled using seed: 902768437
3
4Entering test module "Bitcoin Core Test Suite"
5
6test/scheduler_tests.cpp(11): Entering test suite "scheduler_tests"
7
8test/scheduler_tests.cpp(112): Entering test case "singlethreadedscheduler_ordered"
9
10test/scheduler_tests.cpp(112): Leaving test case "singlethreadedscheduler_ordered"; testing time: 6488us
11
12test/scheduler_tests.cpp(38): Entering test case "manythreads"
13
14test/scheduler_tests.cpp(38): Leaving test case "manythreads"; testing time: 11938us
15
16test/scheduler_tests.cpp(156): Entering test case "mockforward"
17
18terminate called after throwing an instance of 'boost::wrapexcept<boost::condition_error>'
19
20 what(): boost::condition_variable::do_wait_until failed in pthread_cond_timedwait: Invalid argument
21
22unknown location(0): fatal error: in "scheduler_tests/mockforward": signal: SIGABRT (application abort requested)
23
24test/scheduler_tests.cpp(172): last checkpoint
25
26test/scheduler_tests.cpp(156): Leaving test case "mockforward"; testing time: 455us
27
28test/scheduler_tests.cpp(11): Leaving test suite "scheduler_tests"; testing time: 18977us
29
30Leaving test module "Bitcoin Core Test Suite"; testing time: 19091us
31
32*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
54@@ -55,6 +55,11 @@ class CScheduler
55 // need more accurate scheduling, don't use this method.
56 void scheduleEvery(Function f, int64_t deltaMilliSeconds);
57
58+ // Mock the scheduler to fast forward in time.
style-nit in commit 3f5cebe47b:
Using /** will make this a doxygen comment. I know that the other functions don’t use doxygen comment, but for new code we should aim for that.
1263@@ -1265,16 +1264,19 @@ bool AppInitMain(NodeContext& node)
1264 }
1265 }
1266
1267+ assert(!node.scheduler);
1268+ node.scheduler = MakeUnique<CScheduler>();
1269+
1270 // Start the lightweight task scheduler thread
1271- CScheduler::Function serviceLoop = std::bind(&CScheduler::serviceQueue, &scheduler);
1272+ CScheduler::Function serviceLoop = std::bind(&CScheduler::serviceQueue, node.scheduler.get());
style-nit in commit 047c1fd23ab7f281af7acd238f3bbb441817686c:
As you are changing this line anyway, might as well get rid of the std::bind mess
0CScheduler::Function serviceLoop = [&] { node.scheduler->serviceQueue(); };
156@@ -157,7 +157,6 @@ NODISCARD static bool CreatePidFile()
157 static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
158
159 static boost::thread_group threadGroup;
160-static CScheduler scheduler;
161
162 void Interrupt(NodeContext& node)
163 {
style-nit in commit 047c1fd:
It should be safe and a bit cleaner to reset the scheduler unique_ptr at the end of void Shutdown(NodeContext& node).
102@@ -103,10 +103,12 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
103 g_rpc_node = &m_node;
104 RegisterAllCoreRPCCommands(tableRPC);
105
106+ g_rpc_node->scheduler = MakeUnique<CScheduler>();
style-nit in commit 047c1fd23ab7f281af7acd238f3bbb441817686c:
g_rpc_node might go away (#17548), and using m_node directly would be cleaner code anyway.
~TestingSetup
107+
108 // We have to run a scheduler thread to prevent ActivateBestChain
109 // from blocking due to queue overrun.
110- threadGroup.create_thread(std::bind(&CScheduler::serviceQueue, &scheduler));
111- GetMainSignals().RegisterBackgroundSignalScheduler(scheduler);
112+ threadGroup.create_thread(std::bind(&CScheduler::serviceQueue, g_rpc_node->scheduler.get()));
style-nit in commit 047c1fd23ab7f281af7acd238f3bbb441817686c:
Same here
0 threadGroup.create_thread([&] { m_node.scheduler->serviceQueue(); });
ACK db45f73ab93e207f792fb5bb11ca1ec2885d8a91 , only some style changes 🖊
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK db45f73ab93e207f792fb5bb11ca1ec2885d8a91 , only some style changes 🖊
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYEACgkQzit1aX5p
7pUhXZwwAnlRWgEGarDT3ZkLJNksVg2V3EkG8VEMRyDGnmkUYCRNaeemikKnzxvI7
8rrqKckkaYGyZvYqLhgodn+OG+EoZOh3DBrRljH1jdl9DjsNp5VcNVoVV2SHqsdrp
9qfoJeMwuzCqo9XYluZ5pKvJCyXYu+JbKIkhojeLRO4G8G6xfR6vejRDRXeG8wnEL
10KBkFHveCBBsem3JXwjTgj2uxRLzArVg0x2/shMDMpCsDyWdTn+6ngrTWulAyt5qB
11QjlwhCvJ7jyiAp8ymQU0aiRIg7OERYvNbNLxtc3S/DKmMeUq9Tyb1xCQW+y3iD/j
12x3kQi0jSief82PhqT4FhdjZPDhu2PbZlVg/M4zghsUiLOA6B/s1M2g8qaqC7OjrW
13ucC9PFUen/W0eNl7IIE/SBQ8IfYyDpw/Kz32o7fv/dwUlYUM8qXi6KxYIpplS1OG
14pn/Zzh3vdyC6cqPJS0q67+0Crv5at3ac3xx8dF5dTuT5+LHcztXyhANVpJl/y7xt
15oXFK3lOi
16=G9JB
17-----END PGP SIGNATURE-----
Timestamp of file with hash 1ac115fc816927e857fea47e64737b58665a1508da322004b7d0622a653ebbbf -
Add MockForward method to the scheduler that mimics going into the future by rescheduling all items on the taskQueue to be sooner.
- also update test setup & access point in denial of service test
ACK 8bca30ea17cd4c1dacee28eaa27e5fa3493b021d, only change is some style fixups 🕓
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 8bca30ea17cd4c1dacee28eaa27e5fa3493b021d, only change is some style fixups 🕓
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUjeVAv/YuSt6tTjGv6SeqSUSmrURxHQKmaO8WtGS57M8zxKiQLp2IlBrXaccECP
8ktwXi7z+OxVlfaw/acuEoFZn5kIUjOC19D7tIegGh6qIWJxVYcT2cDghEG3JfTFD
9H+gDfpx6+JUjJcXybDwrc0jeOpva0aR6WOV/BBv+JuaOM0G2SXZPFS+DeUkSWGez
108gMhSY4FE1sjGxtYSyhdtlHpROL1qX22joa8kdgVplJIuQJOmn8TGjI/4s4Xu6j7
115Afa60Iqnbqb9paSMyEEi7XPhjOPKtLYWwB/K+itYjvbipaz4uw4yd5+YfMdYy45
12F/KSW9+7bYCSGE+ggEUQykrR60CYVPJay5WbwfDs0UdZhKmfyjlbx/iysTx63uKs
134PKeLZoNCJynQA+nodf1vrXuA8nxJgfYdhDP8bWnUCMdcP84FKPdrG7r3v2x6fBt
14MeCCRSOXPNEAFtu59xnZw9mc+Dctx5EUyznhy5ybp/3QCUlM9DhxvTbx0H7sFzDo
15BTRQiIFP
16=2pAG
17-----END PGP SIGNATURE-----
Timestamp of file with hash 0370e988925c0bd1dc04b6ff2d771181e26d2b0eb3b28b69eb3c0a0976243393 -
This PR seems to have made bitcoinbuilds.org pretty unhappy:
0 what(): boost::condition_variable::do_wait_until failed in pthread_cond_timedwait: Invalid argument
1unknown location(0): fatal error: in "scheduler_tests/mockforward": signal: SIGABRT (application abort requested)
2test/scheduler_tests.cpp(172): last checkpoint
Example build: https://bitcoinbuilds.org/index.php?job=5c006a9e-b8e6-4932-a6a8-3ead3d1fc188
Any ideas why we see a SIGABRT?
I think it happens at std::thread scheduler_thread([&]() { scheduler.serviceQueue(); }); in the scheduler tests.
Master (Linux 64 depends tests) fails when using ccache cache (seems to be reproducible 100%):
https://bitcoinbuilds.org/index.php?job=c1d3a0f5-7d47-40e6-9c9b-cac397c5ff30
Master succeeds when clearing the ccache cache (but keeping the dependency cache): https://bitcoinbuilds.org/index.php?job=f28f0356-d431-4428-832b-49bc870042f5