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.

Sign up for GitHub

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

Jump to bottom

use ibc_proto::protobuf::Protobuf to replace tendermint_proto::Protobuf. #754

Merged
merged 5 commits into from
Jul 11, 2023
Merged

use ibc_proto::protobuf::Protobuf to replace tendermint_proto::Protobuf. #754

merged 5 commits into from
Jul 11, 2023

Conversation

DaviRain-Su
Copy link
Contributor

@DaviRain-Su DaviRain-Su commented Jul 10, 2023 •
edited
Loading

Closes: #747

Description

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

Farhad-Shabani
Farhad-Shabani reviewed Jul 11, 2023
View reviewed changes
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

Just a few nits, otherwise LGTM 🙏

crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs
@@ -117,9 +117,7 @@ where
&msg.proof_consensus_state_of_a_on_b,
consensus_state_of_b_on_a.root(),
Path::ClientConsensusState(client_cons_state_path_on_b),
expected_consensus_state_of_a_on_b
.encode_vec()
.map_err(ConnectionError::ConsensusStateEncodeFailure)?,
Copy link
Member

Choose a reason for hiding this comment

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

Would you remove the ConsensusStateEncodeFailure from the errors? It's no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

crates/ibc/src/core/ics02_client/consensus_state.rs Outdated Show resolved Hide resolved
Farhad-Shabani
Farhad-Shabani reviewed Jul 11, 2023
View reviewed changes
.changelog/unreleased/bug/747-fix-747-Protobuf-out-of-memory.md
@@ -0,0 +1,2 @@
- Tendermint ConsensusState -> Any can crash if out of memory
Copy link
Member

Choose a reason for hiding this comment

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

Would you also add a unclog for the own PR under breaking-changes, so we can track the encode_vec signature change too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Patch coverage: 85.71% and project coverage change: -0.01 ⚠️

Comparison is base (e125999) 73.21% compared to head (fa36bf1) 73.20%.

❗ Current head fa36bf1 differs from pull request most recent head 76f1723. Consider uploading reports for the commit 76f1723 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
- Coverage   73.21%   73.20%   -0.01%     
==========================================
  Files         125      125              
  Lines       16085    16082       -3     
==========================================
- Hits        11776    11773       -3     
  Misses       4309     4309
Impacted Files Coverage Δ
crates/ibc/src/mock/context.rs 74.84% <ø> (ø)
...bc/src/clients/ics07_tendermint/consensus_state.rs 82.97% <50.00%> (-0.18%) ⬇️
crates/ibc-derive/src/consensus_state.rs 98.18% <100.00%> (ø)
...src/core/ics03_connection/handler/conn_open_ack.rs 96.62% <100.00%> (-0.02%) ⬇️
...src/core/ics03_connection/handler/conn_open_try.rs 94.02% <100.00%> (-0.03%) ⬇️
crates/ibc/src/mock/consensus_state.rs 87.71% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Farhad-Shabani Farhad-Shabani merged commit bd2c1e6 into cosmos:main Jul 11, 2023
10 checks passed
@DaviRain-Su DaviRain-Su deleted the fix-747 branch July 11, 2023 04:57
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
@DaviRain-Su
use ibc_proto::protobuf::Protobuf to replace tendermint_proto::Protob…
322ab37 
…uf. (#754)

* use ibc_proto::protobuf::Protobuf to replace tendermint_proto::Protobuf.

* fix tests

* Create 747-fix-747-Protobuf-out-of-memory.md

* Fix issue 747 by replacing tendermint_proto::Protobuf with ibc_proto::protobuf::Protobuf (#754)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Farhad-Shabani Farhad-Shabani Farhad-Shabani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
ibc-rs
Status: 📥 To Do
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tendermint ConsensusState -> Any can crash if out of memory
2 participants
@DaviRain-Su @Farhad-Shabani