WIP - Add maxmempool RPC #21780

rebroad commented at 11:25 AM on April 26, 2021: contributor

Not entirely sure how useful this is given that currently when the mempool is reduced in size, the memory is not yet freed - perhaps this can come in a later pull request.

It's WIP as currently it doesn't work from bitcoin-cli - and I don't quite know why! (can someone help please? it works from within the console of the GUI though).

Add maxmempool RPC 040b280c66

jnewbery commented at 11:51 AM on April 26, 2021: member

Not entirely sure how useful this is ...

It doesn't seem very useful to me. It's already possible to set the max mempool size using a command line arg/config option. Being able to set the value dynamically doesn't seem to add any benefit.

What problem is this solving for you?

DrahtBot added the label RPC/REST/ZMQ on Apr 26, 2021

rebroad commented at 4:44 PM on April 26, 2021: contributor

@jnewbery at the moment, it's helping me test additional features I'm adding, but the other benefit is that I can have bitcoin-qt running low memory at some times, and at higher memory at others, without needing to restart the process. This is useful when using it on a machine that sometimes needs to be used for other things, and at other times can be dedicated to bitcoin.

jnewbery commented at 5:46 PM on April 26, 2021: member

I'm leaning towards a concept NACK. Neither of those seem like a convincing reason to add more dynamic configuration features.

luke-jr commented at 8:01 PM on June 11, 2021: member

Concept ACK: It makes sense for users to be able to control Bitcoin's memory usage dynamically if they might need more memory for other things.

Not so sure about the interface here, though. I would suggest a more generic config-changing RPC that can be reused with other parameters in the future.

Is there a reason not to call LimitMempoolSize to get an immediate response?

in src/rpc/blockchain.cpp:557 in 040b280c66

 551 | @@ -552,6 +552,32 @@ UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose, bool include_mempoo
 552 |      }
 553 |  }
 554 |  
 555 | +static RPCHelpMan maxmempool()
 556 | +{
 557 | +    return RPCHelpMan{"maxmempool",

kristapsk commented at 8:10 PM on June 11, 2021:

Not sure about concept ACK/NACK here myself yet, but better naming would be setmaxmempool IMO.

in src/rpc/blockchain.cpp:570 in 040b280c66

 565 | +                    HelpExampleCli("maxmempool", "150") + HelpExampleRpc("maxmempool", "150")
 566 | +                },
 567 | +        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 568 | +{
 569 | +    int nSize = request.params[0].get_int();
 570 | +    int64_t nMempoolSizeMax = nSize * 1000000;

luke-jr commented at 2:53 AM on June 25, 2021:

This would overflow at 4.3 GB before reaching the int64 conversion.

IMO, just keep it int64 the whole way.

in src/rpc/blockchain.cpp:572 in 040b280c66

 567 | +        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 568 | +{
 569 | +    int nSize = request.params[0].get_int();
 570 | +    int64_t nMempoolSizeMax = nSize * 1000000;
 571 | +    int64_t nMempoolSizeMin = gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT) * 1000 * 40;
 572 | +    if (nMempoolSizeMax < 0 || nMempoolSizeMax < nMempoolSizeMin)

luke-jr commented at 2:54 AM on June 25, 2021:

Need braces here

luke-jr referenced this in commit 1a9386eb58 on Jun 27, 2021

luke-jr referenced this in commit 5f280a2adc on Sep 10, 2021

luke-jr commented at 12:39 AM on September 10, 2021: member

Rebased and addressed reviews here https://github.com/bitcoin/bitcoin/compare/master...luke-jr:rpc_maxmempool

MarcoFalke commented at 11:49 AM on October 22, 2021: member

Closing for now. This needs rebase and there hasn't been any activity for months.

You can leave a comment if you want this to be reopened. Though, please make sure the code is passing tests and is ready for review.

MarcoFalke closed this on Oct 22, 2021

rebroad commented at 8:31 AM on November 2, 2021: contributor

Concept ACK: It makes sense for users to be able to control Bitcoin's memory usage dynamically if they might need more memory for other things.

Not so sure about the interface here, though. I would suggest a more generic config-changing RPC that can be reused with other parameters in the future.

Is there a reason not to call LimitMempoolSize to get an immediate response?

Hi Luke, sorry for the delay in responding. I must admit, that I am not sure yet if reducing the maxmempool actualy frees up the memory used - have you tested this? Also, how do I incorporate your commits? (or is it possible to transfer this pull request to you?)

luke-jr commented at 4:24 PM on November 2, 2021: member

I am not sure yet if reducing the maxmempool actualy frees up the memory used - have you tested this?

That's the purpose of adding 43eb542612eb436dc8a70b7f5923f4b7af7fa159.

Also, how do I incorporate your commits?

Be sure you're on your own branch locally first, then do:

git fetch https://github.com/luke-jr/bitcoin rpc_maxmempool && git reset --hard FETCH_HEAD

Before pushing back to GitHub, have someone re-open this PR.

Note that my branch does not make the suggested changes for a more generic config-changing RPC.

(or is it possible to transfer this pull request to you?)

Rather not, I already have many open right now. :/

luke-jr referenced this in commit 8f9df628e7 on May 21, 2022

DrahtBot locked this on Nov 2, 2022