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

Ensure all events in a batch are processed, even when it contains events for unknown chains #4022

Merged
merged 6 commits into from
Jun 10, 2024
Merged

Ensure all events in a batch are processed, even when it contains events for unknown chains #4022

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6364535
Add a filter at supervisor level for CCQ to verify if the destination…
ljoss17 Jun 7, 2024
0e0965d
Update all MSRVs to v1.76.0
ljoss17 Jun 7, 2024
f9f4a88
Add changelog entry
ljoss17 Jun 7, 2024
926ce1b
Renaming
romac Jun 7, 2024
e3ef237
Move changelog entry under bug fixes
romac Jun 7, 2024
6592625
Ensure we process all events in a batch, even when some of them are r…
romac Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changelog/unreleased/bug-fixes/ibc-relayer/4021-chain-level-ccq-filter.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Discard CrossChain queries intended for unconfigured chains.
([\#4021](https://github.com/informalsystems/hermes/issues/4021))
2 changes: 1 addition & 1 deletion crates/chain-registry/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ license = "Apache-2.0"
keywords = ["cosmos", "ibc", "relayer", "chain", "registry"]
repository = "https://github.com/informalsystems/hermes"
authors = ["Informal Systems <hello@informal.systems>"]
rust-version = "1.71.1"
rust-version = "1.76.0"
description = """
Service to fetch data from the chain-registry
"""
Expand Down
2 changes: 1 addition & 1 deletion crates/relayer-rest/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ readme = "README.md"
keywords = ["ibc", "rest", "api", "cosmos", "tendermint"]
homepage = "https://hermes.informal.systems/"
repository = "https://github.com/informalsystems/hermes"
rust-version = "1.71.1"
rust-version = "1.76.0"
description = """
Rust implementation of a RESTful API server for Hermes
"""
Expand Down
4 changes: 2 additions & 2 deletions crates/relayer-rest/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
[![End to End testing][e2e-image]][e2e-link]
[![Apache 2.0 Licensed][license-image]][license-link]
![Rust Stable][rustc-image]
![Rust 1.71.1+][rustc-version]
![Rust 1.76.0+][rustc-version]

This is the repository for the IBC REST server for use in the Hermes IBC relayer.

Expand Down Expand Up @@ -39,4 +39,4 @@ Unless required by applicable law or agreed to in writing, software distributed
[license-image]: https://img.shields.io/badge/license-Apache2.0-blue.svg
[license-link]: https://github.com/informalsystems/hermes/blob/master/LICENSE
[rustc-image]: https://img.shields.io/badge/rustc-stable-blue.svg
[rustc-version]: https://img.shields.io/badge/rustc-1.71.1+-blue.svg
[rustc-version]: https://img.shields.io/badge/rustc-1.76.0+-blue.svg
2 changes: 1 addition & 1 deletion crates/relayer-types/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ readme = "README.md"
keywords = ["blockchain", "consensus", "cosmos", "ibc", "tendermint"]
repository = "https://github.com/informalsystems/hermes"
authors = ["Informal Systems <hello@informal.systems>"]
rust-version = "1.71.1"
rust-version = "1.76.0"
description = """
Implementation of the Inter-Blockchain Communication Protocol (IBC).
This crate comprises the main data structures and on-chain logic.
Expand Down
2 changes: 1 addition & 1 deletion crates/relayer/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ readme = "README.md"
keywords = ["blockchain", "consensus", "cosmos", "ibc", "tendermint"]
repository = "https://github.com/informalsystems/hermes"
authors = ["Informal Systems <hello@informal.systems>"]
rust-version = "1.71.1"
rust-version = "1.76.0"
description = """
Implementation of an IBC Relayer in Rust, as a library
"""
Expand Down
13 changes: 13 additions & 0 deletions crates/relayer/src/object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ use crate::chain::{
handle::ChainHandle,
requests::{IncludeProof, QueryClientStateRequest, QueryHeight},
};
use crate::config::ChainConfig;
use crate::error::Error as RelayerError;
use crate::supervisor::client_state_filter::Permission;
use crate::supervisor::Error as SupervisorError;

/// Client
Expand Down Expand Up @@ -145,6 +147,17 @@ impl CrossChainQuery {
pub fn short_name(&self) -> String {
format!("cross_chain_query::{}/{}", self.dst_chain_id, self.query_id)
}

pub fn intended_for_known_dst_chain(&self, chain_configs: &[ChainConfig]) -> Permission {
romac marked this conversation as resolved.
Show resolved Hide resolved
if chain_configs
.iter()
.any(|config| *config.id() == self.dst_chain_id)
{
Permission::Allow
} else {
Permission::Deny
}
}
}

/// An object determines the amount of parallelism that can
Expand Down
4 changes: 3 additions & 1 deletion crates/relayer/src/supervisor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,13 +374,15 @@ fn relay_on_object<Chain: ChainHandle>(
};

// Then, apply the client filter
// If the object is a CrossChain query discard it if the destination chain
// is not configured
let client_filter_outcome = match object {
Object::Client(client) => client_state_filter.control_client_object(registry, client),
Object::Connection(conn) => client_state_filter.control_conn_object(registry, conn),
Object::Channel(chan) => client_state_filter.control_chan_object(registry, chan),
Object::Packet(packet) => client_state_filter.control_packet_object(registry, packet),
Object::CrossChainQuery(ccq) => Ok(ccq.intended_for_known_dst_chain(&config.chains)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why we didn't implement something like client_state_filter.control_query_object() here in the past?

Copy link
Member

Choose a reason for hiding this comment

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

An oversight. We could also implement that check there but right it's so simple that it does not require the whole filtering machinery.

Copy link
Collaborator

@ancazamfir ancazamfir Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sure but the added check is not really a "client_filter_outcome". Not sure if we tested the scenario with this PR branch in the issue but it looks like if the chain is not configured we will return an error from relay_on_object() and print a misleading trace.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to my other comment, why can't we do the config check for object.dst_chain_id() before relay_on_object call ?
Then do proper client filtering in relay_on_object and also fix that trace message.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another thing, without the fix I think we hit this line and we exit without processing the other events in the batch, maybe we need to log an error and continue.

Copy link
Member

Choose a reason for hiding this comment

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

Related to my other comment, why can't we do the config check for object.dst_chain_id() before relay_on_object call ?
Then do proper client filtering in relay_on_object and also fix that trace message

Agreed, seems like the best way to go

Copy link
Member

@romac romac Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ancazamfir What do you think of this patch? It should address all the problems you've raised.

Show diff 
diff --git a/crates/relayer/src/object.rs b/crates/relayer/src/object.rs
index 4716a5d5c..aac3efb25 100644
--- a/crates/relayer/src/object.rs
+++ b/crates/relayer/src/object.rs
@@ -21,9 +21,7 @@ use crate::chain::{
     handle::ChainHandle,
     requests::{IncludeProof, QueryClientStateRequest, QueryHeight},
 };
-use crate::config::ChainConfig;
 use crate::error::Error as RelayerError;
-use crate::supervisor::client_state_filter::Permission;
 use crate::supervisor::Error as SupervisorError;
 
 /// Client
@@ -147,17 +145,6 @@ impl CrossChainQuery {
     pub fn short_name(&self) -> String {
         format!("cross_chain_query::{}/{}", self.dst_chain_id, self.query_id)
     }
-
-    pub fn intended_for_known_dst_chain(&self, chain_configs: &[ChainConfig]) -> Permission {
-        if chain_configs
-            .iter()
-            .any(|config| *config.id() == self.dst_chain_id)
-        {
-            Permission::Allow
-        } else {
-            Permission::Deny
-        }
-    }
 }
 
 /// An object determines the amount of parallelism that can
diff --git a/crates/relayer/src/supervisor.rs b/crates/relayer/src/supervisor.rs
index 9a5b0f17a..478aecb84 100644
--- a/crates/relayer/src/supervisor.rs
+++ b/crates/relayer/src/supervisor.rs
@@ -381,7 +381,7 @@ fn relay_on_object<Chain: ChainHandle>(
         Object::Connection(conn) => client_state_filter.control_conn_object(registry, conn),
         Object::Channel(chan) => client_state_filter.control_chan_object(registry, chan),
         Object::Packet(packet) => client_state_filter.control_packet_object(registry, packet),
-        Object::CrossChainQuery(ccq) => Ok(ccq.intended_for_known_dst_chain(&config.chains)),
+        Object::CrossChainQuery(_ccq) => Ok(Permission::Allow),
         Object::Wallet(_wallet) => Ok(Permission::Allow),
     };
 
@@ -816,8 +816,33 @@ fn process_batch<Chain: ChainHandle>(
         workers.notify_new_block(&src_chain.id(), batch.height, new_block);
     }
 
-    // Forward the IBC events.
+    // Forward the IBC events to the appropriate workers
     for (object, events_with_heights) in collected.per_object.into_iter() {
+        if events_with_heights.is_empty() {
+            // Event batch is empty, nothing to do
+            continue;
+        }
+
+        let Ok(src_chain) = registry.get_or_spawn(object.src_chain_id()) else {
+            trace!(
+                "skipping events for '{}': source chain '{}' is not registered",
+                object.short_name(),
+                object.src_chain_id()
+            );
+
+            continue;
+        };
+
+        let Ok(dst_chain) = registry.get_or_spawn(object.dst_chain_id()) else {
+            trace!(
+                "skipping events for '{}': destination chain '{}' is not registered",
+                object.short_name(),
+                object.src_chain_id()
+            );
+
+            continue;
+        };
+
         if !relay_on_object(
             config,
             registry,
@@ -826,32 +851,23 @@ fn process_batch<Chain: ChainHandle>(
             &object,
         ) {
             trace!(
-                "skipping events for '{}'. \
-                reason: filtering is enabled and channel does not match any allowed channels",
+                "skipping events for '{}': filtering is enabled and channel does not match any allowed channels",
                 object.short_name()
             );
 
             continue;
         }
 
-        if events_with_heights.is_empty() {
-            continue;
-        }
-
-        let src = registry
-            .get_or_spawn(object.src_chain_id())
-            .map_err(Error::spawn)?;
-
-        let dst = registry
-            .get_or_spawn(object.dst_chain_id())
-            .map_err(Error::spawn)?;
-
         if let Object::Packet(ref _path) = object {
-            // Update telemetry info
-            telemetry!(send_telemetry(&src, &dst, &events_with_heights, _path));
+            telemetry!(send_telemetry(
+                &src_chain,
+                &dst_chain,
+                &events_with_heights,
+                _path
+            ));
         }
 
-        let worker = workers.get_or_spawn(object, src, dst, config);
+        let worker = workers.get_or_spawn(object, src_chain, dst_chain, config);
 
         worker.send_events(
             batch.height,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks great! The skipping events trace message should be more general imo, it's not only channel filters that can dictate an event should be ignored, it's filtering in general.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps also change the issue/PR to make it more general as it applies to all events

Copy link
Member

Choose a reason for hiding this comment

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

Created a new issue and added a new changelog entry

Object::Wallet(_wallet) => Ok(Permission::Allow),
Object::CrossChainQuery(_) => Ok(Permission::Allow),
};

match client_filter_outcome {
Expand Down
2 changes: 1 addition & 1 deletion crates/telemetry/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ readme = "README.md"
keywords = ["cosmos", "ibc", "relayer", "telemetry"]
repository = "https://github.com/informalsystems/hermes"
authors = ["Informal Systems <hello@informal.systems>"]
rust-version = "1.71.1"
rust-version = "1.76.0"
description = """
Telemetry service for the Hermes IBC relayer
"""
Expand Down
2 changes: 1 addition & 1 deletion tools/integration-test/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
name = "ibc-integration-test"
version = "0.28.0"
edition = "2021"
rust-version = "1.71.1"
rust-version = "1.76.0"
license = "Apache-2.0"
readme = "README.md"
keywords = ["blockchain", "consensus", "cosmos", "ibc", "tendermint"]
Expand Down
Loading