The getrawmempool rpc should wait for ATMP to completely return before sending back the pool contents. Otherwise, the syncwithvalidationinterface rpc might race against ATMP and be a noop, even though it shouldn't.
When writing to ATMP, the cs_main lock is acquired. So we can wait for to release of cs_main to be sure that ATMP is done.
Effectively reverts #8244
454 | + 455 | + const UniValue txpool_json{mempoolToJSON(fVerbose)}; 456 | 457 | - return mempoolToJSON(fVerbose); 458 | + // Wait for ATMP calling thread to release the write lock: 459 | + LOCK(cs_main);
This needs to go before mempoolToJONS, no? ie just do a { LOCK(cs_main); }.
Acquiring the lock and immediately giving it back to the ATMP calling thread will not solve any races. (At least on my machine I can still see them.)
I guess, I could hold it for the whole duration of getrawmempool, if that is what you like.
Ah, oops, yes, sorry, indeed, wrong direction. I guess this is fine for now, but we really need to kill the mempool.cs/cs_main garbage.
I don't understand why does it matter waiting for cs_main when the result is already determined.
We want to wait for https://github.com/bitcoin/bitcoin/blob/8470e64724cb4a668079e7516dc61ec230346618/src/validation.cpp#L974
which happens under cs_main, not pool.cs.
A bit more background - we (essentially) use mempool.cs as a read lock and cs_main as a write lock for the mempool. However, we don't wait for cs_main in getrawmempool so you can have a situation where you start a ATMP call, it holds cs_main, and then getrawmempool is completely unsychronized with anything going on, confusing some tests. Ideally we'd move away from the rather-confused read-write-but-not-upgradeable mempool.cs, but a simple { LOCK(cs_main); } should fix the issue for now.
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.
No, we should not take a cs_main in SyncWithValidationInterface. The bug here is mempool.cs - we're using it as a read lock but not holding cs_main during the whole "write" in ATMP. The fix here is fine - sync getrawmempool with ATMP (as it should be). Alternatively we could hold cs_main for longer in ATMP, but adding a cs_main to validationinterface to fix a mempool-specific bug is way overkill.
Medium-term (ie post-0.16) we should look into making cs_main a read/write/upgrade lock for real and then this should fall out.
On January 27, 2018 10:17:34 PM UTC, "João Barbosa" notifications@github.com wrote:
Thanks for the explanation @MarcoFalke.
IMHO, and to cover other possible cases,
cs_maincould be locked right before returning inSyncWithValidationInterfaceQueue: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
syncwithvalidationinterfaceRPC afterSyncWithValidationInterfaceQueue().This alternative acknowledges it's not a
getrawmempoolbug.-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/12273#issuecomment-361019806
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#pullrequestreview-91735276.
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
Milestone
0.16.0