RPC: Add new 'clearmempool' method #13836

domob1812 · 2018-08-01T16:49:40Z

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 · 2018-08-01T16:50:47Z

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.

maflcko · 2018-08-01T16:57:34Z

doc/release-notes.md

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

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.

jamesob · 2018-08-01T17:09:15Z

src/rpc/blockchain.cpp

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

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

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

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

jamesob · 2018-08-01T17:13:12Z

src/rpc/blockchain.cpp

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

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

Oh right, my mistake.

jonasschnelli · 2018-08-02T06:51:48Z

Yes. Why not.
Concept ACK

promag · 2018-08-02T12:43:00Z

src/rpc/blockchain.cpp

Use same key as?

bitcoin/src/rpc/blockchain.cpp

Line 1423 in 990e182

" \"size\": xxxxx, (numeric) Current tx count\n"

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

skeees

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

skeees · 2018-08-02T16:10:25Z

src/rpc/blockchain.cpp

maybe call this transactions_removed or something more informative

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.

skeees · 2018-08-02T16:11:02Z

src/rpc/blockchain.cpp

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

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

maflcko · 2018-08-02T16:36:29Z

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

domob1812 · 2018-08-02T17:02:45Z

@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 · 2018-08-02T17:15:56Z

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.

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.

promag · 2018-08-03T11:15:46Z

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

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 · 2018-08-03T19:07:37Z

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 · 2018-08-12T01:01:30Z

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 · 2018-08-12T01:22:42Z

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 · 2018-08-12T01:43:21Z

No more conflicts as of last run.

domob1812 · 2018-08-12T09:28:15Z

@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 · 2018-08-18T18:14:12Z

@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 · 2018-08-18T18:26:46Z

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

luke-jr · 2018-08-26T03:15:00Z

src/txmempool.cpp

    mapLinks.clear();
    mapTx.clear();
    mapNextTx.clear();
+    mapDeltas.clear();


mapDeltas probably shouldn't be cleared...?

luke-jr · 2018-08-26T03:15:49Z

src/txmempool.cpp

        assert(&tx == it->second);
    }
+    for (const auto& entry : mapDeltas) {
+        assert(mapTx.count(entry.first) > 0);


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

laanwj · 2018-09-11T07:05:39Z

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')

domob1812 · 2018-09-24T19:20:52Z

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

maflcko reviewed Aug 1, 2018

View reviewed changes

jamesob reviewed Aug 1, 2018

View reviewed changes

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

domob1812 force-pushed the clear-mempool branch from 9a502ee to 0fd9903 Compare August 2, 2018 09:03

promag reviewed Aug 2, 2018

View reviewed changes

laanwj added the Feature label Aug 2, 2018

domob1812 force-pushed the clear-mempool branch from 0fd9903 to 4b06094 Compare August 2, 2018 15:23

skeees reviewed Aug 2, 2018

View reviewed changes

domob1812 force-pushed the clear-mempool branch from 4b06094 to 4fb35b1 Compare August 2, 2018 17:12

domob1812 force-pushed the clear-mempool branch from 4fb35b1 to 838177c Compare August 2, 2018 17:16

Clear mapDeltas also in CTxMemPool::clear.

f73ef0c

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.

domob1812 force-pushed the clear-mempool branch from 838177c to 9029f0c Compare August 2, 2018 17:28

Add new 'clearmempool' RPC.

1976013

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 force-pushed the clear-mempool branch from 9029f0c to 1976013 Compare August 3, 2018 18:57

ken2812221 mentioned this pull request Aug 6, 2018

travis: Enable qt for all jobs #13515

Merged

DrahtBot mentioned this pull request Aug 12, 2018

Refactoring CRPCCommand with enum category #13945

Closed

luke-jr reviewed Aug 26, 2018

View reviewed changes

laanwj closed this Sep 11, 2018

domob1812 deleted the clear-mempool branch September 24, 2018 19:21

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

RPC: Add new 'clearmempool' method #13836

RPC: Add new 'clearmempool' method #13836

Uh oh!

Conversation

domob1812 commented Aug 1, 2018

Uh oh!

domob1812 commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skeees left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 2, 2018

Uh oh!

domob1812 commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domob1812 commented Aug 2, 2018

Uh oh!

promag commented Aug 3, 2018

Uh oh!

domob1812 commented Aug 3, 2018

Uh oh!

promag commented Aug 12, 2018

Uh oh!

sipa commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domob1812 commented Aug 12, 2018

Uh oh!

promag commented Aug 18, 2018

Uh oh!

sipa commented Aug 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domob1812 commented Sep 24, 2018

Uh oh!

domob1812 commented Aug 2, 2018 •

edited

Loading

sipa commented Aug 12, 2018 •

edited

Loading

DrahtBot commented Aug 12, 2018 •

edited

Loading

sipa commented Aug 18, 2018 •

edited

Loading

laanwj commented Sep 11, 2018 •

edited

Loading