RPC: Add new 'clearmempool' method

domob1812 commented at 4:49 pm on August 1, 2018: contributor

The new clearmempool RPC method removes all transactions in the mempool. This can be useful for testing, and might be useful also in some other situations.

domob1812 commented at 4:50 pm on August 1, 2018: contributor

I don’t know if adding this new method is desired. I would have found it useful from time to time, but mostly related to testing (and not production use). For instance, it can be useful in combination with invalidateblock to prevent the transactions from invalidated blocks from getting remined (if that is desired).

Feel free to close if you think this is not useful to have.

in doc/release-notes.md:76 in 9a502eecbd outdated

72@@ -73,6 +73,7 @@ RPC changes
73 - The `fundrawtransaction` RPC will reject the previously deprecated `reserveChangeKey` option.
74 - `sendmany` now shuffles outputs to improve privacy, so any previously expected behavior with regards to output ordering can no longer be relied upon.
75 - The new RPC `testmempoolaccept` can be used to test acceptance of a transaction to the mempool without adding it.
76+- The new RPC `clearmempool` can be used to force all mempool transactions to be removed.

MarcoFalke commented at 4:57 pm on August 1, 2018:

If it is only for testing, it shoulnd’t be mentioned in the release notes?

domob1812 commented at 5:08 pm on August 1, 2018:

I don’t know about the exact policy and included it here to have them “complete”. But of course I’m happy to remove the mention if it is common to not include changes that are not meant (mainly) for end users.

domob1812 commented at 9:04 am on August 2, 2018:

Removed.

in src/rpc/blockchain.cpp:1946 in 9a502eecbd outdated

1941+        throw JSONRPCError(RPC_MISC_ERROR, "The mempool was not loaded yet");
1942+    }
1943+
1944+    mempool.clear();
1945+
1946+    return NullUniValue;

jamesob commented at 5:09 pm on August 1, 2018:

Could return the number of evicted txns to be more useful.

promag commented at 5:19 pm on August 1, 2018:

Then return an object which can have more keys in the future.

domob1812 commented at 5:31 pm on August 1, 2018:

Yes, that makes sense. If you generally think having this method is useful, I’ll add a useful return value to it (e.g. starting with the number of evicated tx).

domob1812 commented at 9:04 am on August 2, 2018:

Changed the RPC to return a JSON object, with nTx holding the number of evicted transactions.

in src/rpc/blockchain.cpp:1953 in 9a502eecbd outdated

1939+
1940+    if (!g_is_mempool_loaded) {
1941+        throw JSONRPCError(RPC_MISC_ERROR, "The mempool was not loaded yet");
1942+    }
1943+
1944+    mempool.clear();

jamesob commented at 5:13 pm on August 1, 2018:

Are we missing some lock acquisitions here (e.g. mempool.cs)?

domob1812 commented at 5:30 pm on August 1, 2018:

I thought so first myself, but CTxMempool::clear explicitly locks for you.

jamesob commented at 6:23 pm on August 1, 2018:

Oh right, my mistake.

fanquake added the label RPC/REST/ZMQ on Aug 1, 2018

jonasschnelli commented at 6:51 am on August 2, 2018: contributor

Yes. Why not. Concept ACK

domob1812 force-pushed on Aug 2, 2018

in src/rpc/blockchain.cpp:1936 in 0fd9903458 outdated

1931+            "clearmempool\n"
1932+            "\nRemoves all transactions from the mempool.\n"
1933+            "It will fail if a previous dump is still being loaded.\n"
1934+            "\nResult:\n"
1935+            "{                           (json object)\n"
1936+            "  \"nTx\": n,                 (numeric) The number of evicted transactions\n"

promag commented at 12:43 pm on August 2, 2018:

Use same key as?https://github.com/bitcoin/bitcoin/blob/990e182587999dba33f2a8172cc39d4a20d2e207/src/rpc/blockchain.cpp#L1423

domob1812 commented at 3:23 pm on August 2, 2018:

Yeah, that makes sense. (nTx was based on getblock, but getmempoolinfo is of course more relevant in this context.) Will update the commit accordingly.

laanwj added the label Feature on Aug 2, 2018

domob1812 force-pushed on Aug 2, 2018

in src/rpc/blockchain.cpp:1951 in 4b060948fc outdated

1946+    }
1947+
1948+    LOCK(mempool.cs);
1949+
1950+    UniValue result(UniValue::VOBJ);
1951+    result.pushKV("size", mempool.size());

skeees commented at 4:10 pm on August 2, 2018:

maybe call this transactions_removed or something more informative

domob1812 commented at 5:13 pm on August 2, 2018:

Changed. size is consistent with getmempoolinfo, but in that context “size” is clearly more descriptive than for clearmempool. I agree that here transactions_removed is a better name.

in src/rpc/blockchain.cpp:1953 in 4b060948fc outdated

1948+    LOCK(mempool.cs);
1949+
1950+    UniValue result(UniValue::VOBJ);
1951+    result.pushKV("size", mempool.size());
1952+
1953+    mempool._clear();

skeees commented at 4:11 pm on August 2, 2018:

is there a future safety benefit to calling the toplevel mempool.clear() here?

domob1812 commented at 5:13 pm on August 2, 2018:

Why not - changed to clear. (Especially since _clear does not AssertLockHeld(cs).)

skeees commented at 4:15 pm on August 2, 2018: contributor

concept ACK

I’d note that this is the first time CTxMempool::clear() will be called during normal operation (apart from startup / shutdown), so I think this requires extensive testing to make sure it leaves the mempool in a consistent state (suitable for re-use and with no cruft around).

For example, just a cursory look at that implementation suggests it might not be doing a full clear of every data structure (mapDeltas for example).

MarcoFalke commented at 4:36 pm on August 2, 2018: member

Should probably run a CTxMemPool::check after the clean and throw if the check fails?

domob1812 commented at 5:02 pm on August 2, 2018: contributor

@MarcoFalke: If we are worried that CTxMemPool::clean might leave the mempool in an inconsistent state, then we should add the check to clean (not the RPC invocation of it), right? I don’t think this is particularly necessary, but I can surely add it “just in case”. (But as I said, I think the proper place would be in CTxMemPool::clean and not the new RPC call.)

EDIT: That said, it seems to have benefits to add the check call in the RPC - we should probably lock cs_main for that (since it also depends on pcoinsTip), and that should probably not be done in the clear function itself to avoid overlocking.

domob1812 force-pushed on Aug 2, 2018

domob1812 commented at 5:15 pm on August 2, 2018: contributor

I’ve now added a call to check to the RPC (see my previous comment - while I think the check belongs semantically into clear, it requires to lock cs_main and I think it is better to do that in the RPC than in clear).

mapDeltas is a good observation - this seems to not be caught by check, either. I’ll add a test for consistently between mapDeltas and mapTx there separately.

domob1812 force-pushed on Aug 2, 2018

Clear mapDeltas also in CTxMemPool::clear.

CTxMemPool::clear() was missing mapDeltas, which should also be cleared
as part of the overall mempool state.  This change fixes that, and adds
a consistency check between mapDeltas and mapTx to CTxMemPool::check.

f73ef0cf35

domob1812 force-pushed on Aug 2, 2018

promag commented at 11:15 am on August 3, 2018: member

@skeees made an excellent observation. Maybe add tests involving unconfirmed wallet transactions after clearing the mempool? Also try to include other RPC’s related to the mempool, like sendrawtransaction, abandontransaction and testmempoolaccept?

Add new 'clearmempool' RPC.

The new 'clearmempool' RPC method removes all transactions in the
mempool.  This can be useful for testing, and might be useful also
in some other situations.

1976013161

domob1812 force-pushed on Aug 3, 2018

domob1812 commented at 7:07 pm on August 3, 2018: contributor

I’ve now split out the test into a new mempool_clear.py file, which I think fits better (especially if we want to have more extensive tests). The test already has an unconfirmed wallet transaction that gets cleared from the mempool and then verifies that it is still unconfirmed after mining a block.

I’m not sure what exactly you are thinking about with other mempool-related functions. In my opinion, the test strategy should look like this:

Create some “non-trivial” mempool; e.g. as now with a wallet transactions and a fee delta.
Call clearmempool.
Rely on CTxMemPool::check to verify that no inconsistencies are left (which might include extending the checker, as I did for mapDeltas).

I don’t think that sendrawtransaction or testmempoolaccept add anything to this. The main things to consider are that the mempool created through 1) is “complicated enough” and that check for step 3) is complete enough.

Do you have any concrete suggestions of what else we should do for step 1) and what might be missing for check?

promag commented at 1:01 am on August 12, 2018: member

I’m not sure what exactly you are thinking about with other mempool-related functions

Just a way to say that their functionality is preserved after clearmempool. For instance, sendrawtransaction tx -> clearmempool -> sendrawtransaction tx should work?

sipa commented at 1:22 am on August 12, 2018: member

What would this RPC be useful for? It seems pretty ill advised to expose it for production use, and for testing it can always be accomplished by restarting the node (and wiping mempool.dat).

DrahtBot commented at 1:43 am on August 12, 2018: member

domob1812 commented at 9:28 am on August 12, 2018: contributor

@sipa: The situation where I would have found it useful from time to time is certainly testing (not production). You are right that restarting with wiping mempool.dat can reproduce the same effect, but especially the latter is at least some hassle to do. (And honestly, when I last wanted to do it, I played around with -nopersistmempool but wasn’t able to get it working - I didn’t think about removing mempool.dat manually, as that seemed a bit like digging around in implementation details that shouldn’t concern me as a tester.)

But if you think we should rather not expose this, then we can of course close this PR. (And I’ll just move the fix for mapDeltas to a separate PR without the new RPC.)

promag commented at 6:14 pm on August 18, 2018: member

@sipa correct me if I’m wrong (can’t test ATM) but a new wallet transaction could be rejected and this would allow to add it?

sipa commented at 6:26 pm on August 18, 2018: member

@promag Yes, but other nodes will still reject it. Permitting is locally seems a bad idea in that case (and at the least a privacy leak).

in src/txmempool.cpp:591 in 1976013161

587@@ -588,6 +588,7 @@ void CTxMemPool::_clear()
588     mapLinks.clear();
589     mapTx.clear();
590     mapNextTx.clear();
591+    mapDeltas.clear();

luke-jr commented at 3:15 am on August 26, 2018:

mapDeltas probably shouldn’t be cleared…?

in src/txmempool.cpp:732 in 1976013161

727@@ -727,6 +728,9 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
728         assert(it2 != mapTx.end());
729         assert(&tx == it->second);
730     }
731+    for (const auto& entry : mapDeltas) {
732+        assert(mapTx.count(entry.first) > 0);

luke-jr commented at 3:15 am on August 26, 2018:

Deltas aren’t necessarily in the pool (needed to prioritise ones that would be rejected otherwise).

laanwj commented at 7:05 am on September 11, 2018: member

What would this RPC be useful for? It seems pretty ill advised to expose it for production use, and for testing it can always be accomplished by restarting the node (and wiping mempool.dat).

I agree with @sipa here. This has much foot-shooting potential and, besides for a few developer corner cases, doesn’t seem worth the maintenance and testing hassle.

(in practice, methods such as this are going to be suggested for any common ailment, “oh have you tried ‘clearmempool’)

laanwj closed this on Sep 11, 2018

domob1812 commented at 7:20 pm on September 24, 2018: contributor

Agreed - I also don’t see much use besides testing, so perhaps it is safer not to add this generally.

domob1812 deleted the branch on Sep 24, 2018

MarcoFalke locked this on Sep 8, 2021

RPC: Add new ‘clearmempool’ method #13836