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

Mismatch in initial block heights at genesis #648

Closed
brentstone opened this issue Oct 19, 2022 · 6 comments · Fixed by #1993
Closed

Mismatch in initial block heights at genesis #648

brentstone opened this issue Oct 19, 2022 · 6 comments · Fixed by #1993
Assignees
Labels
bug Something isn't working has-a-pr The issue has been solved by a PR that has yet to be merged ledger

Comments

@brentstone
Copy link
Collaborator

The initial block height of the chain according to request::InitChain from tower ABCI is 1 (see code here), which does not match the initial block height of the chain in the namada protocol, which is 0.

The initial block height of 1 from tower ABCI is used to determine when the next epoch begins right here, so possibly epoch 1 (the second epoch) starts a block later than intended.

@brentstone brentstone added bug Something isn't working ledger labels Oct 19, 2022
@james-chf
Copy link
Contributor

james-chf commented Oct 20, 2022

Related: #377

We should look to remove BlockHeight(0) in general from our code since it doesn't exist (and change BlockHeight::default() to be BlockHeight(1) instead of BlockHeight(0))

@tzemanovic
Copy link
Member

Good catch! Looks like we should be setting our block height from the initial_height we receive on init chain.

There's one place where BlockHeight(0) is used - in ABCI queries, where it's used as a special meaning for "last committed height"

@tzemanovic
Copy link
Member

tzemanovic commented Oct 28, 2022

Additionally, we should initialize storage.block.pre_epochs with empty first_block_heights and write it from init-chain too

@tzemanovic
Copy link
Member

tzemanovic commented Nov 3, 2022

thanks @james-chf for clearing this up in another discussion. There is no genesis block, InitChain initializes state to be used for the first block, which will have the BlockHeight(1) - the confusing part for me was that InitChain request comes with height = 1, which really means "prepare the genesis state for the first block, which will have height 1".

The issue with storage.block.pre_epochs is actually wrong though - we should use the InitChain height to initialize its value.

@cwgoes
Copy link
Collaborator

cwgoes commented Jan 14, 2023

@tzemanovic What still needs to be done here - just the block height type fix?

@tzemanovic
Copy link
Member

@tzemanovic What still needs to be done here - just the block height type fix?

the default value for pred_epochs.first_block_heights is incorrect - we're using 0 height, but it should be set from the value received in init_chain (typically 1)

@tzemanovic tzemanovic added the has-a-pr The issue has been solved by a PR that has yet to be merged label Oct 23, 2023
phy-chain pushed a commit to phy-chain/namada that referenced this issue Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has-a-pr The issue has been solved by a PR that has yet to be merged ledger
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants