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

Prevent over pruning option #624

Closed
wants to merge 3 commits into from
Closed

Conversation

chillyvee
Copy link
Contributor

@chillyvee chillyvee commented Nov 21, 2022

Recently, some chains suddenly added a new store to rootmulti. (it may not have been intentional, but just callingosmos-sdk's reference baseapp implementation)

The new store (icahost) has the wrong height.

While other stores like bank and slashing have the current block #123456789

icahost has a height equal to the number of blocks since an upgrade/start such as 105 for example

Is it reasonable to find some way to warn rather than panic? This will allow validators to continue to prune.

Then teams can see the warnings and know they need to implement some upgrade handlers to address the situation.

As cosmos-sdk grows, new stores of the wrong height may become more common and difficult for most teams to fix in a short amount of time.

Here is an example of how to not immediately crash and we could enable it with an environment variable.

What do you think?

As a note, here are links to 3 chains with issues: dig, konstellation, comdex
cosmos/cosmos-sdk#13933

@tac0turtle
Copy link
Member

sorry for long delay in reviewing this

@tac0turtle
Copy link
Member

@facundomedica @cool-develope can we get a review on this

nodedb.go Outdated Show resolved Hide resolved
@tac0turtle
Copy link
Member

im worried this change will have adverse side effects. instead maybe we handle the error returned currently higher up the stack. Have you looked into this?

@chillyvee
Copy link
Contributor Author

Could handle at a higher layer. Is there a typical way we should return a typed error so the layer above can easily know to not panic (besides string message compare)?

@tac0turtle
Copy link
Member

closing this as it is handled at a higher level

@tac0turtle tac0turtle closed this May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants