feat(gov): handle panics when executing x/gov proposals #17780

robert-zaremba · 2023-09-18T07:59:36Z

Description

x/gov proposal execution is done in ABCI EndBlock. Proposals contain a custom, potentially untrusted code, which can panic, and putting the whole chain down.

Today, the proposal handler execution is not recoverable, making the chain halt rather than erroring the proposal executing.

In this PR I propose to guard the Msg handler executing in a recoverable block, safely failing the proposal if the panics occurs.

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)

robert-zaremba · 2023-09-18T08:00:41Z

NOTE: I'm adding backport to v0.50, I think it should be backported (v0.50 is not yet released).

tac0turtle

thank you for adding this, the change makes sense same with the backport

julienrbrt · 2023-09-18T08:33:25Z

x/gov/abci.go

+
+	defer func() {
+		if r := recover(); r != nil {
+			err = fmt.Errorf("handling x/gov poposal msg [%s] PANICED: %v", msg, r)


There is another typo I think PANICED -> PANICKED

x/gov/abci.go

odeke-em

Thanks for the PR @robert-zaremba and nice to catch you here after a very long time!

For the commit message and change log entries, please use this

Recover panics and turn them into errors when executing x/gov proposals

Otherwise LGTM. Thank you

x/gov/abci.go

odeke-em · 2023-09-18T10:05:53Z

CHANGELOG.md

@@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/bank) [#17569](https://github.com/cosmos/cosmos-sdk/pull/17569) Introduce a new message type, `MsgBurn `, to burn coins.
 * (genutil) [#17571](https://github.com/cosmos/cosmos-sdk/pull/17571) Allow creation of `AppGenesis` without a file lookup.
 * (server) [#17094](https://github.com/cosmos/cosmos-sdk/pull/17094) Add duration `shutdown-grace` for resource clean up (closing database handles) before exit.
+* (x/gov) [#17780](https://github.com/cosmos/cosmos-sdk/pull/17780) Handle panics when executing x/gov proposals.


Recover panics and turn them into errors when executing x/gov proposals

@odeke-em thanks for suggestion. I will update the changelog. However I don't think this kind of comment should be blocking, because now we need to wait for you to unblock.

robert-zaremba · 2023-09-18T12:46:45Z

I'm thinking more about the backports. What do you think about backporting to 0.47? I think it won't break consensus, because nodes which won't upgrade will halt, without submitting any change, and nodes which will update will continue. So we won't have hash missmatch. Nodes which failed, after update will continue safely.

julienrbrt

lgtm!

julienrbrt · 2023-09-18T13:28:05Z

Could you run make lint-fix?

robert-zaremba · 2023-09-18T14:48:14Z

I'm thinking more about the backports. What do you think about backporting to 0.47? I think it won't break consensus, because nodes which won't upgrade will halt, without submitting any change, and nodes which will update will continue. So we won't have hash missmatch. Nodes which failed, after update will continue safely.

@julienrbrt , @tac0turtle could you 👍 about the proposal to backport to v0.47 as well?

(cherry picked from commit a0b39a1) # Conflicts: # CHANGELOG.md

julienrbrt · 2023-09-18T15:09:16Z

Your reasoning makes sense, this behavior change would not be consensus breaking, unless I am missing something (cc @alexanderbez). So that would make sense to backport to v0.47.

) (#17790) Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Julien Robert <julien@rbrt.fr>

(cherry picked from commit a0b39a1) # Conflicts: # CHANGELOG.md # x/gov/abci.go

) (#17793) Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Julien Robert <julien@rbrt.fr>

…mos#17780) (cosmos#17793) Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Julien Robert <julien@rbrt.fr>

feat(gov): handle panics when executing x/gov proposals

bbcc29f

robert-zaremba requested a review from a team as a code owner September 18, 2023 07:59

robert-zaremba added C:x/gov T:tech debt Tech debt that should be cleaned up backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release labels Sep 18, 2023

github-prbot requested review from a team, facundomedica and samricotta and removed request for a team September 18, 2023 08:01

This comment has been minimized.

Sign in to view

changelog

8f6110d

tac0turtle approved these changes Sep 18, 2023

View reviewed changes

julienrbrt reviewed Sep 18, 2023

View reviewed changes

odeke-em requested changes Sep 18, 2023

View reviewed changes

robert-zaremba added 3 commits September 18, 2023 14:38

TestSafeExecuteHandler

e08fc65

typo

1c1555e

update changelog

08ea4d0

robert-zaremba requested a review from odeke-em September 18, 2023 12:42

Merge branch 'main' into robert/handle-gov-panics

9ca4e86

julienrbrt approved these changes Sep 18, 2023

View reviewed changes

robert-zaremba enabled auto-merge September 18, 2023 12:59

robert-zaremba added 2 commits September 18, 2023 15:52

lint

4369935

Merge branch 'main' into robert/handle-gov-panics

f4f5c89

odeke-em approved these changes Sep 18, 2023

View reviewed changes

robert-zaremba added 2 commits September 18, 2023 16:32

typo

7c074ba

Merge remote-tracking branch 'origin/main' into robert/handle-gov-panics

fa528f0

robert-zaremba added this pull request to the merge queue Sep 18, 2023

Merged via the queue into main with commit a0b39a1 Sep 18, 2023
48 checks passed

robert-zaremba deleted the robert/handle-gov-panics branch September 18, 2023 15:07

mergify bot pushed a commit that referenced this pull request Sep 18, 2023

feat(gov): handle panics when executing x/gov proposals (#17780)

00a55ad

(cherry picked from commit a0b39a1) # Conflicts: # CHANGELOG.md

mergify bot mentioned this pull request Sep 18, 2023

feat(gov): handle panics when executing x/gov proposals (backport #17780) #17790

Merged

tac0turtle pushed a commit that referenced this pull request Sep 18, 2023

feat(gov): handle panics when executing x/gov proposals (backport #17780

87ba5a6

) (#17790) Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Julien Robert <julien@rbrt.fr>

robert-zaremba added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Sep 18, 2023

mergify bot pushed a commit that referenced this pull request Sep 18, 2023

feat(gov): handle panics when executing x/gov proposals (#17780)

43e0ebb

(cherry picked from commit a0b39a1) # Conflicts: # CHANGELOG.md # x/gov/abci.go

mergify bot mentioned this pull request Sep 18, 2023

feat(gov): handle panics when executing x/gov proposals (backport #17780) #17793

Merged

julienrbrt added a commit that referenced this pull request Sep 18, 2023

feat(gov): handle panics when executing x/gov proposals (backport #17780

80705f7

) (#17793) Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Julien Robert <julien@rbrt.fr>

faddat mentioned this pull request Mar 20, 2024

sdk version bump rust-ninja/cosmos-sdk#1

Open

12 tasks

0xstepit mentioned this pull request Jul 1, 2024

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

Merged

faddat mentioned this pull request Nov 8, 2024

faddat/many times #22476

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gov): handle panics when executing x/gov proposals #17780

feat(gov): handle panics when executing x/gov proposals #17780

robert-zaremba commented Sep 18, 2023

robert-zaremba commented Sep 18, 2023

This comment has been minimized.

tac0turtle left a comment

julienrbrt Sep 18, 2023

robert-zaremba Sep 18, 2023

julienrbrt Sep 18, 2023

odeke-em left a comment

odeke-em Sep 18, 2023

robert-zaremba Sep 18, 2023

robert-zaremba commented Sep 18, 2023

julienrbrt left a comment

julienrbrt commented Sep 18, 2023

robert-zaremba commented Sep 18, 2023

julienrbrt commented Sep 18, 2023

feat(gov): handle panics when executing x/gov proposals #17780

feat(gov): handle panics when executing x/gov proposals #17780

Conversation

robert-zaremba commented Sep 18, 2023

Description

Author Checklist

Reviewers Checklist

robert-zaremba commented Sep 18, 2023

This comment has been minimized.

tac0turtle left a comment

Choose a reason for hiding this comment

julienrbrt Sep 18, 2023

Choose a reason for hiding this comment

robert-zaremba Sep 18, 2023

Choose a reason for hiding this comment

julienrbrt Sep 18, 2023

Choose a reason for hiding this comment

odeke-em left a comment

Choose a reason for hiding this comment

odeke-em Sep 18, 2023

Choose a reason for hiding this comment

robert-zaremba Sep 18, 2023

Choose a reason for hiding this comment

robert-zaremba commented Sep 18, 2023

julienrbrt left a comment

Choose a reason for hiding this comment

julienrbrt commented Sep 18, 2023

robert-zaremba commented Sep 18, 2023

julienrbrt commented Sep 18, 2023