Deprecate gas_adjustment in favor of new gas_multiplier setting (i…

…nformalsystems#2353)

# Description

This PR does the following: (cf. informalsystems#2174 (comment))

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`.

This PR enforces 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.

# Commits

* Deprecate `gas_adjustment` in favor of new `gas_multiplier` setting

* Add changelog entry

* Add tests for `adjust_gas`

* Fix `unused_variable` warning

* Improve tests

* Fix typo in test code

* Use 1.1 gas_multiplier in mock tests

* Return zero when `gas_amount` is zero, instead of max_gas

* Short-circuit when gas_amount = 0 or gas_multiplier = 1

.changelog/unreleased/breaking-changes/ibc-relayer-cli/2174-gas-multiplier.md

@@ -0,0 +1,2 @@
+- Deprecate `gas_adjustment` setting in favor of new `gas_multiplier` setting
+ ([#2174](https://github.com/informalsystems/ibc-rs/issues/2174))

config.toml

@@ -157,10 +157,16 @@ max_gas = 400000
 # the denomination of the fee. Required
 gas_price = { price = 0.001, denom = 'stake' }
 
-# Specify the ratio by which to increase the gas estimate used to compute the fee,
-# to account for potential estimation error. Default: 0.1, ie. 10%.
-# Valid range: 0.0 to 1.0 (inclusive)
-gas_adjustment = 1.0
+# Multiply this amoutn by the gas estimate used to compute the fee
+# to account for potential estimation error.
+#
+# Example: With this setting is set to 1.1, then if the estimated gas
+# is 80_000, then gas used to compute the fee will be adjusted to
+# 80_000 * 1.1 = 88_000.
+#
+# Default: 1.0, ie. the gas is not increased.
+# Minimum value: 1.0
+gas_multiplier = 1.0
 
 # Specify how many IBC messages at most to include in a single transaction.
 # Default: 30
@@ -242,7 +248,7 @@ store_prefix = 'ibc'
 default_gas = 100000
 max_gas = 400000
 gas_price = { price = 0.001, denom = 'stake' }
-gas_adjustment = 0.1
+gas_multiplier = 1.0
 max_msg_num = 30
 max_tx_size = 2097152
 clock_drift = '5s'

guide/src/commands/config.md

@@ -38,5 +38,5 @@ the same chain `ibc-1`:
 
 ```text
 hermes --config ./config.toml config validate
-error: hermes fatal error: config error: config file has duplicate entry for the chain with id ibc-1
+error: hermes fatal error: config error: config file has duplicate entry for the chain 'ibc-1'
 ```

guide/src/rest-api.md

@@ -94,7 +94,7 @@ where `:id` stands for the identififer.
  "key_name": "testkey",
  "store_prefix": "ibc",
  "max_gas": 900000000,
- "gas_adjustment": null,
+ "gas_multiplier": 1.0,
  "max_msg_num": 60,
  "max_tx_size": 2097152,
  "clock_drift": "5s",

guide/src/tutorials/local-chains/relay-paths/multiple-paths.md

@@ -40,7 +40,7 @@ Follow the steps below to connect three chains together and relay packets betwee
  store_prefix = 'ibc'
  max_gas = 2000000
  gas_price = { price = 0.001, denom = 'stake' }
- gas_adjustment = 0.1
+ gas_multiplier = 1.0
  clock_drift = '5s'
  trusting_period = '14days'
  trust_threshold = { numerator = '1', denominator = '3' }
@@ -56,7 +56,7 @@ Follow the steps below to connect three chains together and relay packets betwee
  store_prefix = 'ibc'
  max_gas = 2000000
  gas_price = { price = 0.001, denom = 'stake' }
- gas_adjustment = 0.1
+ gas_multiplier = 1.0
  clock_drift = '5s'
  trusting_period = '14days'
  trust_threshold = { numerator = '1', denominator = '3' }
@@ -72,7 +72,7 @@ Follow the steps below to connect three chains together and relay packets betwee
  store_prefix = 'ibc'
  max_gas = 2000000
  gas_price = { price = 0.001, denom = 'stake' }
- gas_adjustment = 0.1
+ gas_multiplier = 1.0
  clock_drift = '5s'
  trusting_period = '14days'
  trust_threshold = { numerator = '1', denominator = '3' }

relayer-cli/src/config.rs

@@ -9,7 +9,7 @@ use std::path::PathBuf;
 
 use flex_error::{define_error, TraceError};
 use ibc::core::ics24_host::identifier::ChainId;
-use ibc_relayer::config::{Config, ModeConfig};
+use ibc_relayer::config::{ChainConfig, Config, ModeConfig};
 use tendermint_light_client_verifier::types::TrustThreshold;
 use tracing_subscriber::filter::ParseError;
 
@@ -45,7 +45,7 @@ define_error! {
  DuplicateChains
  { chain_id: ChainId }
  |e| {
- format!("config file has duplicate entry for the chain with id {0}",
+ format!("config file has duplicate entry for the chain '{0}'",
  e.chain_id)
  },
 
@@ -56,19 +56,35 @@ define_error! {
  reason: String
  }
  |e| {
- format!("config file specifies an invalid `trust_threshold` ({0}) for the chain with id {1}, caused by: {2}",
+ format!("config file specifies an invalid `trust_threshold` ({0}) for the chain '{1}', caused by: {2}",
  e.threshold, e.chain_id, e.reason)
  },
 
- InvalidGasAdjustment
+ InvalidGasMultiplier
+ {
+ gas_multiplier: f64,
+ chain_id: ChainId,
+ }
+ |e| {
+ format!(
+ "config file specifies an invalid `gas_multiplier` ({0}) for the chain '{1}', \
+ the value must be greater than or equal to 1.0",
+ e.gas_multiplier, e.chain_id
+ )
+ },
+
+ DeprecatedGasAdjustment
  {
  gas_adjustment: f64,
+ gas_multiplier: f64,
  chain_id: ChainId,
- reason: String
  }
  |e| {
- format!("config file specifies an invalid `gas_adjustment` ({0}) for the chain with id {1}, caused by: {2}",
- e.gas_adjustment, e.chain_id, e.reason)
+ format!(
+ "config file specifies deprecated setting `gas_adjustment = {1}` for the chain '{0}'; \
+ to get the same behavior, use `gas_multiplier = {2}",
+ e.chain_id, e.gas_adjustment, e.gas_multiplier
+ )
  },
  }
 }
@@ -92,7 +108,7 @@ pub fn validate_config(config: &Config) -> Result<(), Diagnostic<Error>> {
  validate_trust_threshold(&c.id, c.trust_threshold)?;
 
  // Validate gas-related settings
- validate_gas_settings(&c.id, c.gas_adjustment)?;
+ validate_gas_settings(&c.id, c)?;
  }
 
  // Check for invalid mode config
@@ -153,18 +169,27 @@ fn validate_trust_threshold(
  Ok(())
 }
 
-fn validate_gas_settings(
- id: &ChainId,
- gas_adjustment: Option<f64>,
-) -> Result<(), Diagnostic<Error>> {
- match gas_adjustment {
- Some(gas_adjustment) if !(0.0..=1.0).contains(&gas_adjustment) => {
- Err(Diagnostic::Error(Error::invalid_gas_adjustment(
- gas_adjustment,
+fn validate_gas_settings(id: &ChainId, config: &ChainConfig) -> Result<(), Diagnostic<Error>> {
+ // Check that the gas_multiplier is greater than or equal to 1.0
+ if let Some(gas_multiplier) = config.gas_multiplier {
+ if gas_multiplier < 1.0 {
+ return Err(Diagnostic::Error(Error::invalid_gas_multiplier(
+ gas_multiplier,
  id.clone(),
- "gas adjustment must be between 0.0 and 1.0 inclusive".to_string(),
- )))
+ )));
  }
- _ => Ok(()),
  }
+
+ // Check that the gas_adjustment option is not set
+ if let Some(gas_adjustment) = config.gas_adjustment {
+ let gas_multiplier = gas_adjustment + 1.0;
+
+ return Err(Diagnostic::Error(Error::deprecated_gas_adjustment(
+ gas_adjustment,
+ gas_multiplier,
+ id.clone(),
+ )));
+ }
+
+ Ok(())
 }

relayer-cli/src/error.rs

@@ -54,7 +54,7 @@ define_error! {
  MissingChainConfig
  { chain_id: ChainId }
  | e | {
- format_args!("missing chain with id '{}' in configuration file",
+ format_args!("missing chain '{}' in configuration file",
  e.chain_id)
  },
 

relayer-rest/tests/mock.rs

@@ -102,7 +102,7 @@ key_name = 'testkey'
 store_prefix = 'ibc'
 max_gas = 3000000
 gas_price = { price = 0.001, denom = 'stake' }
-gas_adjustment = 0.1
+gas_multiplier = 1.1
 max_msg_num = 30
 max_tx_size = 2097152
 clock_drift = '5s'

relayer/src/chain/cosmos/estimate.rs

@@ -5,7 +5,7 @@ use tonic::codegen::http::Uri;
 use tracing::{debug, error, span, warn, Level};
 
 use crate::chain::cosmos::encode::sign_tx;
-use crate::chain::cosmos::gas::{gas_amount_to_fees, PrettyFee};
+use crate::chain::cosmos::gas::{gas_amount_to_fee, PrettyFee};
 use crate::chain::cosmos::simulate::send_tx_simulate;
 use crate::chain::cosmos::types::account::Account;
 use crate::chain::cosmos::types::config::TxConfig;
@@ -70,7 +70,7 @@ async fn estimate_fee_with_tx(
  ));
  }
 
- let adjusted_fee = gas_amount_to_fees(gas_config, estimated_gas);
+ let adjusted_fee = gas_amount_to_fee(gas_config, estimated_gas);
 
  debug!(
  id = %chain_id,

relayer/src/chain/cosmos/gas.rs

@@ -10,8 +10,12 @@ use crate::config::GasPrice;
 
 pub struct PrettyFee<'a>(pub &'a Fee);
 
-pub fn gas_amount_to_fees(config: &GasConfig, gas_amount: u64) -> Fee {
- let adjusted_gas_limit = adjust_gas_with_simulated_fees(config, gas_amount);
+pub fn gas_amount_to_fee(config: &GasConfig, gas_amount: u64) -> Fee {
+ let adjusted_gas_limit = adjust_estimated_gas(AdjustGas {
+ gas_multiplier: config.gas_multiplier,
+ max_gas: config.max_gas,
+ gas_amount,
+ });
 
  // The fee in coins based on gas amount
  let amount = calculate_fee(adjusted_gas_limit, &config.gas_price);
@@ -42,21 +46,59 @@ pub fn mul_ceil(a: u64, f: f64) -> BigInt {
  (f * a).ceil().to_integer()
 }
 
-/// Adjusts the fee based on the configured `gas_adjustment` to prevent out of gas errors.
+/// Multiply `a` with `f` and round the result down to the nearest integer.
+pub fn mul_floor(a: u64, f: f64) -> BigInt {
+ assert!(f.is_finite());
+
+ let a = BigInt::from(a);
+ let f = BigRational::from_float(f).expect("f is finite");
+ (f * a).floor().to_integer()
+}
+
+struct AdjustGas {
+ gas_multiplier: f64,
+ max_gas: u64,
+ gas_amount: u64,
+}
+
+/// Adjusts the fee based on the configured `gas_multiplier` to prevent out of gas errors.
 /// The actual gas cost, when a transaction is executed, may be slightly higher than the
 /// one returned by the simulation.
-fn adjust_gas_with_simulated_fees(config: &GasConfig, gas_amount: u64) -> u64 {
- let gas_adjustment = config.gas_adjustment;
+fn adjust_estimated_gas(
+ AdjustGas {
+ gas_multiplier,
+ max_gas,
+ gas_amount,
+ }: AdjustGas,
+) -> u64 {
+ assert!(gas_multiplier >= 1.0);
+
+ // No need to compute anything if the gas amount is zero
+ if gas_amount == 0 {
+ return 0;
+ };
+
+ // If the multiplier is 1, no need to perform the multiplication
+ if gas_multiplier == 1.0 {
+ return min(gas_amount, max_gas);
+ }
 
- assert!(gas_adjustment <= 1.0);
+ // Multiply the gas estimate by the gas_multiplier option
+ let (_sign, digits) = mul_floor(gas_amount, gas_multiplier).to_u64_digits();
 
- let (_, digits) = mul_ceil(gas_amount, gas_adjustment).to_u64_digits();
- assert!(digits.len() == 1);
+ let gas = match digits.as_slice() {
+ // If there are no digits it means that the resulting amount is zero.
+ [] => 0,
 
- let adjustment = digits[0];
- let gas = gas_amount.checked_add(adjustment).unwrap_or(u64::MAX);
+  // If there is a single "digit", it means that the result fits in a u64, so we can use that.
+  [gas] => *gas,
 
- min(gas, config.max_gas)
+ // Otherwise, the multiplication overflow and we use u64::MAX instead.
+ _ => u64::MAX,
+ };
+
+ // Bound the gas estimate by the max_gas option
+ min(gas, max_gas)
 }
 
 impl fmt::Display for PrettyFee<'_> {
@@ -72,3 +114,63 @@ impl fmt::Display for PrettyFee<'_> {
  .finish()
  }
 }
+
+#[cfg(test)]
+mod tests {
+ use super::{adjust_estimated_gas, AdjustGas};
+
+ #[test]
+ fn adjust_zero_gas() {
+ let adjusted_gas = adjust_estimated_gas(AdjustGas {
+ gas_multiplier: 1.1,
+ max_gas: 1_000_000,
+ gas_amount: 0,
+ });
+
+ assert_eq!(adjusted_gas, 0);
+ }
+
+ #[test]
+ fn adjust_gas_one() {
+ let adjusted_gas = adjust_estimated_gas(AdjustGas {
+ gas_multiplier: 1.0,
+ max_gas: 1_000_000,
+ gas_amount: 400_000,
+ });
+
+ assert_eq!(adjusted_gas, 400_000);
+ }
+
+ #[test]
+ fn adjust_gas_small() {
+ let adjusted_gas = adjust_estimated_gas(AdjustGas {
+ gas_multiplier: 1.1,
+ max_gas: 1_000_000,
+ gas_amount: 400_000,
+ });
+
+ assert_eq!(adjusted_gas, 440_000);
+ }
+
+ #[test]
+ fn adjust_gas_over_max() {
+ let adjusted_gas = adjust_estimated_gas(AdjustGas {
+ gas_multiplier: 3.0,
+ max_gas: 1_000_000,
+ gas_amount: 400_000,
+ });
+
+ assert_eq!(adjusted_gas, 1_000_000);
+ }
+
+ #[test]
+ fn adjust_gas_overflow() {
+ let adjusted_gas = adjust_estimated_gas(AdjustGas {
+ gas_multiplier: 3.0,
+ max_gas: u64::MAX,
+ gas_amount: u64::MAX / 2,
+ });
+
+ assert_eq!(adjusted_gas, u64::MAX);
+ }
+}