Skip to content
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

Implement proof verification functions for Tendermint client #1583

Merged
merged 30 commits into from
Dec 22, 2021

Conversation

yito88
Copy link
Contributor

@yito88 yito88 commented Nov 22, 2021

Closes: cosmos/ibc-rs#76

Description

Add proof verification functions for Tendermint client.
I referred to ibc-go implementation.
I think we need some tests and discussion. Welcome your feedback.


For contributor use:

  • Added a changelog entry, using unclog.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@hu55a1n1
Copy link
Member

Thanks so much for this PR @yito88! 🙏 Amazing stuff! 👌

@romac romac changed the title Yuji/tm proof verification Implement proof verification functions for Tendermint client Nov 22, 2021
@hu55a1n1 hu55a1n1 added this to the v0.9.1 milestone Nov 23, 2021
@hu55a1n1 hu55a1n1 self-assigned this Nov 23, 2021
@adizere adizere removed this from the v0.9.1 milestone Nov 25, 2021
@hu55a1n1
Copy link
Member

hu55a1n1 commented Dec 10, 2021

Sorry for the delay in getting to this @yito88. I just pushed some changes ->

  • I moved the verify_height() code out of ics02 because I think it's Tendermint specific.
  • I reverted to our old way of passing identifiers to verification functions instead of giving them ready-made paths because I think that is more error-prone and would require an assertion like assert!(matches!(path, Path::Connections(..))) to make sure we passed the right path.
  • Also removed the ics23 file changes as they aren't required anymore.

I will continue to test/review and get back with more feedback.

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work!

I left a bunch of improvement suggestions, none of them major. I think the PR is almost ready!

modules/src/clients/ics07_tendermint/client_def.rs Outdated Show resolved Hide resolved
modules/src/clients/ics07_tendermint/client_def.rs Outdated Show resolved Hide resolved
modules/src/clients/ics07_tendermint/client_state.rs Outdated Show resolved Hide resolved
modules/src/core/ics04_channel/error.rs Show resolved Hide resolved
modules/src/core/ics23_commitment/error.rs Outdated Show resolved Hide resolved
modules/src/core/ics23_commitment/merkle.rs Show resolved Hide resolved
modules/src/mock/context.rs Outdated Show resolved Hide resolved
@hu55a1n1
Copy link
Member

I tried to test this code with basecoin-rs and I see that verification is failing for ConnOpenTry proofs ->

$ RUST_BACKTRACE=1 hermes create connection ibc-0  basecoin-0
# ...
2021-12-21T19:16:55.367966Z ERROR ThreadId(01) Failed ConnTry ConnectionSide { chain: ProdChainHandle { chain_id: ChainId { id: "basecoin-0", version: 0 }, runtime_sender: Sender { .. } }, client_id: ClientId("07-tendermint-0"), connection_id: None }: 
   0: tx response event consists of an error: deliver_tx for A5AE0DDB8AE0AF6248BF97FD9BBF2378A6F38E26B265C864E0061A64DE70CE72 reports error: code=Err(2), log=Log("deliver failed with error: IBC module error\n\nCaused by:\n    ICS03 connection error\n\n    Caused by:\n       0: the client state proof verification failed for client id 07-tendermint-0\n       1: tendermint error\n       2: ics23 commitment error\n       3: proof verification failed\n\n    Location:\n        /home/hussaini/.cargo/registry/src/github.com-1ecc6299db9ec823/flex-error-0.4.4/src/tracer_impl/eyre.rs:10:9\n\nLocation:\n    /home/hussaini/.cargo/registry/src/github.com-1ecc6299db9ec823/flex-error-0.4.4/src/tracer_impl/eyre.rs:30:9")

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks Shoaib for the fixes and the follow-up issue!

Will let you merge this Shoaib 💯

@hu55a1n1
Copy link
Member

hu55a1n1 commented Dec 22, 2021

Okay, so I spent some time again trying to debug the ConnOpenTry proof-verification failure and I see that there is another problem with ConnOpenAck verification too.

$ RUST_BACKTRACE=1 hermes create connection ibc-0 basecoin-0
# ...
2021-12-22T12:33:23.142732Z ERROR ThreadId(10) [ibc-0] estimate_gas: failed to simulate tx. propagating error to caller: gRPC call failed with status: status: InvalidArgument, message: "failed to execute message; message index: 1: connection handshake open ack failed: failed client state verification for target client: 07-tendermint-0: chained membership proof failed to verify membership of value: 0A2B2F6962632E6C69676874636C69656E74732E74656E6465726D696E742E76312E436C69656E74537461746512780A056962632D301204080110031A040880EA4922040880DF6E2A02081432003A02104842190A090801180120012A0100120C0A02000110211804200C300142190A090801180120012A0100120C0A02000110201801200130014A07757067726164654A107570677261646564494243537461746550015801 in subroot 653E3B80A1E23801DCF4485EDD8B670AF579EB22B04B4BC6172EAC6C104FF766 at index 0. Please ensure the path and value are both correct.: invalid proof: invalid request", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} }
2021-12-22T12:33:23.143331Z ERROR ThreadId(01) Failed ConnAck ConnectionSide { chain: ProdChainHandle { chain_id: ChainId { id: "ibc-0", version: 0 }, runtime_sender: Sender { .. } }, client_id: ClientId("07-tendermint-0"), connection_id: Some(ConnectionId("connection-0")) }: 
   0: failed during a transaction submission step to chain id ibc-0
   1: gRPC call failed with status: status: InvalidArgument, message: "failed to execute message; message index: 1: connection handshake open ack failed: failed client state verification for target client: 07-tendermint-0: chained membership proof failed to verify membership of value: 0A2B2F6962632E6C69676874636C69656E74732E74656E6465726D696E742E76312E436C69656E74537461746512780A056962632D301204080110031A040880EA4922040880DF6E2A02081432003A02104842190A090801180120012A0100120C0A02000110211804200C300142190A090801180120012A0100120C0A02000110201801200130014A07757067726164654A107570677261646564494243537461746550015801 in subroot 653E3B80A1E23801DCF4485EDD8B670AF579EB22B04B4BC6172EAC6C104FF766 at index 0. Please ensure the path and value are both correct.: invalid proof: invalid request", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} }

Interestingly, this is also seen on master, so I think it is unrelated to this PR. It is difficult to say whether this is a problem with basecoin-rs, but I'll continue to debug this separately and open an issue if required.

@hu55a1n1 hu55a1n1 closed this Dec 22, 2021
@hu55a1n1 hu55a1n1 reopened this Dec 22, 2021
@hu55a1n1 hu55a1n1 merged commit fd4e413 into informalsystems:master Dec 22, 2021
@yito88 yito88 deleted the yuji/tm_proof_verification branch December 23, 2021 00:17
@yito88
Copy link
Contributor Author

yito88 commented Dec 23, 2021

@hu55a1n1 Thank you so much for your help!

hu55a1n1 added a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…lsystems#1583)

* proof verification functions

* fix counterparty in ConnectionOpenAck

* verify delay

* verify height

* Revert "verify height"

This reverts commit 4735aa7.

* Impl verify_height()

* Revert changes to ics23 types

* Update mock impl to use new ClientDef API with height

* Clippy happy

* Modify ClientDef API

* Implement max_expected_time_per_block()

* Fix mock build

* Refactor verify_delay_passed()

* Move get_block_delay() into ChannelReader trait as provided method

* Remove usages of std::

* Fix clippy errors

* Add keeper methods for processed time and height

* Set processed time using host_timestamp()

* Add new ICS02 error variant InvalidCommitmentProof

* Augment packet delay errors

* Revert to old naming of errors for client upgrade proofs

* Rename processed_{time,height}()

* Apply suggestions from code review

Co-authored-by: Adi Seredinschi <adizere@gmail.com>

* Record height in processed height/time errors

* Fix clippy errors

* Add changelog entry

Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
Co-authored-by: Adi Seredinschi <adizere@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants