refactor: Remove unused CTxMemPool::clear() helper #19909

Seems odd to have code in Bitcoin Core that is unused.

Moreover the function was broken (see #24145) and is brittle, as there is nothing that prevents similar bugs from re-appearing.

Fix both issues by replacing it with C++11 member initializers.

Concept ACK. Mind elaborating "useless calls" in fa947ccbd4579c866760b3b2a032bd3765043835 "validation: Remove useless call to mempool->clear()"?

The mempool.clear() in UnloadBlockIndex has been added in commit 51598b26319bf1ee98b399dee8152b902c62891a to clear global state between unit tests. Now that there is no global mempool anymore, this it not needed anymore. Also, I couldn't find it to be useful for anything else.

Concept ACK

Wrt to preserving recursive locking of CTxMemPool::cs mind considering the following patch:

--- a/src/test/txvalidationcache_tests.cpp
+++ b/src/test/txvalidationcache_tests.cpp
@@ -73,7 +73,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
         LOCK(cs_main);
         BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
     }
-    tx_pool.clearTxs();
+    WITH_LOCK(tx_pool.cs, tx_pool.clearTxs());
 
     // Test 3: ... and should be rejected if spend2 is in the memory pool
     BOOST_CHECK(ToMemPool(spends[1]));
@@ -82,7 +82,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
         LOCK(cs_main);
         BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
     }
-    tx_pool.clearTxs();
+    WITH_LOCK(tx_pool.cs, tx_pool.clearTxs());
 
     // Final sanity test: first spend in tx_pool, second in block, that's OK:
     std::vector<CMutableTransaction> oneSpend;
diff --git a/src/test/util/txmempool.h b/src/test/util/txmempool.h
index 7edc6d607..db52377b0 100644
--- a/src/test/util/txmempool.h
+++ b/src/test/util/txmempool.h
@@ -9,9 +9,9 @@
 
 struct TxMemPoolClearable : public CTxMemPool {
     /** Clear added transactions */
-    void clearTxs()
+    void clearTxs() EXCLUSIVE_LOCKS_REQUIRED(cs)
     {
-        LOCK(cs);
+        AssertLockHeld(cs);
         mapTx.clear();
         mapNextTx.clear();
         totalTxSize = 0;

?

@hebasto It hasn't been decided for the project whether to switch the mempool.cs to non-recursive mutex, nor in what way to do it, so I'll leave that test patch for later.

I'm not very familiar with these tests, so I may be completely off the mark here. However, it seems to me like the purpose of clearTxs is to reset the mempool without re-instantiating it. I wonder if we shouldn't consider just re-instantiating the mempool every time we want to clear it, as:

1. If there were a performance penalty, it wouldn't matter too much as it's in the test code
2. We wouldn't have to have the burden of keeping TxMemPoolClearable::clearTxs() and CTxMemPool in sync.

@dongcarl This was also my idea in the discussion here, but there wasn't really a decision: #19909 (review)

The mempool.clear() in UnloadBlockIndex has been added in commit 51598b2 to clear global state between unit tests. Now that there is no global mempool anymore, this it not needed anymore. Also, I couldn't find it to be useful for anything else.

Does the call to UnloadBlockIndex here: https://github.com/bitcoin/bitcoin/blob/a47e5964861dfb98d61719c9852e12fd6da84c31/src/init.cpp#L1562 Make any difference to the mempool that we should keep in mind?

Is this refactor important to other work? Having the ability to clear the mempool seems useful to me; even though we've never exposed it, I could imagine some situations where invoking the clear() function somehow (say via rpc) might be useful.

Alternatively -- and even more speculatively -- in thinking about how we update the mempool after a reorg, I've wondered if there might be solutions where clearing the mempool and re-adding things back might be better in some scenarios.

I don't think either of those is pressing though so if there's some good reason to get rid of it to help with other work in progress, then that's fine with me, we can always revisit the right design if the functionality I suggest might actually be useful. Just not sure if this might be wasted effort, if there's no important reason or followup work motivating this change?

I think you have a point there @sdaftuar. I'm not sure in how far it's here the case (there is no standard definition of the expectation for a "mempool interface"), but in some cases some methods just belong in an API (say, add/lookup/delete in a table) even if they're temporarily not used, and going over the top to clean APIs to the minimum set can make future changes harder.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26614 (Accurately account for mempool index memory by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

The clear method as currently implemented doesn't help with implementing a RPC that clears the mempool, because it also clears the fee estimates. If the method is kept, it should be renamed to ReInit (or similar), because all it does is (re-)initialize the member variables of the mempool.

Though, according to our release notes, member variables should be initialized inline via C++11 initializers to avoid uninitialized values. (This is what is being done in this pull)

Also, I can not imagine a single use case where clearing the whole mempool over RPC is useful. At most sniping a single tx might be useful. Though even that has been rejected at least twice in the past. Please refer to #15873 and #16523.

If you’re a miner, I could imagine it is possible that clearing the mempool could be helpful in the event of a dos-attack where pathological transaction chains (for example) cause block template creation to be very slow. So a use case could be to empty it and use other rpcs to manually refill it.

That’s just an example, and I don’t know if it is likely we’d support that anytime soon, but I could imagine the use case.

(Your point about fee estimation is a reasonable one, but I believe there is work happening elsewhere to decouple that from the mempool? )

At any rate I don’t feel strongly about this, if removing this function is helpful for other work I am not opposed either.

Concept ACK. Removing test-only code from the product is generally useful. Partly because it reduces the complexity of interfaces, reduces the binary size, etc, but more so because relying on test-only code paths increases the risk that the test logic diverges from the live logic. For example, if some initialization code was added to the mempool constructor, but not clear(), and tests assume that a call to clear() gives a completely freshly initialized mempool, then the test setup is not testing what we want it to.

Concept ACK for the reasons @jnewbery mentioned.

I wonder if we shouldn't consider just re-instantiating the mempool every time we want to clear it, as:

@dongcarl This was also my idea in the discussion here, but there wasn't really a decision: #19909 (review) @MarcoFalke did you consider this approach? On the face of it, it seems better to reinstantiate a mempool every time you want to clear it rather than having custom test code to reach into the object's members.

Yeah, I'll look into this. First part of that is #25024

In reply to Suhas point that the clear method could be used to wipe the txs for example during a reorg or by a miner to speed up block template creation: I am still doubtful that this method can actually achieve this goal, as it also resets other members of mempool. I think it would be sufficient and clearer to just use the existing removeRecursive functionality of the mempool to wipe all txs?

Concept ACK

Concept ACK to removing a function that is not used. I agree that, if manually removing individual or all transactions from mempool is a valid use case we want to implement, this function would not be sufficient.

ACK fa818e103c0ddb515f29ae9ce8de44931e12e69e