A wallet method (like bumping the fee) might have set fInMempool to false because the transaction was removed from the mempool (See commit fa4e088cbac035b8029a10b492849540150d0622).
Avoid setting it back to true (incorrectly) in the validation interface background thread.
Fixes #22357
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Not sure about this, can’t the transaction be removed from the mempool after isInMempool?
Wouldn’t be more correct to SyncWithValidationInterfaceQueue at the end of the RPC call - in the same way it’s called at the beginning? This ensures transactionRemovedFromMempool is processed to the caller?
I could be missing something but it seems to me neither solution is clearly correct in all cases. The advantages of this solution over #18440 are that:
But I do agree with the general point that a fully correct solution is probably a blocking solution. I just think the blocking should be targeted. Not more narrow or more wide than it needs to be. For example could add std::condition_variable m_mempool_cv; to CWallet and m_mempool_cv.notify_all() in transactionRemovedFromMempool() and m_mempool_cv.wait(lock, []{return !replaced_tx.fInMempool}) in bumpfee
For example could add
std::condition_variable m_mempool_cv;toCWalletandm_mempool_cv.notify_all()intransactionRemovedFromMempool()andm_mempool_cv.wait(lock, []{return !replaced_tx.fInMempool})inbumpfee
Thinking about this, in a really pathological case where a replaced transaction got added back to the mempool, this could hang. Dropping CWalletTx::fInMempool and giving transactions clear states like 730df28e00822f06c9fc11c86a74f764a2f5136c from #21206 would help here, because the replaced transaction could ambiguously transition from the in-mempool state to an intermediate in-mempool-but-replaced state to the inactive state, and the cv.wait() could just wait for the transaction to be out of the intermediate state.
Any case, will ACK this PR. IMO, this is the best fix for we have for now, and it is straightforward and simple, even if it is not 100% right in every case and doesn’t have a fancy test.
transactionRemovedFromMempool too perhaps?
Maybe it should avoid fInMempool in some cases, like in CWallet::AbandonTransaction:
0@@ -1078,10 +1078,11 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
1 std::set<uint256> done;
2
3 // Can't mark abandoned if confirmed or in mempool
4+ if (chain().isInMempool(hashTx)) return false;
5 auto it = mapWallet.find(hashTx);
6 assert(it != mapWallet.end());
7 const CWalletTx& origtx = it->second;
8- if (origtx.GetDepthInMainChain() != 0 || origtx.InMempool()) {
9+ if (origtx.GetDepthInMainChain() != 0) {
10 return false;
11 }
Should we do this in transactionRemovedFromMempool too perhaps?
Thx, done.
Maybe it should avoid fInMempool in some cases, like in CWallet::AbandonTransaction:
Maybe the mempool status should be removed and simply be queried when needed? Otherwise it just seems like applying random patches that happen to improve the situation, but don’t actually fix the underlying issue.
Maybe the mempool status should be removed and simply be queried when needed?
I think there are two drawbacks to querying the mempool when needed:
Conceptually it seems less appealing because it means coin selection, balance checking, etc code is combining mempool and chain state from two different points in time instead of one point. Instead of using chain and mempool state from the time of last notification, it is combining data from the last notification time with data from the current time. I don’t think the consequences of this are very bad but they could make coin selection and balance checking glitchy or weird if there is a backlog of notifications.
At least according to 17220d6325ef8d7789373055586e4332977077b0 which introduced the cached fInMempool field, there are some reasons to think this may be worse for performance.
Otherwise it just seems like applying random patches that happen to improve the situation, but don’t actually fix the underlying issue.
I think if we combine transaction states (730df28e00822f06c9fc11c86a74f764a2f5136c from #21206) with the notify suggestion mentioned earlier (in #22359 (comment) and #22359 (comment)), that is a clean and robust solution that gets rid of all race conditions. But since that requires reviewing and writing more code, using tx.fInMempool = chain.isInMempool(tx.GetHash()); is at least a straighforward workaround in the meantime.
transactionRemovedFromMempool. Since we know this workaround is imperfect and the goal of this PR is mainly to fix CI errors, I would probably be inclined to limit the workaround to as few places as possible where we have seen actual failures, instead of adding the workaround to as many places as possible, where there is some chance it might trigger new failures. But since this workaround is so straightforward and almost looks like a real fix, probably it doesn’t matter.
I think if we combine transaction states (730df28 from #21206) with the notify suggestion mentioned earlier (in #22359 (comment) and #22359 (comment)), that is a clean and robust solution that gets rid of all race conditions.
FWIW, I started implementing this in f54631dec3ccb4f2df1545eab86a3f9ad824c1e9 (branch). It could be cleaned up more, and doesn’t have a dedicated test, and might not be totally complete (might need one or two more cv.wait() calls), but this shows the general idea.
EDIT: Updated d622b28227efbb4e0709e768a089e655fceecc19 -> f54631dec3ccb4f2df1545eab86a3f9ad824c1e9 (compare)
