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

LightBlock verification ignores last_block_id #651

Open
Shivani912 opened this issue Oct 22, 2020 · 3 comments
Open

LightBlock verification ignores last_block_id #651

Shivani912 opened this issue Oct 22, 2020 · 3 comments
Labels
bug Something isn't working light-client Issues/features which involve the light client verification

Comments

@Shivani912
Copy link
Contributor

Shivani912 commented Oct 22, 2020

In the current Model Based Testing, we ignore a few Header fields i.e. set them to null and one of them is last_block_id. According to the spec, Blocks, except for the first one, is expected to have last_block_id pointing to the previous block's block_id. It's surprising that neither Tendermint in Go nor in Rust checks this!

Also with the idea of correct-by-construction data structures, I'd expect a Block at height > 1 with a missing last_block_id to fail to translate into Domain type.

@Shivani912 Shivani912 added bug Something isn't working light-client Issues/features which involve the light client verification labels Oct 22, 2020
@ebuchman
Copy link
Member

Thanks @Shivani912 . I think the reason we don't check it is because there's nothing we can validate during skipping verification, so it would only be relevant during sequential, and it doesn't seem to pose a meaningful attack vector, since you already have the blocks chained through the NextValidatorSet and through the next blocks LastCommitID (which points to the blockID of the previous block, effectively giving you the same thing as a LastBlockID).

That said, it would probably be good if the MBT was using complete data structures as much as possible, and you're right that we shouldn't even be able to construct a Block type without it for height > 1. This again calls up the #101 issue

@Shivani912
Copy link
Contributor Author

Thanks @ebuchman and apologies if this is a totally dumb discussion. I'm still not convinced that we shouldn't have it 😄

since you already have the blocks chained through the NextValidatorSet and through the next blocks LastCommitID (which points to the blockID of the previous block, effectively giving you the same thing as a LastBlockID).

I'm not sure what you mean by LastCommitID 🤔

That said, it would probably be good if the MBT was using complete data structures as much as possible,

Yes, ideally it should but it's also interesting to see that an incomplete data structure passes verification.

@ebuchman
Copy link
Member

I'm not sure what you mean by LastCommitID 🤔

Sorry I meant block.LastCommit.BlockID.

Not a dumb discussion at all, we should certainly strive to fill out the data structures properly. Will be especially important so we can use them for block processing tests when we get there.

@thanethomson thanethomson added this to the v0.18 milestone Nov 19, 2020
@thanethomson thanethomson removed this from the v0.19 milestone Apr 7, 2021
@thanethomson thanethomson added this to the Q3 2021 milestone Jul 13, 2021
@thanethomson thanethomson removed this from the Q3 2021 milestone Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working light-client Issues/features which involve the light client verification
Projects
None yet
Development

No branches or pull requests

3 participants