feat: allow governance to update the TrustingPeriod of the 07-tendermint light client

[charleenfei]

Description

closes: #1398

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[codecov-commenter]

Codecov Report

Merging #1713 (647431d) into main (c809ce3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1713   +/-   ##
=======================================
  Coverage   80.05%   80.06%           
=======================================
  Files         164      164           
  Lines       12300    12305    +5     
=======================================
+ Hits         9847     9852    +5     
  Misses       1993     1993           
  Partials      460      460           

Impacted Files	Coverage Δ
...ght-clients/07-tendermint/types/proposal_handle.go	80.00% <100.00%> (+1.81%)	⬆️

[crodriguezvega]

Thanks, @charleenfei!

I think we need to update this text in docs/ibc/upgrades/developer-guide.md, right?

... while ensuring that the relayer submitting the UpgradeClientMsg cannot alter the client-chosen parameters that the users are relying upon (examples in Tendermint client include TrustingPeriod, TrustLevel, MaxClockDrift, etc).

[damiannolan]

🚀

[AdityaSripal]

Thanks for the fix @charleenfei ! This works for the immediate issue at hand, but actually i think governance should be able to change all parameters of the client.

Governance basically has sudo privilege on the state machine. so it should be able to do what it wants. It can do what it wants anyway by just explicitly voting to change the state, so we should make things easy for them when we anticipate changes like this.

Though that fix can go in a future PR.

[AdityaSripal]

Suggested change

Note that this should NOT be lightly updated, as there may be a gap in time between when misbehaviour has occured and when the evidence of misbehaviour is submitted. For example, if the `UnbondingPeriod` is 2 weeks and the `TrustingPeriod` has also been set to two weeks, a validator could wait until right before `UnbondingPeriod` finishes, submit false information, then unbond and exit without being slashed for misbehaviour. Therefore, we recommend that the trusting period for the 07-tendermint client be set at a minimum to 2/3 of the `UnbondingPeriod`.

Note that this should NOT be lightly updated, as there may be a gap in time between when misbehaviour has occured and when the evidence of misbehaviour is submitted. For example, if the `UnbondingPeriod` is 2 weeks and the `TrustingPeriod` has also been set to two weeks, a validator could wait until right before `UnbondingPeriod` finishes, submit false information, then unbond and exit without being slashed for misbehaviour. Therefore, we recommend that the trusting period for the 07-tendermint client be set at 2/3 of the `UnbondingPeriod`.

Minimum doesn't make sense here since it is really more of a maximum. It's bad for it to be 999/1000 of the unbonding period for example. But really the relevant point here isn't a ratio of the two, but rather the delta between them.

i.e. UnbondingPeriod - TrustingPeriod is more relevant than TrustingPeriod / UnbondingPeriod

I think its fine to leave 2/3 as sensible default but wouldn't say that people should treat the ratio as being semantically significant

[charleenfei]

makes sense! i'll take it out

[colin-axner]

Post merge ACK