Valset/Data commitment requests security for QGB

[rach-id]

For Valsets, the orchestrator is currently checking the valset by nonce and then signs it. This works if we suppose that all valset requests are valid.
However, we can have a malicious party that generates invalid valsets (duplicates with different nonce, valsets for non-significant validator set changes etc.), and the orchestrator will keep signing those.
Thus, we will need to add validity checks on valsets before the orchestrator signs them.
These need to be defined not to allow any malicious party to flood the network with incorrect valsets.

The same applies for data commitment requests which will be added here #268.

[rach-id]

@evan-forbes What do you think ?

[evan-forbes]

However, we can have a malicious party that generates invalid valsets (duplicates with different nonce, valsets for non-significant validator set changes etc.), and the orchestrator will keep signing those.

I thought that the only way to set an invalid one is with 2/3's malicious voting power? How would one create one in the state with out 2/3's agreement?

[rach-id]

We're currently setting valsets in end blockers via calling:

celestia-app/x/qgb/keeper/keeper_valset.go

Line 22 in 95f5208

func (k Keeper) SetValsetRequest(ctx sdk.Context) types.Valset {

This function doesn't have any checks in it. And similar with Data commitments.

Thus, if we implement the client requests (maybe not for valsets, but to set data commitments), or similar, under https://github.com/celestiaorg/celestia-app/tree/master/x/qgb/client/cli, we wouldn't have any checks whether the committed state is correct or not.

Furthermore, I created this issue to discuss adding more checks on the keeper level, if it makes sense.

[evan-forbes]

I think its ok to add checks later if we wanted to add cli functionality, but currently we have no plans to, and even if we did, it would only be for data availability commitments.

Since the SetValsetRequest function can only be called during EndBlocker and effects state, anyone attempting to call it inappropriately will result in a different app hash, so I think its safe to not add checks. This was likely the thinking behind not adding checks in the first place.

This is good to think about though!! We might also want to consider adding invariants for this and other things. https://docs.cosmos.network/v0.45/building-modules/invariants.html

[rach-id]

Sounds good. Should we keep the issue open to remind us in the future ?

[evan-forbes]

I think we can close this for now, as there are no direct actionable items. If we ever do variable data commitment windows, then we will only handle this then.