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

fix: Change default pruning options #52

Merged

Conversation

mattverse
Copy link
Member

@mattverse mattverse commented Jan 11, 2022

Description

Current default pruning options keep every 100th height in addition to the latest 362880 heights, which in most cases have no need to keep this much, unless a node wants to run historical queries. This should not be the default case- this PR suggests an override in the default pruning settings as the following:

  • keep-recent = 100000
  • keep-every = 0
  • Interval = 10

Additionally, a significant docs mismatch has been underlying for default pruning options, where keep-every was 100, not 500 as stated.


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

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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)

@mattverse mattverse changed the title Change default pruning options fix: Change default pruning options Jan 11, 2022
@ValarDragon
Copy link
Member

This is kinda crazy , looks like the config message has always been extremely wrong???

@ValarDragon
Copy link
Member

Or are these defaults being changed just not whats happening?

Like if keep-recent was 21days worth of blocks, state synced nodes wouldn't have issues

@mattverse
Copy link
Member Author

mattverse commented Jan 11, 2022

This is kinda crazy , looks like the config message has always been extremely wrong???

Yeah I've noticed. I thought this was different because we already have overwritten default pruning settings at some point, but just noticed that we haven't and it's the same for cosmos sdk as well. I'll ensure that there isn't anywhere else that's handling deafult pruining settings once I get up

@ValarDragon
Copy link
Member

Can we change keep-every to 100k, currently thats the code comment, but code looks like at 10k to me. (Possible I mis-read)

Can we update the docs to point out thats 1 week worth of states.

Can we also make pruning interval 100 blocks. Once we do that, LGTM to merge! Thanks for making this PR!

@ValarDragon
Copy link
Member

for posterity, we confirmed the defaults were changed as this PR suggests, with never updating the config. This honestly explains a lot of the state bloat. The docs mismatch got introduced here: https://github.com/cosmos/cosmos-sdk/pull/9859/files (Thanks @UnityChaos for finding this)

@tac0turtle
Copy link

362880 is meant to be the unbending period which should be for validators in the case of needing to verify evidence, but in 99.9% cases evidence is seen within a block. For now its not an issue, but in the far future it may be.

@ValarDragon
Copy link
Member

ValarDragon commented Jan 12, 2022

If thats the reason, theres an architecture problem. You don't need any old state, you only need the last 362880 block headers. (Which is what min-retain-blocks is for and achieves)

@ValarDragon
Copy link
Member

ValarDragon commented Jan 12, 2022

Dang I said the wrong thing in my comment, so sorry. I intended keep-recent = 100k, keep-every = 0. I'm so sorry, I'll commit the fix and merge

EDIT: Ah perfect, you interpreted what I intended! 😂

@tac0turtle
Copy link

I think based off the comments that was the reasoning, but agree its not needed

@ValarDragon ValarDragon merged commit de1853f into v0.44.3x-osmo-v5 Jan 13, 2022
@ValarDragon ValarDragon deleted the mattverse/change-default-pruning-options branch January 13, 2022 00:20
sunnya97 pushed a commit that referenced this pull request Jan 24, 2022
* Change default pruning settings

* Change config messages

* Change keep-every to 100k

* Updates

Co-authored-by: ValarDragon <dojha12@gmail.com>
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.

3 participants