wallet: Mark replaced tx to not be in the mempool anymore #18842

The wallet does not mark the replaced tx as out-of-mempool. This causes failures in user scripts, because later RPCs may depend on this state change from bumpfee.

For example, the following might fail on current master:

txid = sendtoaddress(...)
bumpfee(txid)
abandontransaction(txid)  # fails because txid is still marked as "in mempool"

Fixes #18831

This is an alternative to my preferred fix #18840

re: #18842 (comment)

This is an alternative to my preferred fix #18840

fa3fdbfa89719797bba3b044b96caa984c5ff4be does seem a little fragile. I guess I was thinking of something more like:

diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index 0e7641ae320..8b288e62ff7 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -309,6 +309,11 @@ public:
         LOCK(::mempool.cs);
         return IsRBFOptIn(tx, ::mempool);
     }
+    bool isInMempool(const uint256& txid) override
+    {
+        LOCK(::mempool.cs);
+        return ::mempool.exists(txid);
+    }
     bool hasDescendantsInMempool(const uint256& txid) override
     {
         LOCK(::mempool.cs);
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index fb77c81cdcb..c76a361f261 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -185,6 +185,9 @@ public:
     //! Check if transaction is RBF opt in.
     virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0;
 
+    //! Check if transaction is in mempool.
+    virtual bool isInMempool(const uint256& txid) = 0;
+
     //! Check if transaction has descendants in mempool.
     virtual bool hasDescendantsInMempool(const uint256& txid) = 0;
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7a6deab1a8c..833414d4880 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -707,6 +707,12 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
 
     wtx.mapValue["replaced_by_txid"] = newHash.ToString();
 
+    // Refresh mempool status without waiting for transactionRemovedFromMempool
+    // notification so the wallet is in an internally consistent state and
+    // immediately knows the old transaction should not be considered trusted
+    // and is eligible to be abandoned
+    wtx.fInMempool = chain().isInMempool(originalHash);
+
     WalletBatch batch(*database, "r+");
 
     bool success = true;

Concept ACK.

This looks a bug to me, a test would be nice. @ryanofsky how could chain().isInMempool(originalHash) be true?

I find odd that mapValue["replaces_txid"] is updated in feebumper and mapValue["replaced_by_txid"] is updated in wallet. Maybe we could move CWallet::MarkReplaced to feebumper.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

Apparently the backwards compatibility test is still intermittently failing. Needs rebase.

Rebased. Not addressed any feedback, because the question by @promag hasn't been answered: "@ryanofsky how could chain().isInMempool(originalHash) be true?"

Rebased. Not addressed any feedback, because the question by @promag hasn't been answered: "@ryanofsky how could chain().isInMempool(originalHash) be true?"

I think it should be true because MarkReplaced should be called after the new transaction is committed and the old transaction is no longer in the mempool. As I understand it, the race happens because transactionRemovedFromMempool notification hasn't been processed yet, not because the old transaction is actually in the mempool.

Oh sorry, I answered why the question how could it be false, not how could it be true. It could be true if MarkReplaced was called at unexpected time on a transaction that wasn't successfully replaced, or maybe restored after a replacement was abandoned. So the change was suggested to make MarkReplaced safer. An alternative to:

wtx.fInMempool = chain().isInMempool(originalHash);

could be:

assert(!chain().isInMempool(originalHash));
wtx.fInMempool = false;

and I think either of these would be more safe. Adding the comment from #18842 (comment) could also be helpful.

Code review ACK faeedff5c87091fd83d2fb2b29eb49c948363f29 or for any of the suggested alternatives.

assert(!chain().isInMempool(originalHash));

This seems fragile, as it (in theory) may crash the node. Imagine calling bumpfee, then deprioritizing the tx with prioritizetransaction, then adding back the original tx (before the interface call can complete).

assert(!chain().isInMempool(originalHash));

This seems fragile, as it (in theory) may crash the node. Imagine calling bumpfee, then deprioritizing the tx with prioritizetransaction, then adding back the original tx (before the interface call can complete).

Right, that's why I suggested wtx.fInMempool = chain().isInMempool(originalHash); to do the correct thing in this case. But crashing seems better than doing the wrong thing if we don't want to handle this case.

Or you could replace crashing assert with throwing CHECK. Basically #18842 (comment) is still my preferred alternative, but all alternatives discussed and current PR seem fine.

to do the correct thing in this case

I don't think it is possible to do a thing that is correct in all scenarios. The wallet is an async subscriber of the mempool, so if there are any mempool RPCs called concurrently (and conflicting) with wallet RPCs there is no way to "do the correct thing". The design here is that the wallet will eventually catch up with the correct data if you give it enough time to eat all previous in-flight notifications.

Is there something not correct about wtx.fInMempool = chain().isInMempool(originalHash); with cs_wallet held?

It won't crash the node, but adding it doesn't guarantee that the in-mempool status is correct at all times.

Imagine calling bumpfee, concurrently you deprioritize the new tx with prioritizetransaction, then adding back the original tx with sendrawtransaction. Then wtx.fInMempool = chain().isInMempool(originalHash); assigns true. But immediately afterward you remove the deprioritization and call sendrawtransaction with the bumped-fee tx. sendrawtransaction is not a wallet RPC, so the status will incorrectly be fInMempool=true for the original tx, when it should be false.

Imagine calling bumpfee, concurrently you deprioritize the new tx with prioritizetransaction, then adding back the original tx with sendrawtransaction. Then wtx.fInMempool = chain().isInMempool(originalHash); assigns true

I think can you avoid problems here just holding cs_wallet throughout the bumpfee call. I haven't dug into the details of what we're doing, but clearly what we're doing now is incorrect.

I'm not sure if you prefer blindly assigning wtx.fInMempool = false over assigning wtx.fInMempool = isInMempool(wtx). If you do, I guess I have a different preference and think that latter would be more correct in long run even if it doesn't fix every problem right now. But either choice seems fine and would be better than current behavior.

Thanks, force pushed your patch (needed rebased)

utACK fa4e088cbac035b8029a10b492849540150d0622

Code review ACK fa4e088cbac035b8029a10b492849540150d0622, and previous ACK faeedff5c87091fd83d2fb2b29eb49c948363f29 is also still valid in case there's a preference for the original fix

This variant has two ACKs, so I think it is good to go in