Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Epoch hooks, panic recovery #1308

Merged
merged 5 commits into from
Apr 21, 2022
Merged

Epoch hooks, panic recovery #1308

merged 5 commits into from
Apr 21, 2022

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Apr 21, 2022

What is the purpose of the change

This PR aims to mitigate the risk of a panic within the epoch logic causing a chain halt (or even a halt of subsequent epoch actions). A subcomponent of #1305

Brief change log

  • Add panic recovery to the epoch hooks
  • Improve the panic recovery -> string printing mechanism

Testing and Verifying

  • This adds a test case to ensure that: Panics are actually caught in an x/epochs/hooks_test.go

We still need to test that this does not notably increase the epoch time. (Due to cosmos/cosmos-sdk#10310 )

I want to understand that issue better, but it may be that the best way to test it is to get this cherry-picked on a v7 branch and just run it.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? specification (x/<module>/spec/), this still needs to be done

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2022

Codecov Report

Merging #1308 (44f0c83) into main (0b3e383) will decrease coverage by 0.45%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1308      +/-   ##
==========================================
- Coverage   20.26%   19.81%   -0.46%     
==========================================
  Files         203      210       +7     
  Lines       26824    27884    +1060     
==========================================
+ Hits         5436     5525      +89     
- Misses      20378    21333     +955     
- Partials     1010     1026      +16     
Impacted Files Coverage Δ
x/epochs/types/hooks.go 100.00% <100.00%> (ø)
x/epochs/types/genesis.pb.go 11.13% <0.00%> (ø)
x/epochs/types/genesis.go 67.85% <0.00%> (ø)
x/epochs/types/identifier.go 33.33% <0.00%> (ø)
x/epochs/types/keys.go 0.00% <0.00%> (ø)
x/epochs/types/query.pb.gw.go 0.00% <0.00%> (ø)
x/epochs/types/query.pb.go 1.49% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b3e383...44f0c83. Read the comment docs.

@ValarDragon ValarDragon marked this pull request as ready for review April 21, 2022 15:13
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

How are we going to handle panics which authors did not mean to be recoverable?

ref: #1305 (comment)

osmoutils/cache_ctx.go Outdated Show resolved Hide resolved
osmoutils/cache_ctx.go Outdated Show resolved Hide resolved
x/epochs/types/hooks_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
osmoutils/cache_ctx.go Outdated Show resolved Hide resolved
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Roman <[email protected]>
@ValarDragon
Copy link
Member Author

How are we going to handle panics which authors did not mean to be recoverable?

Replied in the github issue. I think were right now assuming that none of the epoch code is deliberately chosen to cause a chain halt. (Reward distribution, token minting, superfluid, txfees)

@ValarDragon ValarDragon merged commit 84f0194 into main Apr 21, 2022
@ValarDragon ValarDragon deleted the dev/hook_recovery branch April 21, 2022 21:09
@github-actions github-actions bot mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants