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

state sync: last consensus params height is not set #5889

Merged
merged 5 commits into from Jan 12, 2021

Conversation

cmwaters
Copy link
Contributor

Description

When state syncing, we currently don't save state.LastHeightConsensusParamsChanged. This causes it to be set with a value of 0 when it is saved to the state store. Bootstap() the function that is called ignores this value

if err := store.saveConsensusParamsInfo(height, height, state.ConsensusParams); err != nil {
	return err
}

However, state keeps the 0 value meaning at following heights when state is saved, LastHeightConsensusParamsChanged is 0

if err := store.saveConsensusParamsInfo(nextHeight,
        state.LastHeightConsensusParamsChanged, state.ConsensusParams); err != nil {
	return err
}

This means that the state store isn't saving the consensus params rather referencing the consensus params at height 0 which don't exist and causes this error to arise when pruning:

err="failed to prune state store: couldn't find consensus params at height 0 as last changed from height 11: value retrieved from db is empty"

The second part of this is that we are requesting the consensus params of state.LastBlockHeight + 2 and saving it as state.LastBlockHeight + 1

@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #5889 (09c4e45) into master (956b59a) will decrease coverage by 0.11%.
The diff coverage is 12.50%.

@@            Coverage Diff             @@
##           master    #5889      +/-   ##
==========================================
- Coverage   59.91%   59.79%   -0.12%     
==========================================
  Files         269      269              
  Lines       24550    24552       +2     
==========================================
- Hits        14708    14681      -27     
- Misses       8240     8266      +26     
- Partials     1602     1605       +3     
Impacted Files Coverage Δ
statesync/stateprovider.go 0.00% <0.00%> (ø)
state/store.go 55.89% <50.00%> (+0.16%) ⬆️
crypto/sr25519/pubkey.go 43.47% <0.00%> (-8.70%) ⬇️
consensus/reactor.go 73.83% <0.00%> (-3.65%) ⬇️
libs/autofile/autofile.go 69.35% <0.00%> (-3.23%) ⬇️
statesync/snapshots.go 91.59% <0.00%> (-1.69%) ⬇️
mempool/reactor.go 80.83% <0.00%> (-1.67%) ⬇️
mempool/clist_mempool.go 80.57% <0.00%> (-0.72%) ⬇️
p2p/router.go 70.00% <0.00%> (ø)
proxy/multi_app_conn.go 48.05% <0.00%> (ø)
... and 5 more

@tessr tessr added this to the 0.34.2 milestone Jan 12, 2021
Copy link
Contributor

@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
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@melekes
Copy link
Contributor

melekes commented Jan 12, 2021

changelog entry is missing

Comment on lines +225 to +226
if err := store.saveConsensusParamsInfo(height,
state.LastHeightConsensusParamsChanged, state.ConsensusParams); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the following reads easier IMHO

if err := store.saveConsensusParamsInfo(
  height, state.LastHeightConsensusParamsChanged, state.ConsensusParams,
); err != nil {
  // ...
}

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.

None yet

6 participants