Relay on all paths defined in the configuration #742

romac · 2021-03-12T16:48:06Z

Closes: #748

Description

If the start-multi command is ran without options, spawn a supervisor per connection defined in the config.

~~This PR is not ready yet because in a 3 chains setup, some events attached to an object that is not relevant to a worker are forwarded to it anyways.~~

~~To fix this, we must perform some queries to ensure that the object attached to an event is relevant to the two chains the supervisor is responsible for.~~

TODO

Open issue to track progress on this PR
Ensure that the Object assigned to an event is relevant for the pair of chains the supervisor is responsible for.
Either avoid spawning workers for irrelevant events or let the worker exit gracefully if it's not relevant.
Think about how to deal with worker exiting because of internal error. Will be addressed in future PR that introduces a proper architecture.

For contributor use:

Updated the Unreleased section of CHANGELOG.md with the issue.
If applicable: Unit tests written, added test to CI.
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments.
Re-reviewed Files changed in the Github PR explorer.

ancazamfir · 2021-03-15T16:16:54Z

relayer-cli/src/registry.rs

+/// Registry for keeping track of [`ChainHandle`]s indexed by a `ChainId`.
+///
+/// The purpose of this type is to avoid spawning multiple runtimes for a single `ChainId`.
+pub struct Registry<'a> {
+    config: &'a Config,
+    handles: HashMap<ChainId, Box<dyn ChainHandle>>,
+}
+


adizere

Looks good. Should we update the changelog?

Also, it wasn't clear to me what is the expected behavior. Upon starting with no options, assuming a default configuration (with dev-env) is the expected behavior to just start the supervisor for both chains and then stop?

