Refactor CommitValidator and port over code from the lite_impl module
#358
Conversation
| // | ||
| // It returns `ImplementationSpecific` error if it detects a signer | ||
| // that is not present in the validator set | ||
| fn validate_full( |
There was a problem hiding this comment.
it makes sense to decouple these and have a separate validate_full method instead 👍
| .validate(signed_header, validators) | ||
| .map_err(|e| VerificationError::InvalidCommit(e.to_string()))?; | ||
| commit_validator.validate(signed_header, validators)?; | ||
| commit_validator.validate_full(signed_header, validators)?; |
There was a problem hiding this comment.
Is there actually a place now where commit_validator.validate is used without validate_full?
There was a problem hiding this comment.
No there's not. We currently perform both checks, and I don't see the need/place for doing just validate since we do 2/3 full verification of every block by that point. But perhaps that's not right, and we need to only to validate_full somewhere here: https://github.com/informalsystems/tendermint-rs/pull/358/files/b1aee8d1f4b42d56a5e43dda9fc52572455ff4ea#diff-1911c69336354ecbef5a4524721976faR236-R263
There was a problem hiding this comment.
if we do #377, we can prob get rid of validate_full, especially since we're trying not to rely on the underlying validator_address. though I suppose so long as it is there it should be checked.
Close: #341
Ref: #342