-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Switch IAVL Store query to use ics proofs #6324
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6324 +/- ##
==========================================
- Coverage 55.68% 55.54% -0.15%
==========================================
Files 451 452 +1
Lines 27254 27340 +86
==========================================
+ Hits 15177 15185 +8
- Misses 10985 11064 +79
+ Partials 1092 1091 -1 |
Please ping me when ready for review. Happy to have a look |
Hey Aditya, I had an idea how to make this a bit more generic so that 95% of the code can be reused to parse the simple merkle proofs as well. I will give another review, then make a small PR on top of this one, that you can chose to merge in (or not) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Only real issue is the logic for calculating root of the non-existence proog
A few cleanup comments and then I think this is gold.
More than anything this points out other apis that need updates.
@@ -5,6 +5,8 @@ require ( | |||
github.com/bgentry/speakeasy v0.1.0 | |||
github.com/btcsuite/btcd v0.20.1-beta | |||
github.com/btcsuite/btcutil v1.0.2 | |||
github.com/confio/ics23-iavl v0.6.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow up to this (sometime before tagging the 0.39 sdk), it would be nice to merge this ics23-iavl code into tendermint/iavl, so this can be optimized and integrated in the tendermint codebase.
Do you want to make an issue for that if you agree (I feel odd doing so and cannot label/milestone it)
store/iavl/proof.go
Outdated
"github.com/tendermint/tendermint/crypto/merkle" | ||
) | ||
|
||
const ProofOpIAVLCommitment = "iavlstore" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use a name like ics23:iavl
as this is more clear and as we will be adding other ics23:*
proofs that differ only in spec.
store/iavl/proof.go
Outdated
return op.Key | ||
} | ||
|
||
func (op CommitmentOp) Run(args [][]byte) ([][]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please link to the ProofOperator definition in this go-doc.
Which really needs to be much more clear about expected input and output.
But that is another issue (a core team member could gladly open and link)
store/iavl/proof.go
Outdated
// and at least one proof must be non-nil | ||
root, err := nonexistProof.Nonexist.Left.Calculate() | ||
if err != nil { | ||
// Left proof may be nil, check right proof |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, you should probably check if nonexistProof.Nonexist.Left != nil
above rather than calling Calculate
on it (which would likely panic) and treating any error as missing
var commitmentProof *ics23.CommitmentProof | ||
var err error | ||
|
||
// Must convert store.Tree to iavl.MutableTree with given version to use in CreateProof |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you take a look at ics23-iavl to see if I can change this requirement (and accept iavl.ImmutableTree instead). I was just gettng it to work, but in this use case referring to an immutable tree seems better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea since you aren't using any Versioned function, you should be able to just swap out MutableTree
for ImmutableTree
in the function signatures and everything else should still work.
This will make the code here cleaner
@AdityaSripal please look at #6331 which extends this a bit. You may also want to move the |
* Make the CommitmentOp generic over all ics23 ProofSpecs, using Type to distinguish * Register SimpleMerkle ics23 proof op as well * Addressed linter issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! I mostly just reviewed for code hygiene stuff, see suggestions
Co-authored-by: colin axner <25233464+colin-axner@users.noreply.github.com>
…into aditya/iavl-ics-query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM in general. Can we move the contents of the case "/key":
to a private function? 🙏
Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
// ProofOp implements ProofOperator interface and converts a CommitmentOp | ||
// into a merkle.ProofOp format that can later be decoded by CommitmentOpDecoder | ||
// back into a CommitmentOp for proof verification | ||
func (op CommitmentOp) ProofOp() merkle.ProofOp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would opt to return an error here and allow the caller to decide to panic or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot return an error here, this method is satisfying the ProofOperator interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, I would consider revising that interface if possible. Not a good UX IMHO. Ideally, you should always be able to bubble up errors as much as possible and allow callers to handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... yea that would definitely require a separate discussion with TM team and a separate PR since this is a tendermint interface. Don't think a panic is the worst idea here since it should be an invariant that any well-constructed ProofOperator
should successfully return a merkle.ProofOp
here
Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
@AdityaSripal if this is good to go, could you add the automerge label |
Description
closes: #6324
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorer