-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Enforce ProofSpecs in 23-Commitment #6374
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6374 +/- ##
==========================================
- Coverage 55.71% 55.65% -0.07%
==========================================
Files 465 465
Lines 27512 27651 +139
==========================================
+ Hits 15329 15389 +60
- Misses 11097 11163 +66
- Partials 1086 1099 +13 |
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, don't we want to enforce the full ICS-23 spec (e.g. https://github.com/confio/ics23/blob/master/proofs.proto#L155, including max depth, min depth, etc.)?
If we aim to do that after / separately I suppose this is fine as an intermediate step. This doesn't really constrain what the proof actually contains though.
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 👍 Pending validation comments and test fixes. Thanks @AdityaSripal 🙂
@cwgoes When decoding proof ops, there is a mapping from the string type to the ics23 spec to be enforced: https://github.com/cosmos/cosmos-sdk/blob/master/store/types/proof.go#L48-L60 This is not extensible yet but simply enforcing the string types for ProofOps, and limiting them all to This definitely could be more extensible and configured elsewhere, but I proposed @AdityaSripal pull this logic out as it will provide correctness. And there is a larger discussion that needs to be had within the team if you want to rip out merkle.ProofOp everywhere for *ics23.CommitmentProof. Until now, the changes have been relatively non-invasive. My main point is that whatever proof format you use on the ibc packets should be the same proof format you return for abci.Query. If these are different, you are putting a lot more work on the client to keep them in sync, which is much more fragile than testing compatibility in the same codebase |
I see, thanks. That seems reasonable for now.
Yes, I agree with this. |
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.
ACK. A few suggestions
if err := clientState.Validate(); err != nil { | ||
return ClientState{}, err | ||
} |
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.
not required anymore as the check is on the MsgCreateClient
validation 👍
if err := clientState.Validate(); err != nil { | |
return ClientState{}, err | |
} |
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.
Feel free to remove the Initialize function and replace it with just the ClientState constructor
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.
The code looks fine, but I'm wondering if we want to allow dynamic ics23.ProofSpec
choices when creating a client?
@@ -34,6 +34,7 @@ type CreateClientReq struct { | |||
TrustingPeriod time.Duration `json:"trusting_period" yaml:"trusting_period"` | |||
UnbondingPeriod time.Duration `json:"unbonding_period" yaml:"unbonding_period"` | |||
MaxClockDrift time.Duration `json:"max_clock_drift" yaml:"max_clock_drift"` | |||
ProofSpecs []string `json:"proof_specs" yaml:"proof_specs"` |
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.
Do we want there to be a way for the client creator to provide the full ics23.ProofSpec
?
If we don't have that, then we're limited to whatever specs the SDK has hard-coded "by name".
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.. for now this is not that useful. Since we're still using merkle.Proof
and ProofRuntime
to verify proofs, we cannot actually verify a proof that isn't registered and hard-coded into the proofruntime anyway.
Think this is something to revisit once we pull out ProofRuntime
from ics23 verification
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.
Ahh but I suppose we can change the interfaces now, so we don't have to change them again
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 after thinking about it, realized there isn't a simple way to do this without completely pulling out merkle.ProofRuntime
from the `23-commitment package.
I can do this if there's consensus that this is preferable, but it does involve more work since I have to manually verify the proof with passed in specs, rather than just calling into ProofRuntime
. Not too hard tho
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 think this is probably worthwhile, since it will allow SDK chains to support new ProofSpec
s without manual upgrades
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.
Ok, so now 23-commitment no longer uses ProofRuntime
. Instead we use the ics23 Verify functions directly with the ProofSpecs passed in from the client.
Now, ibc can verify Commitment proofs even if the chain does not have them hardcoded into its proof runtime
Dismissing approval since many changes happened since
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.
ACK, pending a few additional test cases. I recommend you to download the Octolinker browser extension to see line coverage on PRs.
case "simple": | ||
specs = []*ics23.ProofSpec{ics23.TendermintSpec} | ||
default: | ||
return fmt.Errorf("proof Spec: %s not supported", spc) |
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.
return fmt.Errorf("proof Spec: %s not supported", spc) | |
return fmt.Errorf("proof spec %s isn't supported", spc) |
@@ -143,9 +140,31 @@ func (cs ClientState) Validate() error { | |||
if cs.MaxClockDrift == 0 { | |||
return sdkerrors.Wrap(ErrInvalidMaxClockDrift, "max clock drift cannot be zero") | |||
} | |||
if cs.TrustingPeriod >= cs.UnbondingPeriod { |
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.
missing test case
} | ||
// Validate ProofSpecs | ||
if cs.ProofSpecs == nil { | ||
return sdkerrors.Wrap(ErrInvalidProofSpecs, "proof specs cannot be nil for tm client") |
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.
missing test case
} | ||
for _, spec := range cs.ProofSpecs { | ||
if spec == nil { | ||
return sdkerrors.Wrap(ErrInvalidProofSpecs, "proof spec cannot be nil") |
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.
missing test case
@@ -95,6 +99,21 @@ func (msg MsgCreateClient) ValidateBasic() error { | |||
if err := msg.Header.ValidateBasic(msg.Header.ChainID); err != nil { | |||
return sdkerrors.Wrapf(ErrInvalidHeader, "header failed validatebasic with its own chain-id: %v", err) | |||
} | |||
if msg.TrustingPeriod >= msg.UnbondingPeriod { |
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.
missing test case
} | ||
// Validate ProofSpecs | ||
if msg.ProofSpecs == nil { | ||
return sdkerrors.Wrap(ErrInvalidProofSpecs, "proof specs cannot be nil") |
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.
missing test case
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.
utACK
Agreed with @fedekunze comments, also would like a quick look by @ethanfrey if he has time
@@ -88,8 +96,20 @@ func GetCmdCreateClient(cdc *codec.Codec) *cobra.Command { | |||
return err | |||
} | |||
|
|||
spc := viper.GetString(flagProofSpecs) | |||
|
|||
// Currently supports SDK chain or simple kvstore tendermint chain |
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 we support reading a spec from JSON or something?
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.
This can happen later though
... hmm I don't think we wanted to merge this automatically? |
It merges it with 2+ approvals. @AdityaSripal can you add the tests in a separate PR 🙏 |
Co-authored-by: Alexander Bezobchuk <[email protected]>
Description
Simple way to make 23-commitment use ics23-proofs by still requiring that we use
merkle.Proof
but enforce that eachOp
in the proof is as expected by the ibc client.Taken from: #6344 (comment)
Supersedes #6344
closes: #6321
TODO:
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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorer