Baseapp Params Store + Import/Export

[rigelrozanski]

Within #2795
we introduce the need for baseapp to persist information (*consensusParams in this case)
There is currently no easy way to store this information - we have access to the main store so in this PR we just create a unique record in the main store and call it directly.

A better solution involves initializing the params store within baseapp (which is typically initialized at the Gaia level right now) - and further pass back the params store to baseapp.

Unfortunately this then also necessities that the Import/Export functionality move through baseapp as well (see reference conversation below)

CC @jaekwon

[rigelrozanski]

Reference conversation (CC @cwgoes cause you asked earlier!)

This is all the thinking - maybe read from back to front?

jaekwon 
hi rigel
instead of genDocProvider I think we want to use the consensus param.
https://github.com/tendermint/tendermint/blob/master/abci/types/types.proto#L220
rige 
so like pass back the ConsensusParams object? (edited)
jaekwon 
yeah during init chain, and stored somewhere in the main store as a parameter or something.
then, the app itself can in the future update it via endblock.. but we don’t need that for now.
for now just need to stash that param from initchain somewhere and read it every begin block.
or something like that…
i dunno if we have a way of caching that across blocks but… that’s the naive way.
rige 
yeah not sure if the inter-block cache is ready yet
okay - I can make this update - thanks for this input (edited)
jaekwon 
cool! i’ll keep reading… and lmk so i can take a look.
meanwhile i’ll review other PRs
rige 
YO
jaekwon 
yo
lol.
rige 
okay so this doesn’t actually make sense to me, coding this. Currently we are retrieving the max block gas at genesis and initializing that moreless as a constant for the baseapp object created. If we want to be reading this value in every block we either need to be passing back the genDocProvider which we would read from in each BeginBlock OR we could pass back the config and create the genDocProvider in begin block
jaekwon 
BTW if you need my attention and i don’t respond in time feel free to phone me on slack or something (edited)
we don’t ever need to read from the genesis doc…
jaekwon 
tendermint will provide that via initchain consensus param…
jaekwon 
after that, it’s up to the app to update it and tell tendermint about it via endblock
let me see… i’ll try to figure out where we can stash it…
rige 
Wait where do consensus param updates come in from?
OR do we just receive the pointer - and we have to read from that pointer again… i c (edited)
jaekwon 
initially via initChain, once upon genesis.
then, it’s never passed in again.
mmm
basically we need to store it in the param store.
rige 
lol why not just store the pointer directly in the baseapp object?
maybe my understanding is lacking as to why that wouldn’t work
jaekwon 
cuz when we restart the blockchain node we’ll need to read it again
but initChain doesn’t get called anymore.
so we need to have stashed it in the param store.
rige 
aha makes sense
so we’ll need to create a new params keeper specifically for use in baseapp AFAICT
unless a shortcut directly to use on the store?
jaekwon 
yeah i think we need a main param subspace or something
or at least for the gaia app.
and maybe we can consider refactoring it so it’s available by baseapp by default in the future.
rige 
yeah, but this is lower level than the gaia app is it not? we need this in baseapp
it is
unless you’re suggesting passing in the params keeper _to_ baseapp from gaia (anti-pattern lol) (edited)
but at that point we might as well just initialize it in baseapp and pass it to gaia
jaekwon 
ah hmm
rige 
<OPTION 2>
OR <OPTION 3> just create a designated params store only for baseapp not to be used by gaia (edited)
jaekwon 
thinking….
btw it’s weird to me that export doesn’t export the params keeper.
oh i guess it’s just exported per x/module.
which kinda makes sense too…
rige 
yeah - but… we still need to export/import the contents of the params keeper no-doubt… hope that’s being done lol - but yeah as a separate module (edited)
jaekwon 
seems like maybe for now we don’t need to because all the params live in x/modules.
rige 
what do you mean? several params from distribution, staking,  gov are held in the params keeper (we pass the params keeper into respective keeper generation) (edited)
jaekwon 
yes.
but all within subspaces of the params keeper,
and because export exports params for each x/distribution/staking/gov,
we don’t need to separately export the params keeper.
that’s all i’m saying.  that’s what it appears to me.
    ```genState := NewGenesisState(
        accounts,
        auth.ExportGenesis(ctx, app.feeCollectionKeeper),
        stake.ExportGenesis(ctx, app.stakeKeeper),
        mint.ExportGenesis(ctx, app.mintKeeper),
        distr.ExportGenesis(ctx, app.distrKeeper),
        gov.ExportGenesis(ctx, app.govKeeper),
        slashing.ExportGenesis(ctx, app.slashingKeeper),
    )```
rige 
cool - oh I see how this makes baseapp persistence even more fun - it needs to have a base level export function too
jaekwon 
what’s the “mainKey” baseapp kvstore used for?
or keyMain in cmd/gaia/app/app.go
(same thing)
rige 
oh you’re totally correct about the ExportGenesis - all logic to get the correct info from the param keeper is done in the modules… no need to export separately - i see
I think that the mainKey is not being used for anything? There has been lots of discussion around removing it in the past
however it’s used in `LoadLatestVersion` - not familiar with what that is doing
jaekwon 
not doing much.
rige 
crazy talk: why is there no special Init for when a chain is restart?
jaekwon 
I think we want to create a separate subspace in ParamKeeper, for storing “TendermintParams”, which is abci.ConsensusParams.
yeah, I think it could go either way… Tendermint could re-send the app the latest consensus params.. or the app can just remember it and load it itself…
we’d need to add a new abci message or change the meaning of initchain… probably create a new abci message for it.
rige 
maybe not what we need ATM - but worth considering that message in the future
jaekwon 
yeah.
rige 
k so - we can totally create a sub-space in the params keeper - it just means we’ll want to initialize the params keeper ideally in baseapp - and further send it to gaia. Additionally, we’ll need baseapp export/import, which I believe is all in gaia atm
jaekwon 
I think we can initialize it all in gaiad, and just set baseapp.SetBlockGasLimit()?
rige 
nm gaia doesn’t need the key - we could just pass the keeper up and it would work
I want to avoid as much as possible initializing things in gaia which baseapp needs - this circular definition stuff makes the code goop-like
jaekwon 
mmhmm… thinking…
rige 
I mean it shouldn’t be a problem, we just need some more stuff in baseapp - this seems like a good thing to have for potentially other params required at the baseapp level
As in similar to how we pass the TxDecoder to baseapp, we’d just now pass the importer/exporter (edited)
jaekwon 
ok so… i don’t think we even need to use paramkeeper for this?
just stash it in the mainstore…
rige 
wuz scared u would say that
jaekwon 
and have Get/SetConsensusParams() on the baseapp if the app wants to get it or set it.
it’s simple…
isn’t it the right thing to do?
rige 
yeah i mean simple ftw
jaekwon 
we can write a bridge to governance or something so it can be modified from governance in the future for some of these params…
oh
rige 
Yeah well the bridge should actually be to the Tendermint Params not this lil store
jaekwon 
ok so you’re saying that we should have baseapp initialize a paramskeeper?
that way gaia doesn’t have to initialize its own using keyparam etc?
and it comes baked with consensusparams in its own subspace?
rige 
yup
that’s what I was saying
jaekwon 
makes sense to me so far…
oh then…
rige 
the complication is then also we need to pass back the importer/exporter back to baseapp so it can tack on the import/export of the consensusparams
BUT I think we need that anyway? - like even if we just stored it directly in the main store - we need this import/export functionality handled by baseapp directly (edited)
jaekwon 
trying to see if we need to modify server.AppExporter method signature from
    ```// AppExporter is a function that dumps all app state to
    // JSON-serializable structure and returns the current validator set.
    AppExporter func(log.Logger, dbm.DB, io.Writer, int64) (json.RawMessage, []tmtypes.GenesisValidator, error)```
I think we want it to also return the tmtypes.ConsensusParams
(which is not the same as abci.ConsensusParams but close enough)
or…
yeah that’s the easiest way for now.
I think.
rige 
lol k - _what_ is easiest for now… what I was proposing or something else? (haven’t grok’d the other option) (edited)
jaekwon 
mmm what I was saying is that….
if we want the export function to properly populate the tendermint genesis.json file’s ConsensusParams section of the GenesisDoc,
… which would be cool, for baseapp’s default paramKeeper’s ConsensusParams subspace to import from during initChain, and export to … (edited)
if we want to go that far, then currently the easiest way is to modify server.AppExporter to return tmtypes.ConsensusParams in addition to tmtypes.GenesisValidator…
which is not the worst thing in the world…
rige 
yes - okay - that makes sense, however it’s kind of an evolution of the first problem
jaekwon 
you mean we’re maybe going too far?
rige 
it certainly doesn’t belong in this PR
but it’s good to set up this PR properly for this update you just mentioned
The first issue which this PR needs to solve is how to correctly _receive_ the consensus params, which is required as a part of the new feature (which is the centrepiece of this PR) - Block Gas Limits (edited)
Then a second PR could be made to make sure the consensus params can be updated by governance and reflected in Tendermint (edited)
Anyways - I think we solved it in the way I explained earlier
jaekwon 
which is?
rige 
Baseapp creates a param-keeper which it passes to gaia. Baseapp creates an importer and exporter constructor methods (which includes import/export consensus params). Baseapp constructs the full importers and exporters using this and an importer/exporter sent in from Gaia (edited)
- this solution is still in line with the requirements of the Second PR I just mentioned btw
jaekwon 
baseapp constructs full importers?  I do’nt get this part.
oh baseapp.ExportGenesis()?
rige 
aka - the “Export” function variable which is passed to `server` is constructed in baseapp not taken from gaia directly (as it is currently) (edited)
jaekwon 
I think we can do simpler…
lets say we do the baseapp param keeper.
and it has consensusparams subspace where it stashes it upon initchian, and loads it upon app construction after LoadLatestVersion().
I don’t think we need to even worry about export of ConsensusParams.
so I think we can leave all export as is.
cuz consensusparams is already in the tmtypes.GenesisDoc anyways,
and maybe we can make the import/export simulation tests ignore this part.
rige 
what do you mean `by and loads it upon app construction after LoadLatestVersion().` I think I’m missing a fundamental of how this info can be reloaded
jaekwon 
so, say that we use the “main” store for params to be stored in, in the baseapp’s paramsKeeper.
rige 
I will mention, I think the separation of import/export between gaia/baseapp is going to be a bit confusing for some. “some imported/exported this way, some this other way” (edited)
jaekwon 
yeah.
I mean, we can work toward better integration for PR2.
I’m just saying for PR1 to get 9002 out.
we can move the paramskeeper from gaiad to baseapp, use the main store instead of keyParams, create the ConsensusParams subspace…
store it there from initChain… and then load it in baseapp.initFromStore()
and not worry about export for PR1.  just make export/import simulation tests pass by making it ignore this part of the main store.
rige 
hmm I think it’s really nbd to have it properly imported/exported, just not necessarily linked with Tendermint updates (which would be PR2) (edited)
jaekwon 
nbd?
rige 
no-big-deal
probably less work than fixing all the tests tbh
jaekwon 
nono
it’s easy…
    ```storeKeysPrefixes := []StoreKeysPrefixes{
        {app.keyMain, newApp.keyMain, [][]byte{}},
        {app.keyAccount, newApp.keyAccount, [][]byte{}},
        {app.keyStake, newApp.keyStake, [][]byte{stake.UnbondingQueueKey, stake.RedelegationQueueKey, stake.ValidatorQueueKey}}, // ordering may change but it doesn't matter
        {app.keySlashing, newApp.keySlashing, [][]byte{}},
        {app.keyMint, newApp.keyMint, [][]byte{}},
        {app.keyDistr, newApp.keyDistr, [][]byte{}},
        {app.keyFeeCollection, newApp.keyFeeCollection, [][]byte{}},
        {app.keyParams, newApp.keyParams, [][]byte{}},
        {app.keyGov, newApp.keyGov, [][]byte{}},
    }```
