mempool: non validator nodes should not have to run the mempool

[tac0turtle]

Summary

the new app-side mempool handles ordering of txs, but only needs to be run by validators, Other full nodes have no reason to order transactions.

Proposal

On startup, check if a node is a validator, if so it should handle ordering of txs, if not then there is no need to start the mempool.

There may be implications to this like some applications want all nodes to order. There could be others I just cant think of them now

[kocubinski]

In this case the node would just accept the transactions proposed by Tendermint in PrepareProposal ?

[tac0turtle]

a non validating node doesn't vote so they don't care what's in the block and have no need to check.

[alexanderbez]

Let me play the other side of the coin here...shouldn't this be Tendermint's responsibility? IIRC, Tendermint knows if a node is a validator or not. Thus if it knows its not a validator, it will never bother calling PrepareProposal

[tac0turtle]

that is true, it would be the cleaner approach. I can bring it up on the tendermint side. The question arises is what if an application wants everyone to run it?

[yihuang]

I know some user run some customized full nodes just to monitor mempool, mainly quant bots ;D

[sergio-mena]

Thus if it knows its not a validator, it will never bother calling PrepareProposal

This is already the case. The proposer selection algorithm will never select a node that is not in the validator set. So non-validating full nodes will never call PrepareProposal

[prettymuchbryce]

If we're just talking about the app-side mempool (and not the Tendermint mempool) then I think what is being proposed here makes sense. Non-validating nodes will never call PrepareProposal, so they will never call Select to reap transactions from the app-side mempool anyways.

However, if we're talking about the Tendermint mempool, then I would be against disabling it for full nodes with no recourse. As @yihuang above said, there are full nodes that exist to monitor the mempool, index transactions that are received, etc. In our case at dYdX we have done exactly this by instrumenting our full nodes to stream transactions received in CheckTx to an external system for indexing. If you decided to disable it by default, then as long as we had some way to enable the Tendermint mempool for full nodes via the configuration then I think that would be fine.

[tac0turtle]

100% this is only for sdk app mempool. tendermint mempool should live on as it currently is.

[alexanderbez]

So looks like this issue is moot? Tendermint will never call PrepareProposal.

However, it will call CheckTx which will still insert txs into the app-mempool! So essentially for non-validator nodes, we're using a data structure and inserting into it, without actually using it. So the issue does still persist.

I think what we can do is allow the operator to configure to disable/enable mempool setting (set by default) in app.toml.

[tac0turtle]

I think what we can do is allow the operator to configure to disable/enable mempool setting (set by default) in app.toml.

yea this works. its a simple change

[alexanderbez]

Concrete proposal:

Allow operators to enable/disable mempool from being set (default: enabled). For validator nodes, panic when PrepareProposal is called and mempool is disabled. This requires we add mempool != nil checks.

[tac0turtle]

we added a size constraint for the mempool, setting it to 0 should recreate the same functionality, are we fine with that?

[julienrbrt]

we added a size constraint for the mempool, setting it to 0 should recreate the same functionality, are we fine with that?

We should definitely mention the configuration to change for non validator nodes somewhere here as a :::tip: https://docs.cosmos.network/main/run-node/run-node
I think it's setting to -1 btw.

[alexanderbez]

Yeah, since we cannot control what type of mempool is used via configuration (an app would have to explicitly support this themselves), using the size config seems like the sensible and easiest option.

[julienrbrt]

Yeah, since we cannot control what type of mempool is used via configuration (an app would have to explicitly support this themselves), using the size config seems like the sensible and easiest option.

We could enforce it by putting that check in our default baseapp options, so it would work for all type of mempool then.