Change config format to scope configs by type #3644

hdevalence · 2023-10-01T02:50:18Z

Closes: #3636

Description

This turns out to be a very good place to start to discover requirements for Hermes to operate in a more multi-chain way, since it allows discovering which parts of the code access Cosmos-specific data to do Cosmos-specific logic, and which parts are required for general relaying.

The only change to the existing config format so far is the change in semantics of the type field, which now must be CosmosSdk. I did not preserve the existing logic and tests that parsing different variations of casing and dashes are supported, because I don't think this is particularly valuable to support.

So far, only the core ibc-relayer crate is updated; the cli is not, because it's not necessarily clear how the cli should change to accomodate non-SDK chains.

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

Work towards informalsystems#3636 This turns out to be a very good place to start to discover requirements for Hermes to operate in a more multi-chain way, since it allows discovering which parts of the code access Cosmos-specific data to do Cosmos-specific logic, and which parts are required for general relaying. The only change to the existing config format so far is the change in semantics of the `type` field, which now must be `CosmosSdk`. I did not preserve the existing logic and tests that parsing different variations of casing and dashes are supported, because I don't think this is particularly valuable to support. So far, only the core `ibc-relayer` crate is updated; the cli is not, because it's not necessarily clear how the cli should change to accomodate non-SDK chains.

hdevalence · 2023-10-01T02:52:31Z

crates/relayer/src/chain.rs

-#[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize)]
-/// Types of chains the relayer can relay to and from
-pub enum ChainType {
-    /// Chains based on the Cosmos SDK
-    CosmosSdk,
-}


This is gone entirely. Rather than having the config store an enum of chain types, the config is an enum, so that it can have different subtypes for different configs.

hdevalence · 2023-10-01T02:55:06Z

crates/relayer/src/chain/client.rs

+        // TODO: extract Tendermint-related configs into a separate substructure
+        // that can be used both by CosmosSdkConfig and configs for nonSDK chains.


It seems like the right move here is to split out the Tendermint-related fields on the config into a Tendermint-specific config structure. Then that structure can be a field on both CosmosSdkConfig and other Tendermint-using chains (like Penumbra or Namada).

Sounds good!

hdevalence · 2023-10-01T02:55:45Z

