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

Tendermint Block Pruning #7265

Merged
merged 29 commits into from
Sep 14, 2020
Merged

Tendermint Block Pruning #7265

merged 29 commits into from
Sep 14, 2020

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Sep 8, 2020

Description

Implement Tendermint block pruning support via ResponseCommit.RetainHeight:

closes: #6164

/cc @marbar3778 @erikgrinaker


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@alexanderbez alexanderbez self-assigned this Sep 8, 2020
@codecov
Copy link

codecov bot commented Sep 8, 2020

Codecov Report

Merging #7265 into master will increase coverage by 0.65%.
The diff coverage is 51.33%.

@@            Coverage Diff             @@
##           master    #7265      +/-   ##
==========================================
+ Coverage   55.60%   56.25%   +0.65%     
==========================================
  Files         457      584     +127     
  Lines       27440    40563   +13123     
==========================================
+ Hits        15257    22820    +7563     
- Misses      11083    15823    +4740     
- Partials     1100     1920     +820     

baseapp/abci.go Outdated Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM pending comments for maintainers

store/mem/store.go Outdated Show resolved Hide resolved
store/rootmulti/dbadapter.go Outdated Show resolved Hide resolved
store/transient/store.go Outdated Show resolved Hide resolved
store/types/store.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Unblocking

Copy link
Contributor

@sahith-narahari sahith-narahari left a comment

Choose a reason for hiding this comment

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

lgtm

baseapp/abci.go Outdated Show resolved Hide resolved
baseapp/abci.go Outdated Show resolved Hide resolved
baseapp/abci.go Outdated

// Define the state pruning interval, i.e. the block interval at which the
// underlying logical database is persisted to disk.
statePruningInterval := int64(app.cms.GetPruning().Interval)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should use KeepEvery, not Interval. Consider the case of KeepEvery: 1000 and Interval: 10. At height 10890, this will allow block pruning up to 10880, even though the last state height persisted to disk is 10000 - if the node restarts, it will try to replay from 10000, but can't since the blocks are gone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right! Can't believe I missed this. Interval is when the heights are actually pruned from disk -- nomenclature always confuses me...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, in this case H=10890, and X=H-KeepEvery=9890 which isn't totally correct either. I think we need to do X=H-(H%KeepEvery)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to do X=H-(H%KeepEvery)?

Yes, that's more accurate. H-KeepEvery would work, but has an excessive safety margin. If you go for minNonZero then take care when H<KeepEvery, since this expression results in 0 and thus would be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

take care when H<KeepEvery, since this expression results in 0 and thus would be ignored.

Correct, in this case, we must be sure to return 0 as to not prune anything because that would be bad!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. @erikgrinaker could you take another look?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!

It occurred to me that another way to handle this would be to simply return a configuration error if minRetainBlocks was set lower than all these other parameters - instead of magically extending it. Not really sure which UX is better.

baseapp/baseapp.go Outdated Show resolved Hide resolved
server/config/config.go Outdated Show resolved Hide resolved
server/config/toml.go Outdated Show resolved Hide resolved
@alexanderbez alexanderbez merged commit 7ae8489 into master Sep 14, 2020
@alexanderbez alexanderbez deleted the bez/6164-tm-block-pruning branch September 14, 2020 14:12
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add block pruning support
5 participants