Handle some gas corner-cases #2537

ljoss17 · 2022-08-08T10:45:27Z

Closes: #2487

Description

Error 1: "Too much gas wanted"

For this issue Hermes report has been changed from "success" to "submitted" to avoid confusion.

Error 2: "out of gas in location" during `CheckTx`

A warning during the health-check is displayed if the gas_multiplier is lower than 1.1

Error 3: "out of gas in location" during DeliverTx

~~The interval between client refresh calls has been changed to be the client's trusting_period/100~~. In addition, the client refresh method has been updated in order to catch ChainError resulting events. The interval between refresh() calls which fail to update the client increases using the Fibonacci sequence.

PR author checklist:

Added changelog entry, using unclog.
Added tests: integration (for Hermes) or unit/mock tests (for modules).
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).

…bc-rs into 2487_gas_fixes

…nerror IbcEvent retrieved from submitting a client update

relayer/src/error.rs

adizere · 2022-08-11T09:26:20Z

relayer/src/foreign_client.rs

+                if let Some(ibc_events) = res.clone() {
+                    match ibc_events
+                        .into_iter()
+                        .find(|e| e.event_type() == IbcEventType::ChainError)


We should document the assumption we're making.

Suggested change

.find(|e| e.event_type() == IbcEventType::ChainError)

// We make the assumption that we push a single `ChainError` variant

.find(|e| e.event_type() == IbcEventType::ChainError)

Or, alternatively, we could have strong type guarantees on this. CC @soareschen @romac

Like @soareschen, I think we should get rid of these ChainError events altogether and figure out a better way to bubble up such errors.

Tracked in #2565

adizere · 2022-08-11T09:30:55Z

relayer/src/foreign_client.rs

+                                self.dst_chain().id(),
+                                ev,
+                            ));
+                        }


There are many different execution branches and it's not clear what are the post-conditions of the refresh method. Can we briefly describe that?

Let's maybe also put a comment to describe what is this match arm for. There is also an Ok(res) and a Err(e branches.

relayer/src/worker/client.rs

adizere · 2022-08-11T09:40:04Z

relayer/src/worker/client.rs

@@ -14,6 +14,9 @@ use crate::{

 use super::WorkerCmd;

+const TRUSTING_PERIOD_DIVIDER: u32 = 100;


It seems like for Tendermint clients we would refresh every ~3 hours if the trusting period is 18 days. This seems a bit too infrequest. We should retry more frequently.

adizere · 2022-08-11T09:46:53Z

The interval between client refresh calls has been changed to be the client's trusting_period/100. In addition, the client refresh method has been updated in order to catch ChainError resulting events.

Notes from discussing with Luca:

It seems that there are 3 cases:

no update required at the moment -> the client worker can backoff with a large window eg several hours, a fraction of the refresh_period
successful update -> similar to case 1, but the refresh window could be even larger, still a fraction of refresh_period
failed to update -> the client worker should retry aggressively; if the errors keep coming up (such as it is signaled in Hermes corner-cases related to gas #2487 error (3)), then the backoff between retries can be increased (eg Fibonacci or linear increase, but not constant as it currently is).

relayer/src/worker/client.rs

Co-authored-by: Adi Seredinschi <[email protected]>

…bc-rs into 2487_gas_fixes

…to same file

soareschen · 2022-08-16T09:17:16Z

@ljoss17 your changes are causing the integration tests to hang indefinitely. Can you merge #2567 to your branch and also fix the tests?

…ent delay only if 'refresh()' fails

…sation

mzabaluev · 2022-08-19T07:40:05Z

relayer-cli/src/config.rs

-    // 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(


Take care to also remove the Error enum variant if it is no longer in use.

Oh thank you, I forgot to remove it.

mzabaluev · 2022-08-19T07:40:56Z

relayer/src/chain/cosmos/gas.rs

-    assert!(gas_multiplier >= 1.0);
-


I'd rather keep it here, just in case someone removes the checks in a refactoring.

The assert! was replaced so that the test, in client_refresh.rs, using a gas_multiplier = 0.8 to trigger a failed refresh() runs correctly. The checks are now in the new GasMultiplier struct:
https://github.com/informalsystems/ibc-rs/blob/5c522c4d72659ceec7b81ee31ecc3dddf9460700/relayer/src/config/gas_multiplier.rs#L18-L27
I am not sure if there is a way to easily reproduce the failure while keeping a gas_multiplier >= 1.0.

mzabaluev · 2022-08-19T07:41:23Z

relayer/src/chain/cosmos/types/gas.rs

-fn gas_multiplier_from_config(config: &ChainConfig) -> f64 {
-    config.gas_multiplier.unwrap_or(DEFAULT_GAS_MULTIPLIER)
+pub fn gas_multiplier_from_config(config: &ChainConfig) -> f64 {
+    config.gas_multiplier.unwrap_or_default().to_f64()


mzabaluev · 2022-08-19T07:42:49Z

relayer/src/config/types.rs

@@ -258,6 +258,86 @@ pub mod memo {
    }
 }

+pub use gas_multiplier::GasMultiplier;
+
+pub mod gas_multiplier {


I think it's better to create a separate module file, this is quite long already for an inline module.

relayer/src/config/types.rs

mzabaluev · 2022-08-19T07:50:30Z

relayer/src/error.rs

@@ -459,6 +459,15 @@ define_error! {
                    e.chain_id, e.default_gas, e.max_gas)
            },

+        ConfigValidationGasMultiplierLow


There are many ConfigValidationThisAndThat variants in this enum. Prefixing in Rust usually means we should refactor things in a more elegant and concise way.

I wonder if there is a ConfigValidationError enum hiding here, which would make one variant of relayer::Error and be also convertible to that with a From impl. Maybe not for this PR, though.

I will open an issue for this

mzabaluev · 2022-08-19T07:58:31Z

relayer/src/foreign_client.rs

+            Ok(res) => {
+                // If elapsed < refresh_window for the client, `try_refresh()` will
+                // be successful with an empty vector.
+                if let Some(ibc_events) = res.clone() {


I think you could avoid cloning the whole collection here: if it destructures to Some(ibc_events), you already own it, if it's None, you don't need the res variable to return Ok(None). Can also use convenience methods here like Option::map.

I see, so I removed the clone() in commit 47a49b9, but I am not sure I understand how Option::map could be used here. Would the idea be to apply the .map() to res in order to avoid the Ok(None) when res is None?

Yes, see if res.map(|ibc_events| { ... }) could be more elegant here.

… enum in 'relayer-cli/src/config.rs'

relayer/src/config/gas_multiplier.rs

mzabaluev · 2022-08-25T10:36:00Z

relayer/src/foreign_client.rs

@@ -787,17 +787,17 @@ impl<DstChain: ChainHandle, SrcChain: ChainHandle> ForeignClient<DstChain, SrcCh
            Ok(res) => {
                // If elapsed < refresh_window for the client, `try_refresh()` will
                // be successful with an empty vector.
-                if let Some(ibc_events) = res.clone() {
+                if let Some(ibc_events) = &res {


This is better, but I think you can just destructure res since this scope owns it and it's not used anywhere else.

Suggested change

if let Some(ibc_events) = &res {

if let Some(ibc_events) = res {

So the issue I am having is that res is used at this line.

https://github.com/informalsystems/ibc-rs/blob/47a49b95c32b5cbf481d67d897b8ddba9fc51e6a/relayer/src/foreign_client.rs#L804

In order to have a more elegant refresh() method, a part of it was extracted in a function.

I think you can rewrite refresh as follows to avoid such nesting:

pub fn refresh(&mut self) -> Result<Option<Vec<IbcEvent>>, ForeignClientError> { match self.try_refresh() { // If elapsed < refresh_window for the client, `try_refresh()` will // be successful with an empty vector. Ok(Some(ibc_events)) => Self::verify_chain_error(ibc_events, self.dst_chain().id()), // Return the successful empty vector. Ok(None) => Ok(None), // Return the error Err(e) => Err(e), } }

or by using the ? operator on the result of try_refresh:

pub fn refresh(&mut self) -> Result<Option<Vec<IbcEvent>>, ForeignClientError> { // If elapsed < refresh_window for the client, `try_refresh()` will // be successful with an empty vector. match self.try_refresh()? { Some(events) => Self::verify_chain_error(events, self.dst_chain().id()), None => Ok(None), } }

or even like this, but I find the latter harder to read:

pub fn refresh(&mut self) -> Result<Option<Vec<IbcEvent>>, ForeignClientError> { // If elapsed < refresh_window for the client, `try_refresh()` will // be successful with an empty vector. self.try_refresh().and_then(|opt_events| { opt_events .map(|events| Self::verify_chain_error(events, self.dst_chain().id())) .transpose() .map(Option::flatten) }) }

Another alternative is to have verify_chain_error (renamed below to check_no_errors) return a Result<(), _>:

pub fn refresh(&mut self) -> Result<Option<Vec<IbcEvent>>, ForeignClientError> { fn check_no_errors( ibc_events: &[IbcEvent], dst_chain_id: ChainId, ) -> Result<(), ForeignClientError> { // The assumption is that only one IbcEventType::ChainError will be // in the resulting Vec<IbcEvent> if an error occurred. let chain_error = ibc_events .iter() .find(|&e| e.event_type() == IbcEventType::ChainError); match chain_error { None => Ok(()), Some(ev) => Err(ForeignClientError::chain_error_event( dst_chain_id, ev.to_owned(), )), } } // If elapsed < refresh_window for the client, `try_refresh()` will // be successful with an empty vector. if let Some(events) = self.try_refresh()? { check_no_errors(&events, self.dst_chain().id())?; Ok(Some(events)) } else { Ok(None) } }

I think I like the latter solution the best, as it seems a bit weird to me to have veriy_chain_errors return an Option<Vec<_>> where the Option will always be Some, ie. the signature does not really tell us what is going on, and could also have us think that the events that are returned are not the very same than the one which are passed in.

What do you think?

The proposed solutions are great, and I agree that the last one is better, as the check_no_errors makes it more understandable.

relayer/src/foreign_client.rs

mzabaluev

Looks good to me.

adizere and others added 10 commits August 4, 2022 11:37

A stab at #2487

7ad5084

A stab at #2487

634fb08

Merge branch '2487_gas_fixes' of https://github.com/informalsystems/i…

ac7a9aa

…bc-rs into 2487_gas_fixes

Updated interval between client refresh from 1 second to 5 seconds

aae6868

Updated refresh in order to return ignorable error if there is a chai…

3137a64

…nerror IbcEvent retrieved from submitting a client update

Clippy and fmt

32c1f5f

Updated interval between client refresh to depend on the trusting period

0c5a177

fmt

b225c71

Merge branch 'master' into 2487_gas_fixes

00ad6e7

Added changelog entry

0dd3fe1

adizere mentioned this pull request Aug 11, 2022

Add option packet-data-query-height to Hermes packet-recv CLI #2512

Merged

7 tasks

adizere reviewed Aug 11, 2022

View reviewed changes

relayer/src/error.rs Outdated Show resolved Hide resolved

adizere reviewed Aug 11, 2022

View reviewed changes

relayer/src/worker/client.rs Outdated Show resolved Hide resolved

adizere reviewed Aug 11, 2022

View reviewed changes

adizere reviewed Aug 12, 2022

View reviewed changes

relayer/src/worker/client.rs Outdated Show resolved Hide resolved

ljoss17 and others added 10 commits August 15, 2022 08:46

Apply suggestions from code review

9c9040f

Co-authored-by: Adi Seredinschi <[email protected]>

Merge branch '2487_gas_fixes' of https://github.com/informalsystems/i…

af17334

…bc-rs into 2487_gas_fixes

Merge branch 'master' into 2487_gas_fixes

7d4314e

Added new 'GasMultiplier' type for configuration. Fixed clippy errors

b77ec3e

cargo fmt

17b3840

Fixed error in relayer-cli 'gas_multiplier' validation

49050ae

Added integration test for client 'refresh()'

e47ad36

Refactor override code to override_connected_chains

06cfdcd

Documented client refresh integration test and moved related methods …

2b3cbab

…to same file

Add timeout limit to CI tests

59c357d

Merge branch 'master' into 2487_gas_fixes

ca7ca04

ljoss17 added 3 commits August 16, 2022 11:20

Merge branch 'soares/ci-timeout' into 2487_gas_fixes

f59e9b9

Improved GasMultiplier type. Updated 'spawn_refresh_client' to increm…

8aa2877

…ent delay only if 'refresh()' fails

Clippy fix

d8a831a

ljoss17 requested a review from soareschen August 16, 2022 12:51

ljoss17 marked this pull request as ready for review August 16, 2022 12:51

ljoss17 added 3 commits August 16, 2022 14:51

Merge branch 'master' into 2487_gas_fixes

8014545

Removed 'gas_multiplier' validation, as this is done during deseriali…

2dcb204

…sation

Removed outdated comment

ddfec74

ljoss17 added this to the v1.1 milestone Aug 18, 2022

romac marked this pull request as draft August 18, 2022 15:12

mzabaluev suggested changes Aug 19, 2022

View reviewed changes

Moved GasMultiplier to 'gas_multiplier.rs' file. Removed unused Error…

5c522c4

… enum in 'relayer-cli/src/config.rs'

seanchen1991 marked this pull request as ready for review August 22, 2022 15:45

ljoss17 mentioned this pull request Aug 23, 2022

Refactor configuration validation errors #2589

Open

14 tasks

ljoss17 and others added 2 commits August 24, 2022 15:34

Merge branch 'master' into 2487_gas_fixes

64b16d6

Removed cloning the vector of 'IbcEvent' in 'refresh()'

47a49b9

mzabaluev reviewed Aug 25, 2022

View reviewed changes

mzabaluev self-requested a review August 25, 2022 10:51

ljoss17 added 5 commits August 26, 2022 12:04

Cleanup 'refresh()' method

ffa0bbd

Merge branch 'master' into 2487_gas_fixes

f949690

Fixed issue with new GasMultiplier struct in chain registry

6cf2fc7

Cargo fmt

b596d66

Updated 'refresh()' method following comment

e82c9d9

mzabaluev approved these changes Aug 29, 2022

View reviewed changes

romac merged commit c1a5fe7 into master Aug 29, 2022

romac deleted the 2487_gas_fixes branch August 29, 2022 15:00

hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022

Handle some gas corner-cases (informalsystems#2537)

6fddef8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle some gas corner-cases #2537

Handle some gas corner-cases #2537

ljoss17 commented Aug 8, 2022 •

edited

Loading

adizere Aug 11, 2022

adizere Aug 11, 2022

romac Aug 18, 2022

romac Aug 18, 2022

adizere Aug 11, 2022

adizere Aug 11, 2022

adizere commented Aug 11, 2022

soareschen commented Aug 16, 2022

mzabaluev Aug 19, 2022

ljoss17 Aug 23, 2022

mzabaluev Aug 19, 2022

ljoss17 Aug 23, 2022

mzabaluev Aug 19, 2022

mzabaluev Aug 19, 2022

mzabaluev Aug 19, 2022

ljoss17 Aug 23, 2022

mzabaluev Aug 19, 2022

ljoss17 Aug 24, 2022

mzabaluev Aug 25, 2022

mzabaluev Aug 25, 2022

ljoss17 Aug 26, 2022

romac Aug 26, 2022

romac Aug 26, 2022

romac Aug 26, 2022 •

edited

Loading

ljoss17 Aug 26, 2022

mzabaluev left a comment

	.find(\|e\| e.event_type() == IbcEventType::ChainError)
	// We make the assumption that we push a single `ChainError` variant
	.find(\|e\| e.event_type() == IbcEventType::ChainError)

		@@ -14,6 +14,9 @@ use crate::{

		use super::WorkerCmd;

		const TRUSTING_PERIOD_DIVIDER: u32 = 100;

	if let Some(ibc_events) = &res {
	if let Some(ibc_events) = res {

Handle some gas corner-cases #2537

Handle some gas corner-cases #2537

Conversation

ljoss17 commented Aug 8, 2022 • edited Loading

Description

Error 1: "Too much gas wanted"

Error 2: "out of gas in location" during CheckTx

Error 3: "out of gas in location" during DeliverTx

PR author checklist:

Reviewer checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adizere commented Aug 11, 2022

soareschen commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev left a comment

Choose a reason for hiding this comment

ljoss17 commented Aug 8, 2022 •

edited

Loading

Error 2: "out of gas in location" during `CheckTx`

romac Aug 26, 2022 •

edited

Loading