refactor: Remove unused CTxMemPool::clear() helper

maflcko commented at 2:48 pm on September 7, 2020: member

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.

maflcko added the label Refactoring on Sep 7, 2020

maflcko force-pushed on Sep 7, 2020

hebasto commented at 3:42 pm on September 7, 2020: member

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

maflcko commented at 3:59 pm on September 7, 2020: member

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.

in src/test/mempool_tests.cpp:765 in fa0fbb3e5e outdated

761@@ -761,7 +762,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestryTests)
762     tb = make_tx(/* output_values */ {5 * COIN, 3 * COIN}, /* inputs */  {ta});
763     tc = make_tx(/* output_values */ {2 * COIN}, /* inputs */ {tb}, /* input_indices */ {1});
764     td = make_tx(/* output_values */ {6 * COIN}, /* inputs */ {tb, tc}, /* input_indices */ {0, 0});
765-    pool.clear();
766+    pool.clearTxs();

laanwj commented at 12:23 pm on September 8, 2020:

What is the difference between clearing and just instantiating a new mempool?

maflcko commented at 7:57 pm on September 8, 2020:

No difference. Though, a new mempool needs the old cs released and the fresh cs taken, so would be a bit more code

promag commented at 9:28 pm on September 8, 2020:

needs the old cs released

Why?

maflcko commented at 10:34 am on September 9, 2020:

Ok, it is not needed if the scope of pool is kept the same. Though, it is still a larger diff to instantiate a new mempool pool2, lock it and replace pool with pool2. Happy to do whatever reviewers want, so let me know if I should keep this or change it.

promag commented at 10:38 am on September 9, 2020:

I was just asking. It doesn’t need to be a big change, something like:

0{
1CTxMemPool pool;
2// ...
3}
4
5{
6CTxMemPool pool;
7// ...
8}

maflcko commented at 2:07 pm on September 9, 2020:

thx, fixed

laanwj commented at 12:24 pm on September 8, 2020: member

Concept ACK

in src/test/util/txmempool.h:10 in fad3a7c5b9 outdated

 5+#ifndef BITCOIN_TEST_UTIL_TXMEMPOOL_H
 6+#define BITCOIN_TEST_UTIL_TXMEMPOOL_H
 7+
 8+#include <txmempool.h>
 9+
10+struct TxMemPoolClearable : public CTxMemPool {

hebasto commented at 1:44 pm on September 8, 2020:

May I suggest to name this struct more generally, say TxMemPoolTesting, as it seems more functions could be added to it. E.g., a special version of GetTransactionAncestry() that requires external CTxMemPool::cs locking, see #19872 (review).

hebasto commented at 5:50 pm on September 8, 2020:

Ignore all above. It won’t work.

hebasto commented at 5:52 pm on September 8, 2020: member

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

 0--- a/src/test/txvalidationcache_tests.cpp
 1+++ b/src/test/txvalidationcache_tests.cpp
 2@@ -73,7 +73,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
 3         LOCK(cs_main);
 4         BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
 5     }
 6-    tx_pool.clearTxs();
 7+    WITH_LOCK(tx_pool.cs, tx_pool.clearTxs());
 8 
 9     // Test 3: ... and should be rejected if spend2 is in the memory pool
10     BOOST_CHECK(ToMemPool(spends[1]));
11@@ -82,7 +82,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, TestChain100Setup)
12         LOCK(cs_main);
13         BOOST_CHECK(::ChainActive().Tip()->GetBlockHash() != block.GetHash());
14     }
15-    tx_pool.clearTxs();
16+    WITH_LOCK(tx_pool.cs, tx_pool.clearTxs());
17 
18     // Final sanity test: first spend in tx_pool, second in block, that's OK:
19     std::vector<CMutableTransaction> oneSpend;
20diff --git a/src/test/util/txmempool.h b/src/test/util/txmempool.h
21index 7edc6d607..db52377b0 100644
22--- a/src/test/util/txmempool.h
23+++ b/src/test/util/txmempool.h
24@@ -9,9 +9,9 @@
25 
26 struct TxMemPoolClearable : public CTxMemPool {
27     /** Clear added transactions */
28-    void clearTxs()
29+    void clearTxs() EXCLUSIVE_LOCKS_REQUIRED(cs)
30     {
31-        LOCK(cs);
32+        AssertLockHeld(cs);
33         mapTx.clear();
34         mapNextTx.clear();
35         totalTxSize = 0;

?

maflcko force-pushed on Sep 9, 2020

maflcko commented at 10:42 am on September 9, 2020: member

@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.

maflcko force-pushed on Sep 9, 2020

dongcarl commented at 4:48 pm on September 10, 2020: contributor

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:

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

laanwj commented at 4:59 pm on September 10, 2020: member

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

dongcarl commented at 6:07 pm on September 10, 2020: contributor

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?

sdaftuar commented at 11:47 pm on September 10, 2020: member

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?

laanwj commented at 1:48 pm on September 11, 2020: member

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.

DrahtBot commented at 1:47 pm on September 19, 2020: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow
Concept ACK	hebasto, laanwj, jnewbery, practicalswift, fanquake

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

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.

maflcko commented at 2:25 pm on September 19, 2020: member

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.

sdaftuar commented at 3:15 pm on September 19, 2020: member

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.

DrahtBot added the label Needs rebase on Sep 23, 2020

maflcko force-pushed on Sep 28, 2020

maflcko closed this on Sep 28, 2020

maflcko deleted the branch on Sep 28, 2020

jnewbery commented at 8:24 am on October 30, 2020: contributor

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.

practicalswift commented at 12:52 pm on October 30, 2020: contributor

Concept ACK for the reasons @jnewbery mentioned.

bitcoin locked this on Feb 15, 2022

bitcoin unlocked this on Apr 28, 2022

maflcko restored the branch on Apr 28, 2022

maflcko reopened this on Apr 28, 2022

maflcko force-pushed on Apr 28, 2022

DrahtBot removed the label Needs rebase on Apr 28, 2022

jnewbery commented at 7:47 am on April 29, 2022: contributor

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.

maflcko added the label Waiting for author on Apr 29, 2022

maflcko commented at 8:14 am on April 29, 2022: member

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

maflcko commented at 8:26 am on April 29, 2022: member

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?

DrahtBot added the label Needs rebase on Apr 29, 2022

maflcko removed the label Waiting for author on May 27, 2022

fanquake referenced this in commit 9eaa5dbc81 on Oct 10, 2022

maflcko force-pushed on Oct 10, 2022

fanquake commented at 2:04 pm on October 10, 2022: member

Concept ACK

DrahtBot removed the label Needs rebase on Oct 10, 2022

maflcko renamed this:
~~txmempool: Remove unused clear() member function~~
refactor: Remove unused CTxMemPool::clear() helper
on Oct 10, 2022

sidhujag referenced this in commit 1d932c8c4c on Oct 10, 2022

fanquake requested review from glozow on Nov 30, 2022

in src/test/txvalidationcache_tests.cpp:81 in fa299b39ee outdated

76@@ -77,7 +77,9 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup)
77         LOCK(cs_main);
78         BOOST_CHECK(m_node.chainman->ActiveChain().Tip()->GetBlockHash() != block.GetHash());
79     }
80-    m_node.mempool->clear();
81+    BOOST_CHECK_EQUAL(m_node.mempool->size(), 1U);
82+    WITH_LOCK(m_node.mempool->cs, m_node.mempool->removeRecursive(CTransaction{spends[0]}, MemPoolRemovalReason::CONFLICT));

glozow commented at 2:45 pm on December 12, 2022:

Question: is there a reason you are calling removeRecursive() instead of the approach from #25073 / #19909 (comment) of instantiating a new mempool between test cases?

maflcko commented at 12:19 pm on December 13, 2022:

25073 is using a mempool local to the test only (and passing it to the miner). However, here the “global” mempool is used, which is also used by validation. So creating a new mempool would also require updating the pointer in validation.

I don’t think this is worth it, but I am happy to work on this, if reviewers want me to.

glozow commented at 1:54 pm on December 19, 2022:

Thanks, resolving, seems fine to me since the test is just checking block results when a conflicting tx is in mempool.

glozow commented at 2:46 pm on December 12, 2022: member

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.

txmempool: Remove unused clear() member function fa818e103c

maflcko force-pushed on Dec 13, 2022

glozow commented at 3:54 pm on December 19, 2022: member

ACK fa818e103c0ddb515f29ae9ce8de44931e12e69e

glozow merged this on Jan 4, 2023

glozow closed this on Jan 4, 2023

maflcko deleted the branch on Jan 4, 2023

sidhujag referenced this in commit db64668faf on Jan 4, 2023

bitcoin locked this on Jan 4, 2024

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

Reviews

Conflicts