This was hit twice on travis, you might find one of the logs here https://travis-ci.org/bitcoin/bitcoin/jobs/332197296 (might disappear soon, on reset)

A bit more background - we (essentially) use mempool.cs as a read lock and cs_main as a write lock for the mempool.

Why not just use mempool.cs for both read and write? Stopping a lot of stuff just to dump the mempool sounds bad.

@promag Pull request welcome, but I'd prefer not to do major refactoring in a bug-fix pull request.

Thanks for the explanation @MarcoFalke.

IMHO, and to cover other possible cases, cs_main could be locked right before returning in SyncWithValidationInterfaceQueue:

void SyncWithValidationInterfaceQueue() {
    AssertLockNotHeld(cs_main);
    // Block until the validation queue drains
    std::promise<void> promise;
    CallFunctionInValidationInterfaceQueue([&promise] {
        promise.set_value();
    });
    promise.get_future().wait();
    // Block until other tasks holding cs_main finish
    LOCK(cs_main);
}

Or in syncwithvalidationinterface RPC after SyncWithValidationInterfaceQueue().

This alternative acknowledges it's not a getrawmempool bug.

The question for me is whether the production (i.e. non-test) use of getrawmempool benefits from the added synchronization. If taking the lock is only to synchronize against the test-only syncwithvalidationinterface, then it doesn't seem right to pay the lock cost in a production path.

Also, looking at the code, it does seem like cs_main is held for all of ATMP, so I don't understand the comment from @TheBlueMatt.

It seems to me that { LOCK(cs_main); } at the start of syncwithvalidationinterface should work fine (i.e. lock and immediate release).

The idea is to guarantee the result has transactions already processed and are not in the queue to be processed. Once the lock is acquired before returning means that ATMP finished.

How does the user of getrawmempool use that synchronization guarantee? i.e. what followup would be invalid if the tx is not completely processed?

(we can move to IRC if this is too much back and forth)

@TheBlueMatt explains above #12273 (review).

I don't think this is a production/mainnet concern.

I understand the comment you linked. It still seems a bit better to solve the test issue by synchronizing an RPC call that is never used in production rather than getrawmempool, which could be used in production. But the performance difference is likely very small, so it's not a strong concern.

I disagree wholly that this is not a "production concern" - we could absolutely have users who are doing a getrawmempool and then calling wallet functions based on the result, introducing a new race for them. I believe we should be marking this 0.16 as a regression.

Wouldn't all such followup actions take cs_main, so would be safe?

Err, sorry, not wallet calls, sendrawtransaction followed by a getrawmempool to verify its there - if we can hit a realistic race in testing we probably should consider it a potentially-production-issue.

Err, sorry, not wallet calls, sendrawtransaction followed by a getrawmempool to verify its there

sendrawtransaction fails if rejected by mempool. But it's a regression like you said.

One naive usage (which is already at least somewhat racy, but...) would be to do a getrawmempool to cheaply look up a transaction you just sent's fee. Not a massive deal, but definitely a regression worth fixing.

With the proposed fix, sendrawtransaction followed by a getrawmempool is not guaranteed to return the sent transaction, since the lock is taken after the results are computed.

Oh, I seem to have confused myself an mis-remembered the issue here - you cannot hit this purely from RPC, but can hit wallet errors:
If you're polling getrawmempool to wait for a transaction to appear in your mempool, you can then race ATMP and, thus, see a balance that is as-of an old mempool and not the one you just got out of getrawmempool

Changed milestone to 0.16

There has been too much discussion around this simple fix.

Closing in favor of #12368