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

Conflicts

No conflicts as of last run.

@promag I agree. This is also my preferred solution (see #18840). Happy to reopen that one.

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:

• It's more obvious what this solution is trying to do. This solution is plainly updating mempool status to reflect what's in the mempool, where #18440 is broadening a sync that happens in a different part of the code without a specific explanation about why it's necessary.
• #18440 waits for extra notifications to be handled that may not be relevant, blocking longer than it needs to.
• It's not clear whether #18440 fixes the problem in all cases, since it's assuming that the mempool implementation will alway queue the removedFromMempool notification before the wallet RPC tries to sync with the queue. And even if this does work now, there is no unit test or other coverage for it to make sure the sync runs at the right point in the broken case, to make sure this stays fixed in the future.

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; to CWallet and m_mempool_cv.notify_all() in transactionRemovedFromMempool() and m_mempool_cv.wait(lock, []{return !replaced_tx.fInMempool}) in bumpfee

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 730df28 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.

Should we do this in transactionRemovedFromMempool too perhaps?

Maybe it should avoid fInMempool in some cases, like in CWallet::AbandonTransaction:

@@ -1078,10 +1078,11 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
     std::set<uint256> done;

     // Can't mark abandoned if confirmed or in mempool
+    if (chain().isInMempool(hashTx)) return false;
     auto it = mapWallet.find(hashTx);
     assert(it != mapWallet.end());
     const CWalletTx& origtx = it->second;
-    if (origtx.GetDepthInMainChain() != 0 || origtx.InMempool()) {
+    if (origtx.GetDepthInMainChain() != 0) {
         return false;
     }

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 17220d6 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 (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. 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.

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 f54631d (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 d622b28 -> f54631d (compare)