feat!: override default mempool params

[rootulp]

Supersedes celestiaorg/celestia-core#885

Closes celestiaorg/celestia-core#812
Closes celestiaorg/celestia-core#867
Closes #590

[codecov-commenter]

Codecov Report

Merging #1008 (b75ee0c) into main (f67acce) will increase coverage by 23.93%.
The diff coverage is 44.44%.

@@             Coverage Diff             @@
##             main    #1008       +/-   ##
===========================================
+ Coverage   27.15%   51.09%   +23.93%     
===========================================
  Files          81       71       -10     
  Lines        9021     4378     -4643     
===========================================
- Hits         2450     2237      -213     
+ Misses       6335     1915     -4420     
+ Partials      236      226       -10     

Impacted Files	Coverage Δ
x/blob/keeper/keeper.go	91.30% <ø> (ø)
x/blob/types/params.go	33.33% <28.57%> (-3.71%)	⬇️
x/blob/keeper/params.go	100.00% <100.00%> (ø)
x/blob/types/params.pb.go
x/qgb/types/genesis.pb.go
x/qgb/types/query.pb.gw.go
x/blob/types/query.pb.gw.go
x/blob/types/tx.pb.go
x/blob/types/query.pb.go
x/blob/types/genesis.pb.go
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[evan-forbes]

thanks!! tested locally and it works as expected

[evan-forbes]

thanks!! tested this locally and it worked as expected

[rootulp]

One thing I didn't consider until after merging is that it should be possible for validators to override this default param by configuring ~/.celestia-app/config/config.toml. I'll test shortly.

[evan-forbes]

One thing I didn't consider until after merging is that it should be possible for validators to override this default param by configuring

thanks for testing, I didn't test, but I did check it out the code in the sdk and it looked like we should be good https://github.com/celestiaorg/cosmos-sdk/blob/a78f3e1b2d09a853695c1b85da72a37445d7124f/server/util.go#L201-L225

[rootulp]

Tested by adding the following log after this:

	logger.Info("config.Mempool.Version", "config.Mempool.Version", config.Mempool.Version)

and running

rm -rf ~/.celestia-app
./scripts/single-node.sh
cat ~/.celestia-app/config.toml // verified that mempool version is "v1"

the mempool version configured in config.toml is respected so after stopping the node above, overriding the version to "0", and restarting the node, logs indicate that the v0 mempool is in fact being used. E.g.

2:48PM INF config.Mempool.Version config.Mempool.Version=v0

[evan-forbes]

@rootulp was this breaking?

[rootulp]

I think so because configuration changes can be considered breaking changes

[evan-forbes]

there's no harm in leaving it, but I actually think this is one of the few changes that we can make that is not breaking given that it is strictly a default change (anyone can change this at any time) and its part of the mempool (not consensus critical)

[rootulp]

I think it's subject to interpretation but assume we don't mark this change as breaking and a validator upgrades from a celestia-app release before this change to a release after this change. Assume the validator intends to use the FIFO mempool but hasn't overriden any config because the default config previously did what they want. The validator operator upgrades blindly because the release notes don't mark this as a breaking change so they don't expect any visible behavior change. After the upgrade, the validator (contrary to their desire) is using the prioritized mempool.

I see your point that it's not consensus critical and is overridable so I think this change could be interpreted as non-breaking but if there's ambiguity on whether a change should be marked breaking vs. non-breaking, I think it's safer to mark as breaking to explicitly call out to consumers behavior changes that they should be aware of when upgrading. In the future (i.e. post mainnet launch), I think we'll have to be much more careful about releasing / labeling breaking changes and make an effort to minimize the number of such changes.

[evan-forbes]

I think we'll have to be much more careful about releasing / labeling breaking changes and make an effort to minimize the number of such changes.

Yes I agree 🙂. As you and I have discussed in the past, almost everything we do is breaking (except changes to the default config imho 😅).

upgrades from a celestia-app release before this change to a release after this change

Doesn't this require resetting their config? If they only upgrade binaries (most situations) or use their own custom config (all validator setups and most power users) thier mempool will be the same, that's why I don't see this as breaking.

I think it's safer to mark as breaking...

okay let's keep it breaking 🙂.

[rootulp]

Oh very good point, the situation I described would involve resetting their config which wouldn't happen if strictly upgrading binaries.