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

Absurd values for max_msg_num and max_tx_size #1245

Closed
7 tasks
adizere opened this issue Jul 28, 2021 · 0 comments · Fixed by #1246
Closed
7 tasks

Absurd values for max_msg_num and max_tx_size #1245

adizere opened this issue Jul 28, 2021 · 0 comments · Fixed by #1246
Assignees
Labels
A: bug Admin: something isn't working
Milestone

Comments

@adizere
Copy link
Member

adizere commented Jul 28, 2021

Crate

ibc-relayer

Summary

The types max_msg_num and max_tx_size are defined as simple usize with current defaults to:

https://github.com/informalsystems/ibc-rs/blob/9f38c37bc80ea7e315cf8a16cee845924dde6970/relayer/src/chain/cosmos.rs#L93-L94

If the user specifies these configuration options in the config.toml, they can be set to arbitrary values, within the bounds of usize type, which can lead to undefined behavior.

Problem Definition

If absurd values are specified, then undefined behavior can occur. For example, a trace shows the following weird error when Hermes attempts to send 5400 messages in a single transaction:

Jul 28 17:33:46.344 INFO [gaia:transfer/channel-0 -> osmo] relay op. data of 5400 msgs(s) to Destination (height 0-586), delayed by: 24.712586263s [try 1/5]
Jul 28 17:33:46.358 INFO [gaia:transfer/channel-0 -> osmo] prepending Destination client update @ height 0-586
Jul 28 17:33:46.365 TRACE light client verification trusted=0-559 target=0-586
Jul 28 17:33:46.373 TRACE adjusting headers with 0 supporting headers trusted=0-559 target=586
Jul 28 17:33:46.373 TRACE fetching header height=0-560
Jul 28 17:33:46.378 DEBUG [gaia -> osmo:07-tendermint-7] MsgUpdateAnyClient from trusted height 0-559 to target height 0-586
Jul 28 17:33:46.378 INFO [gaia:transfer/channel-0 -> osmo] assembled batch of 5401 message(s)
Jul 28 17:33:46.390 DEBUG [osmo] send_tx: sending 5401 messages using nonce 2
Jul 28 17:33:46.493 TRACE [osmo] send_tx: based on the estimated gas, adjusting fee from Fee { amount: [Coin { denom: "uosmo", amount: "0" }], gas_limit: 30000000000, payer: "", granter: "" } to Fee { amount: [Coin { denom: "uosmo", amount: "0" }], gas_limit: 30000000000, payer: "", granter: "" }
Jul 28 17:33:46.634 ERROR [gaia:transfer/channel-0 -> osmo] worker: schedule execution encountered error: failed with underlying error: RPC error to endpoint http://127.0.0.1:26457/: RPC error to endpoint http://127.0.0.1:26457/: Parse error. Invalid JSON: missing field id at line 8 column 1 (code: -32700) path=packet::channel-0/transfer:gaia->osmo

Proposal

We should add some basic domain type validation for the two configuration parameters max_msg_num and max_tx_size. Additionally, the parameter max_tx_size should be within a certain fraction of the tendermint core consensus params max_bytes parameter.

Acceptance Criteria

  • domain type validation for max_msg_num and max_tx_size so that they respect certain upper-bounds
  • validation of max_tx_size against TM core max_bytes parameter

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere adizere added the A: bug Admin: something isn't working label Jul 28, 2021
@adizere adizere added this to the 07.2021 milestone Jul 28, 2021
@adizere adizere self-assigned this Jul 28, 2021
adizere added a commit that referenced this issue Jul 28, 2021
romac added a commit that referenced this issue Jul 29, 2021
…config options (#1246)

* Fix for #1245

* Split out params semantic validation from health check

* Fixup changelog entry

* Add .changelog entry

* Extract max fraction of genesis block max size into a consant

* Doc comments for constants

* Harmonize error messages

* Improve error message layout

* Comments & output consistent with parameters

* Final aestetic change

Co-authored-by: Romain Ruetschi <romain@informal.systems>
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this issue Sep 13, 2022
…config options (informalsystems#1246)

* Fix for informalsystems#1245

* Split out params semantic validation from health check

* Fixup changelog entry

* Add .changelog entry

* Extract max fraction of genesis block max size into a consant

* Doc comments for constants

* Harmonize error messages

* Improve error message layout

* Comments & output consistent with parameters

* Final aestetic change

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Admin: something isn't working
Projects
None yet
1 participant