The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #28026 (test: bugfix, synchronize indexes synchronously by furszy)

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.

Thx, edited OP

Little note: I think that the only downside here with respect to increasing the timeout to a much higher number or #28026 is that the tests could run forever if the index m_synced flag is never set by the thread. I'm not sure how the CI will behave in this scenario, guess that we will realize that something like this happened just by reading the last executed test name.

We've been plagued by intermittent timeouts locally (for months/years) and now in CI (for days), but never a thread that never finished. So I think it should be evident that this or ##28026 should be preferred. After all, it is no different than any other deadlock or infinite loop anywhere else in the code.

Actually, it is not when the thread never finishes. The thread could have finished, but never set the m_synced flag to true. Which would make BlockUntilSyncedToCurrentChain never return true. Which could happen on all the index sync errors.

utACK fabed7e

Could also restore the original behaviour by doing something like:

void IndexWaitSynced(BaseIndex& index, std::chrono::microseconds timeout)
{
    const auto end = SteadyClock::now() + timeout;
    while (!index.BlockUntilSyncedToCurrentChain()) {
        Assert(timeout <= 0s || SteadyClock::now < end);
        UninterruptibleSleep(100ms);
    }
}

and calling IndexWaitSynced(index, 10s) normally, but IndexWaitSynced(index, -1s) for coinstatsindex.

ACK fabed7e

as per #28026 (review) I like this approach more than running the sync synchronously.

Could also restore the original behaviour by doing something like:

The problem in the first place was that that the tests for txindex and blockfilterindex would have rare intermittent timeouts. So instead of making guesses about the slowest environment the tests could be run in I think it's ok to just have no timeout.

Which could happen on all the index sync errors.

Good point. Though, that seems like a bigger problem of the unit tests not handling AbortNode/FatalError/StartShutdown at all?

Which could happen on all the index sync errors.

Good point. Though, that seems like a bigger problem of the unit tests not handling AbortNode/FatalError/StartShutdown at all?

The quickest fix would be to add a ShutdownRequested() call into the IndexWaitSynced() loop. And verify at the end of the function that the index class is synced by calling:

BOOST_CHECK(index.GetSummary().synced);

In this way, we can be guarded against the infinite loop. And also verify the index synchronization result (which must be always successful).

The "AbortNode/FatalError/StartShutdown" treatment on the tests topic seems to be a different problem that we should aboard in the future, yeah. Here the test main thread will actively-wait for a flag to be set, and the flag could never be set. So, even if we start handling "AbortNode/FatalError/StartShutdown" on the base tests class, the error would need to be handled by a different thread (not sure if we already have something for it).