crates/relayer/src/chain/cosmos.rs

    fn bootstrap(config: ChainConfig, rt: Arc<TokioRuntime>) -> Result<Self, Error> {
+        #[allow(irrefutable_let_patterns)]


(Currently, the pattern is irrefutable, but it won't be in the future, and that should be handled now)

hdevalence · 2023-10-01T02:56:08Z

crates/relayer/src/chain/cosmos.rs

+    fn get_key(&mut self) -> Result<Self::SigningKeyPair, Error> {
+        // Get the key from key seed file
+        let key_pair = self
+            .keybase()
+            .get_key(&self.config.key_name)
+            .map_err(|e| Error::key_not_found(self.config().key_name.clone(), e))?;
+
+        Ok(key_pair)
+    }


hdevalence · 2023-10-01T02:57:09Z

crates/relayer/src/config.rs

@@ -31,7 +31,7 @@ use ibc_relayer_types::core::ics23_commitment::specs::ProofSpecs;
 use ibc_relayer_types::core::ics24_host::identifier::{ChainId, ChannelId, PortId};
 use ibc_relayer_types::timestamp::ZERO_DURATION;

-use crate::chain::ChainType;
+//use crate::chain::ChainType;


oops, will clean up after next round of discussion

hdevalence · 2023-10-01T02:57:21Z

crates/relayer/src/config.rs

+    /*
    pub fn chain_type() -> ChainType {
        ChainType::CosmosSdk
    }
+     */


ditto above cleanup comment

hdevalence · 2023-10-01T02:59:59Z

crates/relayer/src/config.rs

+#[serde(tag = "type")]
+pub enum ChainConfig {
+    CosmosSdk(CosmosSdkConfig),
+}


As mentioned in the issue description for #3636, this will produce TOML like

[[chains]] type = "CosmosSdk" id = "chain_id" rpc_addr = "http://127.0.0.1:26657/" grpc_addr = "http://127.0.0.1:8080/"

The only difference from the current format is that the type field is now mandatory, and it must be CosmosSdk, not cosmos-sdk or cOsMo-sSdk or any other variation. I don't think that supporting arbitrary variations is a worthwhile complexity vs ergonomics tradeoff.

I don't think that supporting arbitrary variations is a worthwhile complexity vs ergonomics tradeoff.

Agreed! Looks good otherwise!

hdevalence · 2023-10-01T03:07:57Z

crates/relayer/src/config.rs

+impl ChainConfig {
+    pub fn id(&self) -> &ChainId {
+        match self {
+            Self::CosmosSdk(config) => &config.id,
+        }
+    }
+
+    pub fn packet_filter(&self) -> &PacketFilter {
+        match self {
+            Self::CosmosSdk(config) => &config.packet_filter,
+        }
+    }
+
+    pub fn max_block_time(&self) -> Duration {
+        match self {
+            Self::CosmosSdk(config) => config.max_block_time,
+        }
+    }
+}


These are the attributes that are used by Hermes outside of the Cosmos backend.

All of the other attributes are only used inside the Cosmos backend, so this is effectively a way to discover which parts of the configuration data is used by the relayer engine, and which parts are used only by the chain backend.

All other backends' configuration objects will need these fields.

An alternative representation would be to have the ChainConfig contain these common fields unconditionally, AND an inner enum with only the non-common configuration data. But I think this is actually worse, because now the backends can't cleanly model their own data: they need access to common fields like the chain id, and those fields would live in an overarching ChainConfig object. With the current structure, the common fields would be duplicated across different objects (the PenumbraConfig would have an id field as well), but this actually makes the code more modular, because each backend can just pass around and work with one object it defines (the cosmos logic can work with a CosmosSdkConfig, the penumbra logic can work with a PenumbraConfig, and so on).

The way you have done it currently sounds like a good way to me to enforce that each config type provides this information, so we can leave it like this.

hdevalence · 2023-10-01T03:08:45Z

crates/relayer/src/keyring.rs

@@ -287,9 +287,10 @@ impl KeyRing<Ed25519KeyPair> {
    }
 }

+// Why is this not a method on `ChainConfig`?


Is there any reason this isn't a method on ChainConfig, rather than a bare function defined in this other module?

I guess the whole story about keys is a bit messy right now, as we have seen elsewhere. Agreed that this should be backend-specific.

hdevalence · 2023-10-01T03:10:11Z

I was planning to leave self-review comments on the build failures that reveal issues with the CLI interface, but that will need to wait for the CI to run.

hdevalence · 2023-10-01T03:18:25Z

crates/relayer/src/config.rs

+
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct CosmosSdkConfig {


This should move to the cosmos/ backend but I left it for now for ease of initial commentary/review.

Good idea to leave it here for the review, we can move it later on.

hdevalence · 2023-10-03T14:13:58Z

I thought it'd be possible to add comments to check failures, but this doesn't seem to be the case. Instead I put them inline.

hdevalence · 2023-10-03T14:28:18Z

crates/relayer-cli/src/config.rs

+        // TODO: why are these not methods on the config object?
+        // why are they defined only in the relayer-cli crate?
+        if let ChainConfig::CosmosSdk(cosmos_c) = c {
+            validate_trust_threshold(&cosmos_c.id, cosmos_c.trust_threshold)?;
+            // Validate gas-related settings
+            validate_gas_settings(&cosmos_c.id, cosmos_c)?;
+        }


Shouldn't config validation code be attached to the configs in the core crate, rather than being in the cli? Happy to move it if that sounds good.

Agreed, let's move that function into the core crate. No good reason to have it here.

hdevalence · 2023-10-03T14:29:52Z

crates/relayer-cli/src/commands/clear.rs

+            // Q: should the key name be required across chain types, meaning that
+            // key management is common to all chain types, or should key management
+            // be the responsibility of the backend? If key management is common
+            // across backends, how should it be agnostic to the key type? Can it
+            // just be an opaque byte string handled by the backend?


This ties in to #3641

Perhaps the key manager should only support opaque byte strings, and let the rest be handled by the backend?

romac

Thank you so much for taking on this whole endeavor. What you've done so far looks really good to me.

Now we need to figure out a good story for backend-specific commands, eg. keys, as you've pointed out here and in #3641.

romac · 2023-10-04T15:32:45Z

crates/relayer-cli/src/commands/listen.rs

@@ -144,6 +144,8 @@ fn subscribe(
    compat_mode: CompatMode,
    rt: Arc<TokioRuntime>,
 ) -> eyre::Result<Subscription> {
+    // Q: Should this be restricted only to backends that support it,
+    // or are all backends expected to support subscriptions?
    let (event_source, monitor_tx) = match &chain_config.event_source {


Yes that should be restricted only to backends to support subscriptions, but right now Hermes requires all backends to do so. Definitely more work needed there to remove this assumption.

romac · 2023-10-04T15:34:43Z

crates/relayer-cli/src/commands/listen.rs

@@ -166,6 +168,7 @@ fn subscribe(
    Ok(subscription)
 }

+// Q: why isn't this part of the CosmosSDK chain endpoint impl?


Agreed, we have similar code in the CosmosSdkChain::boostrap which ought to be refactored to use this function after we move it under the cosmos module.

romac · 2023-10-04T15:35:58Z

crates/relayer-cli/src/commands/tx/client.rs

@@ -203,6 +203,8 @@ impl Runnable for TxUpdateClientCmd {

        if let Some(restart_params) = self.genesis_restart_params() {
            if let Some(c) = config.find_chain_mut(&reference_chain_id) {
+                // Q: How should this be handled? Should the cli command only
+                // work on supported chain types?


Yes, we probably want such commands to restrict which types of chains they work against, either by explicitly checking that and throwing an error, or by moving them under a new sub-command per chain type, eg. this would become tx cosmos update client. Not sure which way to go yet.

romac · 2023-10-04T15:37:52Z

crates/relayer-cli/src/config.rs

+        // TODO: why are these not methods on the config object?
+        // why are they defined only in the relayer-cli crate?
+        if let ChainConfig::CosmosSdk(cosmos_c) = c {
+            validate_trust_threshold(&cosmos_c.id, cosmos_c.trust_threshold)?;
+            // Validate gas-related settings
+            validate_gas_settings(&cosmos_c.id, cosmos_c)?;
+        }


Agreed, let's move that function into the core crate. No good reason to have it here.

romac · 2023-10-04T15:38:24Z

crates/relayer/src/chain/client.rs

+        // TODO: extract Tendermint-related configs into a separate substructure
+        // that can be used both by CosmosSdkConfig and configs for nonSDK chains.


Sounds good!

romac · 2023-10-04T15:39:15Z

crates/relayer/src/config.rs

+#[serde(tag = "type")]
+pub enum ChainConfig {
+    CosmosSdk(CosmosSdkConfig),
+}


I don't think that supporting arbitrary variations is a worthwhile complexity vs ergonomics tradeoff.

Agreed! Looks good otherwise!

romac · 2023-10-04T15:40:26Z

crates/relayer/src/config.rs

+impl ChainConfig {
+    pub fn id(&self) -> &ChainId {
+        match self {
+            Self::CosmosSdk(config) => &config.id,
+        }
+    }
+
+    pub fn packet_filter(&self) -> &PacketFilter {
+        match self {
+            Self::CosmosSdk(config) => &config.packet_filter,
+        }
+    }
+
+    pub fn max_block_time(&self) -> Duration {
+        match self {
+            Self::CosmosSdk(config) => config.max_block_time,
+        }
+    }
+}


The way you have done it currently sounds like a good way to me to enforce that each config type provides this information, so we can leave it like this.

romac · 2023-10-04T15:40:39Z

crates/relayer/src/config.rs

+
+#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+pub struct CosmosSdkConfig {


Good idea to leave it here for the review, we can move it later on.

romac · 2023-10-04T15:41:10Z

crates/relayer/src/keyring.rs

@@ -287,9 +287,10 @@ impl KeyRing<Ed25519KeyPair> {
    }
 }

+// Why is this not a method on `ChainConfig`?


I guess the whole story about keys is a bit messy right now, as we have seen elsewhere. Agreed that this should be backend-specific.

romac · 2023-10-04T15:43:56Z

Now we need to figure out a good story for backend-specific commands, eg. keys, as you've pointed out here and in #3641.

Maybe for now we can restrict these commands to CosmosSdk chains, by throwing a runtime error if used against another type of chain?

erwanor · 2023-10-12T00:35:11Z

I have done some work propagating the configuration changes, but I'm leaving some of the refactor items as a last step (as previously agreed above).

I would like to pause and see if CI passes, tie up loose ends, before pushing on the specific TODOs (e.g. extracting the tendermint config) and doing another pass.

erwanor · 2023-10-16T16:45:08Z

This took longer than anticipated, but the branch is finally passing all integration tests. Now, we should be able to move quickly on getting the remaining items done and ready for review by tomorrow:

small review issues
- remove dead code
- move list_keys to be a method on ChainConfig
- move config validation out of relayer-cli

when this is done:
~~- [ ] extract the tendermint fields into a struct and make the chain config embed it~~

and finally:

move CosmosSdkConfig into the cosmos/ backend
move the cosmos specific config validation routines into the sdk backend
~~- [ ] move detect_compatibility_mode into the cosmos sdk impl~~

erwanor · 2023-10-23T23:47:47Z

@romac I think this is ready for review! We can split out the tendermint config in future work since it has a small surface area, ditto for the websocket methods, for now we can just error if they are not supported.

romac

Amazing work 🚀 Thanks so much to both of you for seeing this through, much appreciated! 💐

romac · 2023-10-24T18:07:16Z

@hdevalence Before I can merge this, can you please enable edits from maintainers on this PR?

hdevalence · 2023-10-24T20:26:57Z

Hmm, the Github docs only talk about personal-owned forks, I'm not sure if it's possible because we made it from an org fork. But I can certainly add you to our org fork ? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#

erwanor · 2023-10-24T20:32:14Z

@romac I can't see that button, maybe it's only visible to the PR creator, but I added you as a collaborator to the forked repository: invitations

edit: ah henry's comment just loaded for me!

romac · 2023-10-24T21:43:56Z

Perfect, thank you both! 🙏

seanchen1991 · 2023-10-25T13:36:36Z

Will these config changes require updates to the Hermes guide?

romac · 2023-10-25T13:56:29Z

I thought they were all taken care of, but actually found three templates that we missed: #3679

hdevalence commented Oct 1, 2023

View reviewed changes

hdevalence mentioned this pull request Oct 1, 2023

Change config format to scope configs by type #3636

Closed

5 tasks

hdevalence commented Oct 1, 2023

View reviewed changes

romac marked this pull request as ready for review October 2, 2023 08:16

romac marked this pull request as draft October 2, 2023 08:17

propagate changes into relayer-cli and add questions

f315763

hdevalence marked this pull request as ready for review October 3, 2023 14:13

hdevalence commented Oct 3, 2023

View reviewed changes

romac reviewed Oct 4, 2023

View reviewed changes

erwanor added 7 commits October 11, 2023 18:18

relayer-cli: propagate enum chain config changes

362f4a7

relayer: add ChainConfig::set_key_name

9b25ce6

test-framework: begin propagating config changes

4b6f769

relayer-cli: momentarily allow irrefutable patterns

71cf297

relayer: run cargo fmt

baad978

integration-tests: propagate config enum api changes

6b5f8fb

Merge remote-tracking branch 'upstream/master' into config-enum

4457c2b

erwanor added 8 commits October 13, 2023 14:56

hermes: include chain type in default config file

f28601b

hermes: add chain type to all config examples

64083a3

ci: update misbehavior config

8f02d10

relayer-rest(tests): add chain type to mock config

44032e5

ci(misbehavior): edit config of forked chain

0a09a0c

relayer(config): remove dead code

b1a39d7

relayer: lift keys to ChainConfig::list_keys

c158cba

relayer: move config validation to ibc_relayer::config

e7028fc

erwanor added 7 commits October 17, 2023 09:47

relayer: move cosmos config to chain::cosmos::config

3cb9db3

relayer: add chain::cosmos::config module

33e2cf5

relayer-cli(chain-registry): update path to CosmosSdkConfig

cddce8d

Merge remote-tracking branch 'upstream/master' into config-enum

4e7b5ee

ci(misbehavior-ics): add chain type to config file

c42de92

relayer(evidence): exit early if chain type is not cosmos sdk

e588311

relayer(cosmos): refactor cosmos-specific config validation

979cef4

romac approved these changes Oct 24, 2023

View reviewed changes

romac changed the title ~~wip: make config an endpoint-specific enum~~ Change config format to scope configs by type Oct 24, 2023

Add changelog entries

d7f8674

romac closed this Oct 24, 2023

romac reopened this Oct 24, 2023

romac merged commit df57aa3 into informalsystems:master Oct 25, 2023
42 of 78 checks passed

romac mentioned this pull request Jan 18, 2024

Use CosmosSdk as the chain type if omitted, for backward compatibility #3787

Merged

7 tasks

		// TODO: extract Tendermint-related configs into a separate substructure
		// that can be used both by CosmosSdkConfig and configs for nonSDK chains.

		fn bootstrap(config: ChainConfig, rt: Arc<TokioRuntime>) -> Result<Self, Error> {
		#[allow(irrefutable_let_patterns)]

Change config format to scope configs by type #3644

Change config format to scope configs by type #3644

Conversation

hdevalence commented Oct 1, 2023

Description

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

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

hdevalence commented Oct 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdevalence commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac left a 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 commented Oct 4, 2023

erwanor commented Oct 12, 2023

erwanor commented Oct 16, 2023 • edited Loading

erwanor commented Oct 23, 2023

romac left a comment

Choose a reason for hiding this comment

romac commented Oct 24, 2023

hdevalence commented Oct 24, 2023

erwanor commented Oct 24, 2023 • edited Loading

romac commented Oct 24, 2023

seanchen1991 commented Oct 25, 2023

romac commented Oct 25, 2023

erwanor commented Oct 16, 2023 •

edited

Loading

erwanor commented Oct 24, 2023 •

edited

Loading