$ hermes start-multi
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/hermes start-multi`
{"timestamp":"Mar 16 10:27:00.559","level":"INFO","fields":{"message":"spawning supervisor for chains ibc-0 and ibc-1"},"target":"ibc_relayer_cli::commands::start_multi"}
{"timestamp":"Mar 16 10:27:00.772","level":"INFO","fields":{"message":"running listener","chain.id":"ibc-0"},"target":"ibc_relayer::event::monitor"}
{"status":"success","result":"ok"}
{"timestamp":"Mar 16 10:27:00.875","level":"INFO","fields":{"message":"running listener","chain.id":"ibc-1"},"target":"ibc_relayer::event::monitor"}

romac · 2021-03-16T13:01:03Z

That's an oversight on my part, I forgot to commit the code which waits for the threads to finish.

There are also other issues with this PR which we discussed with @ancazamfir, so I am going to put it back in draft until I address them (see TODO in PR description).

…i is ran without options

romac · 2021-03-23T20:17:26Z

Looks good. Should we update the changelog?

Done.

adizere · 2021-03-25T15:36:31Z

relayer/src/supervisor.rs

+
+    let src_channel = src_chain.query_channel(src_port_id, src_channel_id, Height::zero())?;
+
+    // TODO: Check channel state?


Shall we fix the TODO or is this for later?

It's actually more of a question than a TODO, was hoping you or @ancazamfir could advise me as to what to do here.

If the channel doesn't exist, this query returns an Uninitialized channel with zero connection hops. So we will see an error when we try to get the connection further down.

scripts/dev-env

scripts/init-clients

scripts/setup-chains

adizere · 2021-03-25T15:44:02Z

Using the following config h/t @romac

[global]
timeout = '10s'
strategy = 'naive'
log_level = 'info'
[[chains]]
id = 'ibc0'
rpc_addr = 'tcp://localhost:26657'
grpc_addr = 'tcp://localhost:9090'
account_prefix = 'cosmos'
key_name = 'testkey'
store_prefix = 'ibc'
gas = 3000000
clock_drift = '5s'
trusting_period = '14days'
[chains.trust_threshold]
numerator = '1'
denominator = '3'
[[chains]]
id = 'ibc1'
rpc_addr = 'tcp://localhost:26557'
grpc_addr = 'tcp://localhost:9091'
account_prefix = 'cosmos'
key_name = 'testkey'
store_prefix = 'ibc'
gas = 3000000
clock_drift = '5s'
trusting_period = '14days'
[chains.trust_threshold]
numerator = '1'
denominator = '3'
[[chains]]
id = 'ibc2'
rpc_addr = 'tcp://localhost:26457'
grpc_addr = 'tcp://localhost:9092'
account_prefix = 'cosmos'
key_name = 'testkey'
store_prefix = 'ibc'
gas = 3000000
clock_drift = '5s'
trusting_period = '14days'
[chains.trust_threshold]
numerator = '1'
denominator = '3'
[[connections]]
a_chain = 'ibc0'
b_chain = 'ibc1'
[[connections.paths]]
a_port = 'transfer'
b_port = 'transfer'
[[connections]]
a_chain = 'ibc1'
b_chain = 'ibc2'
[[connections.paths]]
a_port = 'transfer'
b_port = 'transfer'
[[connections]]
a_chain = 'ibc2'
b_chain = 'ibc0'
[[connections.paths]]
a_port = 'transfer'
b_port = 'transfer'

adizere · 2021-03-25T16:24:44Z

Not a problem, but got me confused (thinking that an error occurred), but upon running:

./scripts/dev-env ~/.hermes/multi_config.toml ibc0 ibc1 ibc2

There is in the output some error-level messages:

...
Removing light client peers from configuration...
error: hermes fatal error: no peers configured for chain: ibc0
error: hermes fatal error: no peers configured for chain: ibc1
error: hermes fatal error: no peers configured for chain: ibc2
...

Again, this is not exactly an error: there are no peers configured for the chain (and therefore they cannot be removed). But others may also find it confusing.

Co-authored-by: Adi Seredinschi <adi@informal.systems>

relayer/src/supervisor.rs

romac · 2021-03-25T17:53:45Z

Not a problem, but got me confused (thinking that an error occurred), but upon running:

./scripts/dev-env ~/.hermes/multi_config.toml ibc0 ibc1 ibc2

There is in the output some error-level messages:
...
Removing light client peers from configuration...
error: hermes fatal error: no peers configured for chain: ibc0
error: hermes fatal error: no peers configured for chain: ibc1
error: hermes fatal error: no peers configured for chain: ibc2
...
Again, this is not exactly an error: there are no peers configured for the chain (and therefore they cannot be removed). But others may also find it confusing.

Yeah that's pretty bad UX, sorry about that. This should be a warning at most or even a different message

relayer/src/supervisor.rs

romac · 2021-03-29T15:26:20Z

Again, this is not exactly an error: there are no peers configured for the chain (and therefore they cannot be removed). But others may also find it confusing.

Yeah that's pretty bad UX, sorry about that. This should be a warning at most or even a different message

Tracked in #782

ancazamfir

Looks good! Thanks Romain!

* Make supervisor::collect_events public * Fix rustdoc warning * Spawn a supervisor per connection defined in the config if start-multi is ran without options * Make sure we don't spawn a runtime for the same chain twice * Use a registry to avoid spawning a chain runtime multiple times * Wait for all supervisor threads to finish * Update scripts to spawn three chains * Disable JSON output for start-multi command * Show output and errors of hermes commands in init-clients script * Cleanup * Only send relevant events to the workers * Fix typo * Update scripts to allow spawning either 2 or 3 chains * Send NewBlock events after all other events * Update changelog * Formatting * Remove commented out code Co-authored-by: Adi Seredinschi <adi@informal.systems> * Fix WriteAck object and discard bogus events * Fix for master merge * Check channel and connection state when computing counterparty chain Co-authored-by: Adi Seredinschi <adi@informal.systems>

romac requested a review from ancazamfir March 12, 2021 16:48

ancazamfir reviewed Mar 15, 2021

View reviewed changes

adizere reviewed Mar 16, 2021

View reviewed changes

romac marked this pull request as draft March 16, 2021 13:01

romac changed the title ~~Allow spawning a supervisor per connection defined in the config~~ Relay for all paths defined in the configuration Mar 18, 2021

romac changed the title ~~Relay for all paths defined in the configuration~~ Relay on all paths defined in the configuration Mar 21, 2021

romac added 13 commits March 23, 2021 20:46

Make supervisor::collect_events public

602e074

Fix rustdoc warning

b5760e4

Spawn a supervisor per connection defined in the config if start-mult…

73c783f

…i is ran without options

Make sure we don't spawn a runtime for the same chain twice

5dbc573

Use a registry to avoid spawning a chain runtime multiple times

12d7f68

Wait for all supervisor threads to finish

6e5d852

Update scripts to spawn three chains

52eb9f4

Disable JSON output for start-multi command

4faa4c6

Show output and errors of hermes commands in init-clients script

90ed918

Cleanup

416a36c

Only send relevant events to the workers

da9104d

Fix typo

cebf61a

Update scripts to allow spawning either 2 or 3 chains

a36829d

romac force-pushed the romac/load-chains2 branch from 74c7ab1 to a36829d Compare March 23, 2021 19:59

Send NewBlock events after all other events

85b755d

This comment has been minimized.

Sign in to view

Update changelog

349d9e3

romac mentioned this pull request Mar 24, 2021

Reactive relayer architecture for packets relaying and channel/connection handshakes #764

Closed

11 tasks

romac added 2 commits March 24, 2021 15:17

Merge branch 'master' into romac/load-chains2

5e9c0f9

Formatting

89259c4

romac marked this pull request as ready for review March 24, 2021 18:53

adizere reviewed Mar 25, 2021

View reviewed changes

Remove commented out code

b85a2e9

Co-authored-by: Adi Seredinschi <adi@informal.systems>

romac commented Mar 25, 2021

View reviewed changes

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

ancazamfir reviewed Mar 29, 2021

View reviewed changes

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

ancazamfir reviewed Mar 29, 2021

View reviewed changes

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

romac added 3 commits March 29, 2021 13:25

Fix WriteAck object and discard bogus events

a476c35

Merge branch 'master' into romac/load-chains2

a3ef47d

Fix for master merge

de3725e

Check channel and connection state when computing counterparty chain

ecf4dd9

romac requested review from adizere and ancazamfir March 29, 2021 15:56

ancazamfir approved these changes Mar 30, 2021

View reviewed changes

romac merged commit a75cb58 into master Mar 30, 2021

romac deleted the romac/load-chains2 branch March 30, 2021 12:08

romac mentioned this pull request Apr 6, 2021

Documentation for Hermes v0.2 #792

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relay on all paths defined in the configuration #742

Relay on all paths defined in the configuration #742

romac commented Mar 12, 2021 •

edited

Loading

ancazamfir Mar 15, 2021

adizere left a comment

romac commented Mar 16, 2021 •

edited

Loading

This comment has been minimized.

romac commented Mar 23, 2021

adizere Mar 25, 2021

romac Mar 25, 2021

ancazamfir Mar 29, 2021

adizere commented Mar 25, 2021

adizere commented Mar 25, 2021 •

edited

Loading

romac commented Mar 25, 2021

romac commented Mar 29, 2021 •

edited

Loading

ancazamfir left a comment


		let src_channel = src_chain.query_channel(src_port_id, src_channel_id, Height::zero())?;

		// TODO: Check channel state?

Relay on all paths defined in the configuration #742

Relay on all paths defined in the configuration #742

Conversation

romac commented Mar 12, 2021 • edited Loading

Description

TODO

ancazamfir Mar 15, 2021

Choose a reason for hiding this comment

adizere left a comment

Choose a reason for hiding this comment

romac commented Mar 16, 2021 • edited Loading

This comment has been minimized.

romac commented Mar 23, 2021

adizere Mar 25, 2021

Choose a reason for hiding this comment

romac Mar 25, 2021

Choose a reason for hiding this comment

ancazamfir Mar 29, 2021

Choose a reason for hiding this comment

adizere commented Mar 25, 2021

adizere commented Mar 25, 2021 • edited Loading

romac commented Mar 25, 2021

romac commented Mar 29, 2021 • edited Loading

ancazamfir left a comment

Choose a reason for hiding this comment

romac commented Mar 12, 2021 •

edited

Loading

romac commented Mar 16, 2021 •

edited

Loading

adizere commented Mar 25, 2021 •

edited

Loading

romac commented Mar 29, 2021 •

edited

Loading