Light client conformance tests differences between description and real errors

[OStevan]

When running tests for light client I noticed that for not_enough_commits (in the current canonical implementation) and invalid_validator_set (the new one in light-client) the expected error based on the description is not the same as the error we get when the test is run.

not_enough_commits: fails with InvalidNextValidatorSet for height 5 instead of InsufficientCommitPower for height 6

invalid_validator_set: fails with request for height 6 instead of invalid InvalidNextValidatorSet for height 11

[OStevan]

@ebuchman I'll continue adding differences between errors we would expect based on the description and the ones which are currently registered in this issue.

[OStevan]

wrong_header_height, wrong_header_timestamp, wrong_last_block_id: fails with InvalidCommitValue while the description would suggest a different error.

[Shivani912]

I'm looking into this now

[Shivani912]

When running tests for light client I noticed that for not_enough_commits (in the current canonical implementation) and invalid_validator_set (the new one in light-client) the expected error based on the description is not the same as the error we get when the test is run.

not_enough_commits: fails with InvalidNextValidatorSet for height 5 instead of InsufficientCommitPower for height 6

invalid_validator_set: fails with request for height 6 instead of invalid InvalidNextValidatorSet for height 11

So as @OStevan explained, there were errors in not_enough_commits and invalid_validator_set. These have been fixed in PR #366 .

wrong_header_height, wrong_header_timestamp, wrong_last_block_id: fails with InvalidCommitValue while the description would suggest a different error.

Well, description in tests explain "where" the error should be instead of "what". Specifying the error type could be tricky here because the above cases are essentially all about detecting differences in header and commit. So for the same error, you could say it's an invalid header. As long as it fails for the expected reason, I wouldn't care much about the error type.

[OStevan]

@Shivani912 thanks for following up on this.

Well, description in tests explain "where" the error should be instead of "what". Specifying the error type could be tricky here because the above cases are essentially all about detecting differences in header and commit. So for the same error, you could say it's an invalid header. As long as it fails for the expected reason, I wouldn't care much about the error type.

I'll just explain my reasoning why I mentioned this. I considered this as an issue because the current implementation passes tests because header hash check catches the differences. Therefore someone could easily delete those height checks, etc. in the code without failing any of those tests and then someone could make artificial light blocks which would pass the validation. Also it seems a bit wasteful to compute the hash for the whole block while there is only an error in height values.

[Shivani912]

@Shivani912 thanks for following up on this.

Well, description in tests explain "where" the error should be instead of "what". Specifying the error type could be tricky here because the above cases are essentially all about detecting differences in header and commit. So for the same error, you could say it's an invalid header. As long as it fails for the expected reason, I wouldn't care much about the error type.

I'll just explain my reasoning why I mentioned this. I considered this as an issue because the current implementation passes tests because header hash check catches the differences. Therefore someone could easily delete those height checks, etc. in the code without failing any of those tests and then someone could make artificial light blocks which would pass the validation. Also it seems a bit wasteful to compute the hash for the whole block while there is only an error in height values.

Thanks for explaining further @OStevan but I still don't understand what you mean by deleting checks and and validating artificial light blocks 🤔

I do agree it'll be useful to have less expensive checks for fields like height or timestamp