celestiaorg / celestia-core

Celestia node software based on Tendermint.
https://celestia.org/
Apache License 2.0
470 stars 244 forks source link

Difference between `Validate` and `Verify` in `ShareProof` and `RowProof` #1356

Closed rach-id closed 3 weeks ago

rach-id commented 1 month ago

Currently, in the implementation of ShareProof and RowProof, we have Validate() and Verify() methods. Now I am looking at the implementation, both of them seem to be doing the same thing.

https://github.com/celestiaorg/celestia-core/blob/b67d79ecbdb8ebb046131a1f733356fa915dc192/types/row_proof.go#L24-L52

and,

https://github.com/celestiaorg/celestia-core/blob/b67d79ecbdb8ebb046131a1f733356fa915dc192/types/share_proof.go#L66-L133

I am thinking that if a proof.Verify returns true, then it's a valid proof. Similarly, if a proof.Validate returns true, there is no need to call Verify. So they're basically the same methods.

So I am wondering if it makes sense to remove the proof verification from the Validate method, and keep it only doing basic checks. And putting the responsibility of verifying the proof on Verify.

What do you think?

rootulp commented 1 month ago

So I am wondering if it makes sense to remove the proof verification from the Validate method, and keep it only doing basic checks. And putting the responsibility of verifying the proof on Verify.

That makes sense to me and aligns with how I would interpret Validate vs. Verify.

rach-id commented 1 month ago

The issue is that it will be a breaking change... Should we just create a PR for it in main and leave it there until someday?

rootulp commented 1 month ago

Hmm yea I think it's still worth fixing. Separately we also need to figure out how we're going to release celestia-core breaking changes (cc: @evan-forbes because figuring that out seems like it falls into the maintenance workstream).