in cmd/gaia/app/sim_test.go
in the first line we just need to include the prefix where consensus params are stashed.
then import/export test I think will ignore those.
rige 
well it’s going to cause us problems if we want to restart, we pretty much need to have this in by the time we restart the chain
jaekwon 
no
cuz we won’t have any logic to update the block gas limit… and if we wanted to, all we need to do is just update the tmtypes.GenesisDoc.ConsensusParams.BlockGasLimit.
manually in the genesis.json file.  no big deal. (edited)
rige 
uhhh oh if we restart from a genesis file - then you’re correct (edited)
jaekwon 
yes.
oh if you mean restarting…
rige 
nvm hahaha
it’s l8 here
jaekwon 
ok
restarting without export/import is solved via baseapp.initFromStore, which gets called upon LoadLatestVersion().
that function initFromStore would need to be modified to load the consensus params as it was stored from initChain.
anyways, nightnight.
I can actually work on this…
prolly can finish in 2 hours or so
rige 
cool! yeah I need to hit the hay soon - this would be a tomorrow thing for me no-question
jaekwon 
ok.  if i get to it i’ll ping you here.
nighty
rige 
wait
jaekwon 
hrm?
i can voip too if that’s easier
rige 
just to confirm my understanding: you’re saying we would load up the params store in initFromStore and then load consensus params ? ?
jaekwon 
yes.
rige 
but how does initFromStore get the consensus params?
jaekwon 
two scenarios… (1) upon genesis, it won’t exist… so initFromStore will not load anything… but later initChainer will give it to the app, and baseapp can store it then.
(2) upon restart, app initialization calls LoadLatestVersion > initFromStore > loads the paramsKeeper’s consensus-state subspace.
and from (1) (during initChainer) and (2) (during initFromStore) it gets stashed on a baseapp.consensusParams via I guess baseapp.SetConsensusParams(). (edited)
rige 
oh so this is the point in which the updated consensus params are actually set - then they can be accesses from app.BeginBlock later without having to load the store or anything… this makes sense to me (edited)
jaekwon 
yeah
no need to reread it from the store in beginblock or anything…
rige 
GOT IT
thanks - all clear now
haha I can rest in ease
jaekwon 
lol.  cool.
thanks for jamming and being thorough
rige 
of course.
alright I’m off - night (ping me if you end up working on this)
jaekwon 
night!
jaekwon 
i’m not so sure about it… paramsKeeper requiring cdc… prob easier not to use paramsKeeper… oy, this is why maybe paramsKeeper should just be storing simple key/value strings.  or, we don’t pass in a cdc to paramsKeeper and it can’t use interfaces…. maybe that’s the answer. (edited)
going to sleep.  if you want, let me pick it up in the morning, esp if you have other PRs to work on. (edited)

[rigelrozanski]

further relevant conversation happened in this issue #4260 (comment)

CC @alexanderbez @jackzampolin

[alexanderbez]

I haven't read all the details between @jaekwon and @rigelrozanski, but I wondering, since we now have parameter change proposals, would it be ideal to leverage the param store?

What I mean is, every keeper gets its own unique Subspace, so can the BaseApp also get its own Subspace -- under key main, app or whatever? This Subspace would now be a simple interface and be defined in the /types package because all we really need is Get and Set (correct me if I'm wrong). Exporting should be simple too -- it can be done from app.go.

[rigelrozanski]

As long as we are using a params interface receiver on the baseapp side of things then this should be an acceptable design. reviewing the code a bit, I think this idea may work

[jackzampolin]

Is this work we should pick up soon?

[rigelrozanski]

it doesn't seem pressing maybe a 2.0 issue

[tac0turtle]

Consensus param handling was refactored by Bez recently.