refactor: Changes to AppModuleGenesis for concurrent modules' genesis states unmarshaling

[catShaark]

Description

Change AppModuleGenesis interface for concurrent modules' genesis states unmarshaling.

Changes

• Adding a new function to ModuleManager for unmarshaling modules' GenesisState
• Adding a new function UnmarshalGenesis to AppModule interface for unmarshaling modules' GenesisState, changing AppModule.InitGenesis params from (sdk.Context, codec.JSONCodec, json.RawMessage) to (sdk.Context, proto.Message) so that AppModule.InitGenesis only does the work of module initialization with the passed in unmarshaled GenesisState.

Closes: #12172

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[alexanderbez]

Excellent work!

[alexanderbez]

There is concurrent access to genesisStates, which might trip off any race detectors. While in practice this should be OK since each goroutine accesses it's own unique key in genesisStates, we might wanna make this thread-safe. Perhaps Manager could have a mutex.

[ValarDragon]

This wasn't safe as written before! As genesisStates is a hashmap, with grow operations =p (You can not safely parallel write to new keys in a hashmap ~ever. Depending on implementation you can update existing keys, or deflate some of the unsafe concurrency in new keys)

The mutex operations were definitely needed for correctness

[tac0turtle]

high level question, how would this differ from genesis streaming? I think part of the bottleneck is we load everything into memory then call init genesis, but if we streamed it as it was coming in, it may be faster than this approach?

[alexanderbez]

It's orthogonal somewhat @marbar3778 -- AFAIK, streaming is still in R&D or even just a rough idea ATM. This PR is just a small win in the context of the current design of "load and init".

[ValarDragon]

This is a breaking change for all API consumers -- not sure how the rollout plan for this one will look. Not really in favor of merging until that has a plan. Also would like to note in weighting the API break, this is good, but really not even 1% of the time in Genesis initialization for large states. Thats really all in DB work. Nor is this a dev UX improvement.

If we really wanted it, would be nice if its something we could gate behind an extension API, or part of app wiring.

But to be honest, if we take a step back, I've always been puzzled as to why is the AppModule even responsible for the unmarshal logic. Why can't module registration communicate the Genesis type (via function or generic), and the global system handles the unmarshalling into the correct type (as done here). This could be done by making DefaultGenesis return the base proto.Message, rather than having to make the module JSON serialize it themself.

So this would look like the following type signatures changing from these befores:

func (AppModuleBasic) DefaultGenesis(cdc codec.JSONCodec) json.RawMessage {}
func (am AppModule) InitGenesis(ctx sdk.Context, cdc codec.JSONCodec, gs json.RawMessage) []abci.ValidatorUpdate {}

to this after

func (AppModuleBasic) DefaultGenesis(cdc codec.JSONCodec) proto.Message {}
func (am AppModule) InitGenesis(ctx sdk.Context, genesisState proto.Message) []abci.ValidatorUpdate {}

The change here would involve less of a migration overhead (deleting code), rather than also writing a second function and shuffling stuff around. This would also carry with it longer term dev UX improvements, rather than making the already bad dev UX worse =p

So if we wanted this, I think we should push for ^^, but still unclear to me if worth the tradeoff.

If we lived in a world with generics with associated types (Rust style), these args could each be of the desired type, but I don't know how to do it in golang.

[catShaark]

should I close this ?