feat: update the slashing and evidence modules to work with ICS #15908

sainoe · 2023-04-21T12:14:01Z

Description

The ICS consumer module doesn't store validators' pubkeys in the slashing module making the slashing and evidence modules either panicing or no-oping when these keys aren't stored see:

cosmos-sdk/x/slashing/keeper/infractions.go

Line 19 in 9acdbb9

if _, err := k.GetPubkey(ctx, addr); err != nil {

cosmos-sdk/x/evidence/keeper/infraction.go

Line 30 in 9acdbb9

if _, err := k.slashingKeeper.GetPubkey(ctx, consAddr.Bytes()); err != nil {

This PR removes these two checks in order to work with ICS.

Note that this line was previously added to the evidence module by #13122 for the same reason.

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...

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)

julienrbrt · 2023-04-21T12:58:21Z

x/evidence/keeper/infraction.go

-		// constraints and ignore evidence that cannot be handled.
-		return
-	}
-


If we do not panic l75, can we at least log something?

Why don't we move the validator and pubkey check above this and we can short-circuit everything else?

Perhaps we can replicate this no-op in the slashing infractions.go ?

yarn.lock

x/slashing/keeper/infractions.go

faddat · 2023-04-24T16:11:15Z

There's a really disturbing pattern of behavior coming out of informal systems, where they just gank stuff from us.

They do not answer our questions in any forum, they are not helpful, and they are holding the hub back.

faddat · 2023-04-25T03:43:15Z

Btw this PR is fine.

There may be more needed.

It is certainly a result of notionals work.

That should be recognized so I don't need to prove it because this is very tedious.

#15856 informal told us it was not needed.

vuong177 · 2023-04-25T07:01:35Z

We have same change in this https://github.com/notional-labs/cosmos-sdk/tree/sdkv47-ics

x/slashing/keeper/infractions.go

