Add SmtSpec for SMT proofs
#57
Conversation
|
This will need a Go version bump, should it go to 0.6.8 or would 0.7.0 be more semver-appropriate? |
There was a problem hiding this comment.
Thank you for adding this.
This is non-breaking addition, and actually could be copied directly in the app (no need to block on it). It is great to keep the standard specs used by most cosmos chains in this repo for easy reference.
I would just ask for a test case to show this test properly verifying (and rejecting) at leats one good and one bad proof from SMT
| @@ -40,6 +40,25 @@ var TendermintSpec = &ProofSpec{ | |||
| }, | |||
| } | |||
|
|
|||
| // SmtSpec constrains the format for SMT proofs (as implemented by github.com/celestiaorg/smt) | |||
There was a problem hiding this comment.
It would be nice to have a test case (even json file, table test... something generated by a real SMT that we can test against).
Let's verify this works with your SMT proofs.
There was a problem hiding this comment.
Originally this was in the cosmos-sdk repo, but I moved it here to keep a single source of truth.
Test cases are here, in the code for the sdk PR. I don't have one with a failed verification yet, but I will add that. Should I add any tests within this repo?
There was a problem hiding this comment.
Please add some basic tests in this repo.
Happy to have it here, just tested here as well
|
@roysc bumping I am happy to merge as soon as there are a couple test specs. |
|
Sorry, I haven't had time to get to creating a proper serialization tool like |
|
After testing more thoroughly, I found that the proof verification algorithm needs to be updated to check for empty child nodes in non-existence proof paths to support SMT proofs - see PR at #61. I will rebase this branch on that PR and add the tests and data to complete it. I've also written a ics23-smt library to generate and check proof data (which also depends on the above fix). |
roysc
force-pushed
the
add-smt-spec
branch
2 times, most recently
from
ceb62fd to
1d04694
Compare
😄 See I'm not just being annoying when asking for test cases... they do find issues. I will review #61 now |
|
Haha yes, the tests in the SDK were a bit too trivial it seems |
|
Bumping this one to check the status. Are you waiting for my review or am I waiting for more tests? |
|
This is also ready for review, sorry for the delay. The JS part is not complete, but I will leave it in for now. |
The code erroneously assumed that the inner op has the correct padding for the side it is checking. e.g. during IsLeftMost: when hasPadding(step) == false, leftBranchesAreEmpty(step, 0) is called, but 0 indicates a left-branch, and hasPadding == false implies this is _not_ a left branch. Instead, we must detect the branch index from the op's padding, since the caller does not know which, if any, branch this padding is valid for.
|
I've rebased and updated this now that the placeholder check is merged. I've also had to add a fix to the placeholder check logic, since it was being called with the wrong branch index. The fix is to detect the index from the padding within the empty check. |
| @@ -230,7 +250,7 @@ func IsRightMost(spec *InnerSpec, path []*InnerOp) bool { | |||
|
|
|||
| // ensure every step has a prefix and suffix defined to be rightmost, unless it is a placeholder node | |||
| for _, step := range path { | |||
| if !hasPadding(step, minPrefix, maxPrefix, suffix) && !rightBranchesAreEmpty(spec, step, int32(last)) { | |||
| // leftBranchesAreEmpty returns true if the padding bytes correspond to all empty siblings | ||
| // on the left side of a branch, ie. it's a valid placeholder on a leftmost path | ||
| func leftBranchesAreEmpty(spec *InnerSpec, op *InnerOp) bool { | ||
| idx, err := orderFromPadding(spec, op) |
| // count branches to left of this | ||
| let left_branches = idx as usize; | ||
| if left_branches == 0 { | ||
| fn left_branches_are_empty(spec: &ics23::InnerSpec, op: &ics23::InnerOp) -> Result<bool> { |
This will be needed for implementing ADR-040 in Cosmos-SDK; specifically this PR depends on it: cosmos/cosmos-sdk#10015