10000 types: is a `Proposal` with `Height == 0` valid? · Issue #2210 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

types: is a Proposal with Height == 0 valid? #2210

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.

Sign up for GitHub

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

Jump to bottom
Open
cason opened this issue Jan 31, 2024 · 7 comments
Open

types: is a Proposal with Height == 0 valid? #2210

cason opened this issue Jan 31, 2024 · 7 comments
Labels
hygiene Any work relating to code legibility/hygiene to make it easier to read P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably

Comments

@cason
Copy link
Contributor
cason commented Jan 31, 2024 •
edited
Loading

A Proposal with height 0 is considered valid, see:

cometbft/types/proposal.go

Lines 53 to 55 in 9613e7a

if p.Height < 0 {
return errors.New("negative Height")
}

The associated test unit only tests height -1, see:

cometbft/types/proposal_test.go

Line 136 in 9613e7a

{"Invalid Height", func(p *Proposal) { p.Height = -1 }, true},

But as @sergio-mena pointed out in #2206 (comment):

          I checked the validation function and it's true it allows for height 0, but is it a bug? I searched around and never saw the height being set to 0, neither in production nor in tests (maybe I missed it)
@cason cason mentioned this issue Jan 31, 2024
Merged
4 tasks
@cason cason added the hygiene Any work relating to code legibility/hygiene to make it easier to read label Jan 31, 2024
@andynog
Copy link
Contributor
andynog commented Feb 2, 2024

In the data structures spec doc it says height > 0 https://github.com/cometbft/cometbft/blob/main/spec/core/data_structures.md#header

@cason
Copy link
Contributor Author
cason commented Feb 5, 2024

In the data structures spec doc it says height > 0 https://github.com/cometbft/cometbft/blob/main/spec/core/data_structures.md#header

Or >= initialHeight, which would allow for 0? Not sure.

The introduction of this parameter, which appears not to be really documented, was ADR 078. Not 100% clear to me that it must be greater than zero.

@sergio-mena
Copy link
Contributor

initialHeight MUST be > 0 IIRC

@cason
Copy link
Contributor Author
cason commented Feb 6, 2024

Indeed. We allow for 0 but we then replace 0 by 1:

https://github.com/cometbft/cometbft/blob/main/types/genesis.go#L76-L81

@cason
Copy link
Contributor Author
cason commented Feb 6, 2024

This comment should encourage updating the documentation once genesis fields/validation rules are changed: https://github.com/cometbft/cometbft/blob/main/types/genesis.go#L24-L27

The documentation https://docs.cometbft.com/v0.38/core/using-cometbft#fields is not exactly clear regarding that.

@cason
Copy link
Contributor Author
cason commented Feb 6, 2024

This parameter was introduced by cc247c0.

Originally, 1 was the default and 0 should not be used.

@cason
Copy link
Contributor Author
cason commented Feb 6, 2024

Last time this documentation was updated: 226af0a

@cason cason added the P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
hygiene Any work relating to code legibility/hygiene to make it easier to read P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@cason @sergio-mena @andynog
0