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:
0txid = sendtoaddress(...)
1bumpfee(txid)
2abandontransaction(txid) # fails because txid is still marked as "in mempool"
Fixes #18831
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:
0diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
1index 0e7641ae320..8b288e62ff7 100644
2--- a/src/interfaces/chain.cpp
3+++ b/src/interfaces/chain.cpp
4@@ -309,6 +309,11 @@ public:
5 LOCK(::mempool.cs);
6 return IsRBFOptIn(tx, ::mempool);
7 }
8+ bool isInMempool(const uint256& txid) override
9+ {
10+ LOCK(::mempool.cs);
11+ return ::mempool.exists(txid);
12+ }
13 bool hasDescendantsInMempool(const uint256& txid) override
14 {
15 LOCK(::mempool.cs);
16diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
17index fb77c81cdcb..c76a361f261 100644
18--- a/src/interfaces/chain.h
19+++ b/src/interfaces/chain.h
20@@ -185,6 +185,9 @@ public:
21 //! Check if transaction is RBF opt in.
22 virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0;
23
24+ //! Check if transaction is in mempool.
25+ virtual bool isInMempool(const uint256& txid) = 0;
26+
27 //! Check if transaction has descendants in mempool.
28 virtual bool hasDescendantsInMempool(const uint256& txid) = 0;
29
30diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
31index 7a6deab1a8c..833414d4880 100644
32--- a/src/wallet/wallet.cpp
33+++ b/src/wallet/wallet.cpp
34@@ -707,6 +707,12 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
35
36 wtx.mapValue["replaced_by_txid"] = newHash.ToString();
37
38+ // Refresh mempool status without waiting for transactionRemovedFromMempool
39+ // notification so the wallet is in an internally consistent state and
40+ // immediately knows the old transaction should not be considered trusted
41+ // and is eligible to be abandoned
42+ wtx.fInMempool = chain().isInMempool(originalHash);
43+
44 WalletBatch batch(*database, "r+");
45
46 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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
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:
0wtx.fInMempool = chain().isInMempool(originalHash);
could be:
0assert(!chain().isInMempool(originalHash));
1wtx.fInMempool = false;
and I think either of these would be more safe. Adding the comment from #18842 (comment) could also be helpful.
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 withprioritizetransaction, 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.
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.
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 withprioritizetransaction, then adding back the original tx withsendrawtransaction. Thenwtx.fInMempool = chain().isInMempool(originalHash);assignstrue
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.
