fix: Use bytes instead of string comparison in delete validator queue

[williamchong]

Description

We had a chain upgrade that introduced bech32 prefix change and dual prefix support in our cosmos-sdk fork, and after ~21 days of running the upgrade, our chain halted due to this issue. Details of our chain halt incident can be found here

The root issue is in x/staking, DeleteValidatorQueue does a string comparison instead of bytes, which would cause panic when dequeueing validator if the address string format is changed (in our case, default bech32 prefix changed).

I am not sure if this should be considered as a useful fix for general case, since changing bech32 prefix and dual prefix support seems not common. I am also not sure how to write a test without introducing our own dual prefix support feature.

Feel free to comment how would you like to handle this PR.

---

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]

I think this change is totally acceptable. Thanks for contributing @williamchong :)

Can you please add a quick changelog entry under "improvements" or maybe it should go under "bug fixes".

[alexanderbez]

@williamchong can you rebase the PR please or allow the core team to do it?

[williamchong]

@williamchong can you rebase the PR please or allow the core team to do it?

sure and rebased

[alexanderbez]

@marbar3778 how does a contributor allow owners to rebase/update themselves again?

[aaronc]

@marbar3778 how does a contributor allow owners to rebase/update themselves again?

There should be a button for them in the sidebar

[williamchong]

@marbar3778 how does a contributor allow owners to rebase/update themselves again?

There should be a button for them in the sidebar

Can't see the option there, maybe because I am opening this PR from a fork under org account?
Anyway rebased again

[alexanderbez]

It's out of date again haha. Best to figure out how to enable that option in the likecoin org 👍 🙏

[codecov]

Codecov Report

Merging #12303 (733f629) into main (bd31b7c) will decrease coverage by 0.38%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##             main   #12303      +/-   ##
==========================================
- Coverage   66.54%   66.16%   -0.39%     
==========================================
  Files         689      675      -14     
  Lines       72357    71262    -1095     
==========================================
- Hits        48147    47147    -1000     
+ Misses      21544    21473      -71     
+ Partials     2666     2642      -24     

Impacted Files	Coverage Δ
x/staking/keeper/validator.go	86.39% <54.54%> (-1.33%)	⬇️
depinject/errors.go
depinject/simple.go
depinject/module_dep.go
depinject/struct_args.go
depinject/location.go
depinject/group.go
depinject/container.go
depinject/supply.go
depinject/config.go
... and 5 more