Skip to content

fix: ValidateUpdate: allow no-change updates regardless of current height (backport #2112)#2115

Merged
sergio-mena merged 1 commit intov1.xfrom
mergify/bp/v1.x/pr-2112
Jan 24, 2024
Merged

fix: ValidateUpdate: allow no-change updates regardless of current height (backport #2112)#2115
sergio-mena merged 1 commit intov1.xfrom
mergify/bp/v1.x/pr-2112

Conversation

@mergify
Copy link
Contributor

@mergify mergify bot commented Jan 24, 2024

This is an automatic backport of pull request #2112 done by Mergify.


Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

…height (#2112)

Closes #2109

In `v0.38.2`, when the App was setting `VoteExtensionsEnableHeight` to a
value that was the same as the current one, even if is useless as a
ConsensusParams update, we were accepting it because it doesn't do any
harm.

When we released `v0.38.3` we refactored function `ValidateUpdate` and
we started rejecting those (useless but harmless) cases.

Since `v0.38.3` is a point release, we shouldn't add restrictions on
inputs to a function without a good reason for it. This PR is adding a
rule to `ValidateUpdate` so re-allow all cases where the app is not
really updating the value of `VoteExtensionsEnableHeight`, regardless of
which height we are currently in.

### Testing done

Apart form CI tasks, we carried out the following extra (manual)
activities:
* We reproduced the error reported (#2109) using our e2e test infra, by
configuring the manifest to set a vote extension to a height in the
past, which is the same height that was previously passed via the
genesis file
* [THE FIX] In the production code, we introduced an early rule in
function `ValidateUpdate` to return no error if the value to update is
equal to the current value
* We also updated two unit tests that should not be returning an error
now (and that were returning an error in `v0.38.3`)
* We re-ran the e2e tests as explained above with the new fix, and the
problem reported in #2109 no longer shows up
* Then, to make sure no other extra restrictions were introduced in
`v0.38.3`, we did the following:
* We temporarily replaced the implementation of `ValidateUpdate` with
the one existing before `v0.38.3` (these temporary changes are not
included in the PR, it was just done for testing pruposes)
* We ran the new exhaustive unit tests introduced in `v0.38.3` and
modified in this PR against the replaced (`v0.38.3`) implementation
* We went through all the UTs that are now failing in
`TestConsensusParamsUpdate_VoteExtensionsEnableHeight` (getting an
unexpected error, or not getting an expected error), to make sure they
don't represent a set of inputs to `ValidateUpdate` that were accepted
in v0.38.2 but no longer accepted with the current version (the one in
this PR)
  * We found no problematic test case

---

#### PR checklist

- [x] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Greg Szabo <16846635+greg-szabo@users.noreply.github.com>
Co-authored-by: Greg Szabo <greg@philosobear.com>
(cherry picked from commit 1d99e05)
@mergify mergify bot requested review from a team as code owners January 24, 2024 14:33
@mergify mergify bot requested a review from a team January 24, 2024 14:33
@sergio-mena sergio-mena merged commit 421e7c2 into v1.x Jan 24, 2024
@sergio-mena sergio-mena deleted the mergify/bp/v1.x/pr-2112 branch January 24, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants