fix bech32 prefix in evidence

[yihuang]

Description

When decoding the consensus address, it should use the dynamic configured prefix, rather than the constant.

Problem:

• when cosmos based chain customized prefix, double sign behaviour don't get jailed.

---

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

[tomtau]

technically, this may be a consensus-breaking change for non-gaia live networks on 0.41 but probably ok to go with that?

[alessio]

Hi @yihuang, and thanks for taking the time the submit this patch and help make Cosmos SDK better.

If the bug is confirmed and it is reproducible in master, we generally accept PRs in master first. Once a PR is fixed in master, we cherry-pick the respective commit and open PRs against the supported stable branches.

Thus, would you mind re-targeting this PR to master please?

[yihuang]

Hi @yihuang, and thanks for taking the time the submit this patch and help make Cosmos SDK better.

If the bug is confirmed and it is reproducible in master, we generally accept PRs in master first. Once a PR is fixed in master, we cherry-pick the respective commit and open PRs against the supported stable branches.

Thus, would you mind re-targeting this PR to master please?

Done.

I didn't confirmed in master, because master needs some extra changes for our tests to run. But the patch should be good in master too.

[codecov]

Codecov Report

Merging #8461 (4f55c93) into master (784a9a6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8461      +/-   ##
==========================================
+ Coverage   61.47%   61.49%   +0.01%     
==========================================
  Files         630      630              
  Lines       36398    36399       +1     
==========================================
+ Hits        22377    22384       +7     
+ Misses      11671    11664       -7     
- Partials     2350     2351       +1     

Impacted Files	Coverage Δ
x/evidence/types/evidence.go	87.50% <100.00%> (+19.75%)	⬆️

[robert-zaremba]

I confirm, this indeed causes problem, and the changeset is a proper fix.

[robert-zaremba]

@yihuang - thanks for catching this and contributing back to the SDK.

@alessio , indeed , the problem is in the following lines:

evidence := types.FromABCIEvidence(tmEvidence)
k.HandleEquivocationEvidence(ctx, evidence.(*types.Equivocation))

Above, the evidence doesn't have a prefix from config, however, when decoding, in Equivocation.GetConsensusAddress() sdk.ConsAddress, we use config to get the right prefix.

Another fix would be to use []byte instead of string for Equivocation.ConsensusAddress. This way we would avoid bech32 roundtrip at all. Probably we would need to add one more type for "presentation layer".

I approved this PR, because it solves the issue, we can think if we would prefer to alternative solution I mentioned above (I slightly prefer a solution with two types: one for model / operations, and one for presentation) - it's more bulletproof, we avoid bugs with wrong cons/val etc... prefixes.

Finally we have to add a test - ideally in this PR, but I'm OK to do it in a new PR. The test should be more or less following:

evidence := types.FromABCIEvidence(tmEvidence)
consAddr := evidence.GetConsensusAddress()
/// now we do a check:
assert.EqualValues(e.Validator.Address, consAddr)

[alessio]

Finally we have to add a test - ideally in this PR, but I'm OK to do it in a new PR

@yihuang feel like adding a test by chance?

[yihuang]

unit test added, manually confirmed that without the change, the test fails.

[alessio]

@tomtau dixit:

technically, this may be a consensus-breaking change for non-gaia live networks on 0.41 but probably ok to go with that?

This would go right away into v0.42 I suppose

[alessio]

@yihuang last bit: can you add a changelog entry please?

@tomtau I was thinking again about your comment, and yes, this is potentially breaking for v0.41 networks, except the Cosmos Hub.

[robert-zaremba]

I was thinking again about your comment, and yes, this is potentially breaking for v0.41 networks, except the Cosmos Hub.

Breaking change term is a ambiguous. This change is backward compatible, but creates valid state changes which were not valid before. So it's a soft fork.

[alessio]

Yep, the change is potentially state breaking

[yihuang]

I was thinking again about your comment, and yes, this is potentially breaking for v0.41 networks, except the Cosmos Hub.

Breaking change term is a ambiguous. This change is backward compatible, but creates valid state changes which were not valid before. So it's a soft fork.

Isn't it a hard fork? old version won't jail the faulting validator, new version will, they won't agree on the state ever after?

[yihuang]

@yihuang last bit: can you add a changelog entry please?

Done

[tac0turtle]

@alessio & @robert-zaremba was this backported?

[robert-zaremba]

@marbar3778 - this is scheduled f the upcoming release.

[robert-zaremba]

For the record:

• Based on the current release schedule, this won't go into 0.41.x line because it's a state machine breaking change
• This doesn't affect Gaia - so we don't need this patch for Gaia update
• However, 0.41.x is buggy for anyone else and we need to communicate clearly that nobody except Gaia should use 0.41.x (nor 0.40.x) and should wait for 0.42.x.

[robert-zaremba]

cc: @clevinson , @zmanian - check above.

[amaury1093]

@Mergifyio backport release/v0.42.x

[mergify]

Command backport release/v0.42.x: success

Backports have been created

• #8787 fix bech32 prefix in evidence (bp #8461) has been created for branch release/v0.42.x