Configuration and problems with gas_adjustment parameter

[loredanacirstea]

Summary

Allow configurable gas limits beyond the gas_adjustment = 1.0

ibc-rs tries to do a gas estimation before sending a transaction. If a gas estimation can be computed, then the gas limit value can only be tweaked with gas_adjustment (0.0. - 1.0). If you need to increase this gas estimation more, you need to change the code and rebuild from source.

Problem Definition

I personally needed to have custom gas because I was testing cutting-edge code and the gas estimation was off - gas_adjustment = 1.0 was not enough.
I don't see drawbacks to introducing a gas limit overwrite variable that is properly commented.

Proposal

1. increase gas_adjustment range (I am not aware of the drawbacks that made the team limit this value)
2. introduce a gas_overwite variable that can be used to provide a custom value that overrides all gas estimations

Acceptance Criteria

• User is free to customize/overwrite the gas limit.
• The value in config.toml for gas_adjustment is currently set to 1.0, and it should reflect the actual default (0.1)
  • https://github.com/informalsystems/ibc-rs/blob/3e7b2760ef214e43a16adc2220d53f95bc5e3772/config.toml#L163
• It seems that gas_adjustment = 0.0 leads to a panic. We should disallow that value
  • set up a two network setup and configure the gas adjustment to 0.0 for ibc-0, leave it default for ibc-1
  • do hermes create client ibc-0 ibc-1 -- this should work
  • do hermes create client ibc-1 ibc-0 -- this should panic

  • $ ./target/debug/hermes create client ibc-0 ibc-1
    2022-05-30T08:38:26.450768Z INFO ThreadId(01) using default configuration from '/Users/adi/.hermes/config.toml'
    thread '' panicked at 'assertion failed: digits.len() == 1', relayer/src/chain/cosmos/gas.rs:54:5
    note: run with RUST_BACKTRACE=1 environment variable to display a backtrace
    Error: foreign client error: error raised while creating client for chain ibc-0: failed sending message to dst chain : internal message-passing failure while receiving inter-thread request/response: receiving on an empty and disconnected channel

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[adizere]

Thanks for documenting this request!

Currently, gas_adjustment only allows values in the interval [0.0, 1.0]

• https://github.com/informalsystems/ibc-rs/blob/b6a4b0615fc6840f9ff9829d71c3c463ef7c06f8/relayer-cli/src/config.rs#L161
• https://github.com/informalsystems/ibc-rs/blob/37dbe8c42ae09403432ebfcfdb2a8a34d86909e9/relayer/src/chain/cosmos/gas.rs#L51

At first glance, it looks easier to increase the upper-limit on gas_adjustment from 1.0 to 10.0. We still need to double-check if this approach works as expected on the SDK side, but seems easier than the gas_overwrite approach.

Would the upper-bound 10.0 be sufficient for your use-case?

[loredanacirstea]

Also adding gas_overwrite would make ibc-rs unopinionated and most flexible about gas, but the 10.0 max on gas_adjustment should work for my use case. Thank you.

[ljoss17]

Hi @loredanacirstea I am exploring potential solutions for the gas_adjustment configuration. One of them would be having a special experimental Hermes binary, which would have looser configuration bounds. This would give access to a Hermes which could be used with gas_adjustment parameter between 0 and 10, without having any impact on the release version.
Would this solution be satisfactory ?

[romac]

Here's a proposal:

1. Let's deprecate gas_adjustment. The name of the setting does not properly convey what is used for imho, and its semantics (multiply the estimated gas by the value and add it to the estimation) are confusing.
2. Instead, replace it with a gas_multiplier setting, with the following semantics: take the estimated gas and multiply it by the value of the setting.

Meaning that when we previously had gas_adjustment = 0.1, which increases the gas by 10%, we would now have gas_multiplier = 1.1.

I believe we should enforce a lower bound of 1.0 for the new setting but no upper bound so that people are free to use as high gas as they want.

What do you think @loredanacirstea @ljoss17?

[loredanacirstea]

I believe we should enforce a lower bound of 1.0 for the new setting but no upper bound so that people are free to use as high gas as they want.

@romac that's perfect. Your proposal is also consistent with how cosmos sdk uses the gas-adjustment flag - as a multiplier for the gas value.