Remove valset/data commitment confirm logic from state machine

[rach-id]

Remove valset/data commitment confirm logic from state machine in preparation for merging the branch and implementing ADR 5: Reduce QGB state usage

[codecov-commenter]

Codecov Report

Merging #720 (cb5b85e) into qgb-integration (fd7c742) will increase coverage by 1.51%.
The diff coverage is 1.20%.

@@                 Coverage Diff                 @@
##           qgb-integration     #720      +/-   ##
===================================================
+ Coverage            11.09%   12.61%   +1.51%     
===================================================
  Files                   50       45       -5     
  Lines                 8563     6446    -2117     
===================================================
- Hits                   950      813     -137     
+ Misses                7518     5548    -1970     
+ Partials                95       85      -10     

Impacted Files	Coverage Δ
x/qgb/keeper/msg_server.go	0.00% <ø> (ø)
x/qgb/module.go	3.57% <0.00%> (ø)
x/qgb/types/data_commitment_confirm.go	0.00% <ø> (-31.92%)	⬇️
x/qgb/types/keys.go	0.00% <ø> (ø)
x/qgb/types/query.pb.go	0.86% <0.00%> (+0.13%)	⬆️
x/qgb/types/query.pb.gw.go	0.00% <ø> (ø)
x/qgb/types/valset_confirm.go	0.00% <ø> (ø)
x/qgb/keeper/keeper.go	57.69% <100.00%> (-16.73%)	⬇️
x/qgb/handler.go	0.00% <0.00%> (-80.00%)	⬇️
... and 1 more

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

[rootulp]

Approved w/ a question

[rootulp]

[question] this version number seems odd, is it intended?

[rach-id]

it's certainly not. I am still investigating it

[rach-id]

this is probably coming from the conflicting cosmos-sdk and celestia-core versions.
I guess it will be fixed when we merge the cosmos-sdk QGB branch to the default one and we rebase on master.
Will merge for now and take a look on it later

[evan-forbes]

overall this is definitely headed in the correct direction, as it removes the state storing and querying, but I we might have removed a bit too much. Left some comments/questions

we still need to make sure we can create and submit the txs, and we might even want to emit an event (maybe not, this depends on the relayer design and if things like block explorers want to keep track of these things). (we could also potentially do these things in a different PR, but will leave it up to the author)

The only thing that needs removing is storing in the state

[rach-id]

@evan-forbes

we still need to make sure we can create and submit the txs, and we might even want to emit an event (maybe not, this depends on the relayer design and if things like block explorers want to keep track of these things). (we could also potentially do these things in a different PR, but will leave it up to the author)

Orchestrators and relayers will be parsing the blocks. So, no need for events.
And for block explorer, I guess since we're not adding any checks on the submitted confirms, even if an explorer shows that 100 confirm was published, they might all be invalid.
Thus, I don't see a need for events.

However, I am happy to re-introduce and not delete until later. Let me know what you think

[evan-forbes]

currently blocked by celestiaorg/cosmos-sdk#276 which is blocked by celestiaorg/cosmos-sdk#279

[evan-forbes]

And for block explorer, I guess since we're not adding any checks on the submitted confirms, even if an explorer shows that 100 confirm was published, they might all be invalid.

I think you're right, since it would require running something that looks like a relayer (but doesn't actually relay) to know for sure, we can delete them.

Presumably tho, given that delegators can get slashed if a validator is not signing properly, they will be interested in that info. We should definitely want to make it easy to monitor, so as long as we have some mechanism for that. (perhaps a third node, relayer, orchestrator, and a monitor)

[rach-id]

I don't see another way of monitoring besides parsing the blocks and checking if confirms are correct. And if we're parsing blocks, events won't be useful.
Anyway, I defer to you for the decision. I'm fine with both.

[evan-forbes]

I don't see another way of monitoring besides parsing the blocks and checking if confirms are correct. And if we're parsing blocks, events won't be useful.
Anyway, I defer to you for the decision. I'm fine with both.

yeah, same, I think you're right and we can delete them. We just have to keep this fact in mind and provide a way for people to monitor them is all

[rach-id]

I will create an issue for this 👍

[evan-forbes]

currently blocked by celestiaorg/cosmos-sdk#276 which is blocked by celestiaorg/cosmos-sdk#279

ahh sorry, I was wrong on this, thought this was for merging the qgb branch into main lolol 🤦