@@ -18,9 +18,6 @@ func (k Keeper) HandleValidatorSignature(ctx sdk.Context, addr cryptotypes.Addre

 	// fetch the validator public key
 	consAddr := sdk.ConsAddress(addr)
-	if _, err := k.GetPubkey(ctx, addr); err != nil {


julienrbrt · 2023-04-25T11:30:37Z

Could you add a changelog?

julienrbrt · 2023-04-25T15:45:00Z

Just adding a flag because we want to add an extra commit here.

Co-authored-by: Jacob Gadikian <faddat@users.noreply.github.com>

Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: Jacob Gadikian <faddat@users.noreply.github.com> (cherry picked from commit 97e4978) # Conflicts: # CHANGELOG.md # x/evidence/keeper/infraction.go

…port #15908) (#15947)

Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: Jacob Gadikian <faddat@users.noreply.github.com>

…port cosmos#15908) (cosmos#15947)

sainoe added 3 commits April 17, 2023 16:06

remove dead code in evidence module

f894943

remove validators pubkey condition in slashing module

2f94f8d

remove validators pubkey condition in evidence module

0e93916

sainoe requested a review from a team as a code owner April 21, 2023 12:14

github-actions bot added C:x/evidence C:x/slashing labels Apr 21, 2023

This comment has been minimized.

Sign in to view

julienrbrt reviewed Apr 21, 2023

View reviewed changes

sainoe added 3 commits April 21, 2023 15:46

remove yarn.lock

1e6aed6

add logger in evidence

0475025

remove test files

8a21bf6

github-actions bot added the C:orm label Apr 21, 2023

remove test files

f412172

julienrbrt reviewed Apr 21, 2023

View reviewed changes

x/slashing/keeper/infractions.go Outdated Show resolved Hide resolved

revert unwanted

758d63a

julienrbrt added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Apr 21, 2023

move up validator in pubkey checks in equivocation handler

0af4d35

julienrbrt mentioned this pull request Apr 24, 2023

fix: changes to sdk for interchain security #15917

Closed

19 tasks

This comment was marked as off-topic.

Sign in to view

faddat reviewed Apr 25, 2023

View reviewed changes

julienrbrt approved these changes Apr 25, 2023

View reviewed changes

alexanderbez approved these changes Apr 25, 2023

View reviewed changes

update changelog

bebfdb7

julienrbrt assigned alexanderbez and julienrbrt Apr 25, 2023

julienrbrt added the S: DO NOT MERGE Status: DO NOT MERGE label Apr 25, 2023

julienrbrt added S: DO NOT MERGE Status: DO NOT MERGE and removed S: DO NOT MERGE Status: DO NOT MERGE C:orm labels Apr 25, 2023

julienrbrt force-pushed the sainoe/ics-changes branch from 9a48333 to d553fff Compare April 25, 2023 15:59

collab

c70b907

Co-authored-by: Jacob Gadikian <faddat@users.noreply.github.com>

julienrbrt force-pushed the sainoe/ics-changes branch from d553fff to c70b907 Compare April 25, 2023 16:03

Merge branch 'main' into sainoe/ics-changes

90bafef

julienrbrt removed the S: DO NOT MERGE Status: DO NOT MERGE label Apr 25, 2023

julienrbrt enabled auto-merge April 25, 2023 16:22

julienrbrt added this pull request to the merge queue Apr 25, 2023

Merged via the queue into main with commit 97e4978 Apr 25, 2023

julienrbrt deleted the sainoe/ics-changes branch April 25, 2023 16:37

mergify bot mentioned this pull request Apr 25, 2023

feat: update the slashing and evidence modules to work with ICS (backport #15908) #15947

Merged

julienrbrt pushed a commit that referenced this pull request Apr 25, 2023

feat: update the slashing and evidence modules to work with ICS (back…

6b30073

…port #15908) (#15947)

cool-develope pushed a commit that referenced this pull request Apr 26, 2023

feat: update the slashing and evidence modules to work with ICS (#15908)

4f27904

Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: Jacob Gadikian <faddat@users.noreply.github.com>

roy-dydx pushed a commit to dydxprotocol/cosmos-sdk that referenced this pull request Jul 11, 2023

feat: update the slashing and evidence modules to work with ICS (back…

08ec7ae

…port cosmos#15908) (cosmos#15947)

0xstepit mentioned this pull request Jul 1, 2024

chore(cachemulti): add Copy method evmos/cosmos-sdk#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update the slashing and evidence modules to work with ICS #15908

feat: update the slashing and evidence modules to work with ICS #15908

sainoe commented Apr 21, 2023

This comment has been minimized.

julienrbrt Apr 21, 2023

sainoe Apr 21, 2023

alexanderbez Apr 21, 2023

sainoe Apr 24, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as duplicate.

This comment was marked as off-topic.

faddat commented Apr 24, 2023 •

edited

Loading

This comment was marked as off-topic.

faddat commented Apr 25, 2023 •

edited

Loading

vuong177 commented Apr 25, 2023

This comment was marked as off-topic.

julienrbrt commented Apr 25, 2023

julienrbrt commented Apr 25, 2023

feat: update the slashing and evidence modules to work with ICS #15908

feat: update the slashing and evidence modules to work with ICS #15908

Conversation

sainoe commented Apr 21, 2023

Description

Author Checklist

Reviewers Checklist

This comment has been minimized.

julienrbrt Apr 21, 2023

Choose a reason for hiding this comment

sainoe Apr 21, 2023

Choose a reason for hiding this comment

alexanderbez Apr 21, 2023

Choose a reason for hiding this comment

sainoe Apr 24, 2023

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as duplicate.

This comment was marked as off-topic.

faddat commented Apr 24, 2023 • edited Loading

This comment was marked as off-topic.

faddat commented Apr 25, 2023 • edited Loading

vuong177 commented Apr 25, 2023

This comment was marked as off-topic.

julienrbrt commented Apr 25, 2023

julienrbrt commented Apr 25, 2023

faddat commented Apr 24, 2023 •

edited

Loading

faddat commented Apr 25, 2023 •

edited

Loading