Allow more granular configuration of Hermes modes of operation #1539

hu55a1n1 · 2021-11-03T10:58:22Z

Closes: #1518

Description

Default config for mode would look like ->

[mode]

[mode.clients]
enabled = true
refresh = true
misbehaviour = true

[mode.connections]
enabled = false

[mode.channels]
enabled = false

[mode.packets]
enabled = true
clear_interval = 100
clear_on_start = true
filter = false
tx_confirmation = true

strategy and clear_packets_interval options would be removed.
...

TODO (aside from acceptance criteria)

Guide changes.
Fix E2E tests.
Fix gm related stuff.

For contributor use:

Added a changelog entry, using unclog.
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.

relayer/src/supervisor.rs

romac

Amazing stuff @hu55a1n1 🥇

Here are a few comments for discussion after a first pass over the code.

relayer/src/worker.rs

relayer/src/worker/packet.rs

Co-authored-by: Romain Ruetschi <romain@informal.systems>

adizere · 2021-11-10T15:00:44Z

@greg-szabo we would appreciate your quick look here, at the gm related changes.

greg-szabo · 2021-11-10T16:31:39Z

Looks good to me. The only thing missing is the gm Changelog update but I can deal with that separately in my own PR.

…lect_events`

ancazamfir · 2021-11-11T09:22:06Z

@hu55a1n1 I tried with a config that has:

[mode.packets]
# Whether or not to enable the packet workers. [Required]
enabled = true
# Parametrize the periodic packet clearing feature.
# Interval (in number of blocks) at which pending packets
# should be eagerly cleared. A value of '0' will disable
# periodic packet clearing. [Default: 100]
clear_interval = 10
# Whether or not to clear packets on start. [Default: false]
clear_on_start = true

all other modes disabled.
Create a channel, send some packets then start hermes. The packets are not cleared immediately. Also not cleared on the clear_interval. If I send a packet after hermes start then everything is back to normal.
Didn't look yet at the code but I think it should be straight forward to fix.

Also e2e fails with this config.

… into 1518-config-modes

hu55a1n1 · 2021-11-11T11:38:57Z

The packets are not cleared immediately. Also not cleared on the clear_interval.

Thanks for catching that @ancazamfir! I think this was a bug in the way packet workers were spawned. I just pushed a possible fix -> 4ec957b

relayer/src/worker/client.rs

ancazamfir

Tested a bunch of scenarios, it looks very good!!
I will look again through the changes next but I think we should be good to go.

romac

Amazing work @hu55a1n1 🌷

ancazamfir

Amazing work @hu55a1n1!

* WIP relayer test * Bootstrap chain almost working * Refactor chain bootstrap code * Trying to get create client work. * Test create client is now working * Fix formatting * WIP IBC packet transfer * Got IBC transfer working * Refactor supervisor and denom tracing * More refactoring * More refactoring * Refactor build_and_send_transfer_messages * Round trip IBC token transfer is now working * Refactor channel bootstrap code * Add tagged chain handle wrapper * Rename ChainCommand to ChainDriver * Make bootstrap pair return impl ChainHandle * Add tagged types * Refactor bootstrap pair * Tag ChainDriver * Add mono tag, refactor single chain bootstrap * More refactoring * More refactoring * Fix chain tags * Channel creation ordering is now consistent * Test IBC transfer in both directions * Fix monitor shutdown error * Refactor wait wallet amount * Make init script return test config. Add hang test helper * Draft memo testing working * Add boostrap_chain_and_channel_pair * Add memo test as separate test * Disable health check in tests to suppress version errors from gaiad in Nix * Add CI workflow * Use experimental Nix with flakes * Refactor test cases to use composable traits * Use NoTestConfig wrapper type for test defaults * Reorder code definitions * Try out new way to override test config and implement override defaults * Preemptively shutdown supervisor at the end of chain tests * Rearrange type definitions * Rename framework traits * Rename and move override traits * Rename ChainDeployment to ConnectedChains * Refactor chain bootstrap * Add BinaryNodeTest trait * Add alpha- and beta- prefix to binary chain nodes * Add HANG_ON_FAIL test hooks * Add non-owned BinaryNodeTest * Refactor Supervisor to use shared reference to chain registry * Spawn supervisor in OwnedBinaryChainTest * New test override approach with separate override argument * Clean up overrides design * Fix cargo doc * Save relayer config onto file system * Simplify test overrides * Use absolute path for data directory path * Adding documentation * Add documentation for `framework` module * Adding more documentation * Reorganize test data directory * Add documentation for `bootstrap` module * Reorganize tagged module * Add documentation for MonoTagged * Refactor tagged methods into traits * Add documentation for DualTagged * Finish documenting most constructs * Move example documentation * Rename hang to suspend * Fix example doctest * Add more detailed documentation for DualTagged::contra_map * Remove ChainClientServer and inline the handle and node fields directly * Add documentation for connected chains and channels * Finish documenting everything * Only check for version in health check. Failure in parsing chain version should not result in failure of the entire relayer. * Stop chain handle when it is dropped * Add simulation manual test * Add documentation for new constructs * Move manual tests into the tests::manual module * Add test framework for running tests with self-connected chains * Add NodeConfigOverride for overriding full node config * Draft definition for N-ary connected chains using const generics * Implement N-ary bootstrap chains code * Done initial bootstrap code for N-ary channels. * Refactor to allow dynamic sized n-ary chains * Running binary IBC transfer test as n-ary test successful * Add environment exporter to test * Separate drop handlers from chain and node types * Remove reference version of test traits * Remove "Owned" and "owned_" prefixes * Put N-ary chain features behind "experimental" feature flag * Clean up cargo configuration, address review feedback * Fix build error * Fix doctests * Add custom Default instance for ModeConfig which enables everything by default * Follow default ModeConfig as specified in #1539 * Add test_setup_with_binary_channel command wrapper * Fix clippy * Fix unit test CI * Randomize created client and connection ids * Randomize channel IDs when bootstrapping channels * Move bootstrap connection code to separate module * Add back child process as a shared Arc pointer in FullNode * Move experimental N-ary test chains to separate PR * Fill out missing documentation * Move N-ary chain constructs to #1572 * Minor fix documentation * Add a bit more documentation to tagged identifiers * Slightly clean up bootstrap logs

…malsystems#1539)

* WIP relayer test * Bootstrap chain almost working * Refactor chain bootstrap code * Trying to get create client work. * Test create client is now working * Fix formatting * WIP IBC packet transfer * Got IBC transfer working * Refactor supervisor and denom tracing * More refactoring * More refactoring * Refactor build_and_send_transfer_messages * Round trip IBC token transfer is now working * Refactor channel bootstrap code * Add tagged chain handle wrapper * Rename ChainCommand to ChainDriver * Make bootstrap pair return impl ChainHandle * Add tagged types * Refactor bootstrap pair * Tag ChainDriver * Add mono tag, refactor single chain bootstrap * More refactoring * More refactoring * Fix chain tags * Channel creation ordering is now consistent * Test IBC transfer in both directions * Fix monitor shutdown error * Refactor wait wallet amount * Make init script return test config. Add hang test helper * Draft memo testing working * Add boostrap_chain_and_channel_pair * Add memo test as separate test * Disable health check in tests to suppress version errors from gaiad in Nix * Add CI workflow * Use experimental Nix with flakes * Refactor test cases to use composable traits * Use NoTestConfig wrapper type for test defaults * Reorder code definitions * Try out new way to override test config and implement override defaults * Preemptively shutdown supervisor at the end of chain tests * Rearrange type definitions * Rename framework traits * Rename and move override traits * Rename ChainDeployment to ConnectedChains * Refactor chain bootstrap * Add BinaryNodeTest trait * Add alpha- and beta- prefix to binary chain nodes * Add HANG_ON_FAIL test hooks * Add non-owned BinaryNodeTest * Refactor Supervisor to use shared reference to chain registry * Spawn supervisor in OwnedBinaryChainTest * New test override approach with separate override argument * Clean up overrides design * Fix cargo doc * Save relayer config onto file system * Simplify test overrides * Use absolute path for data directory path * Adding documentation * Add documentation for `framework` module * Adding more documentation * Reorganize test data directory * Add documentation for `bootstrap` module * Reorganize tagged module * Add documentation for MonoTagged * Refactor tagged methods into traits * Add documentation for DualTagged * Finish documenting most constructs * Move example documentation * Rename hang to suspend * Fix example doctest * Add more detailed documentation for DualTagged::contra_map * Remove ChainClientServer and inline the handle and node fields directly * Add documentation for connected chains and channels * Finish documenting everything * Only check for version in health check. Failure in parsing chain version should not result in failure of the entire relayer. * Stop chain handle when it is dropped * Add simulation manual test * Add documentation for new constructs * Move manual tests into the tests::manual module * Add test framework for running tests with self-connected chains * Add NodeConfigOverride for overriding full node config * Draft definition for N-ary connected chains using const generics * Implement N-ary bootstrap chains code * Done initial bootstrap code for N-ary channels. * Refactor to allow dynamic sized n-ary chains * Running binary IBC transfer test as n-ary test successful * Add environment exporter to test * Separate drop handlers from chain and node types * Remove reference version of test traits * Remove "Owned" and "owned_" prefixes * Put N-ary chain features behind "experimental" feature flag * Clean up cargo configuration, address review feedback * Fix build error * Fix doctests * Add custom Default instance for ModeConfig which enables everything by default * Follow default ModeConfig as specified in informalsystems#1539 * Add test_setup_with_binary_channel command wrapper * Fix clippy * Fix unit test CI * Randomize created client and connection ids * Randomize channel IDs when bootstrapping channels * Move bootstrap connection code to separate module * Add back child process as a shared Arc pointer in FullNode * Move experimental N-ary test chains to separate PR * Fill out missing documentation * Move N-ary chain constructs to informalsystems#1572 * Minor fix documentation * Add a bit more documentation to tagged identifiers * Slightly clean up bootstrap logs

hu55a1n1 added 6 commits November 3, 2021 15:38

Add config mode

9f559b2

Remove config strategy

403d280

Remove config global clear_packets_interval option

1287576

Skip misbehaviour based on client mode in config

59e02c2

Clear packets on start based on config mode

26e72cf

Refresh clients only if enabled in config

0fb2d6c

romac assigned romac and hu55a1n1 Nov 5, 2021

Collect event based on config mode

3957b2e

romac reviewed Nov 5, 2021

View reviewed changes

relayer/src/supervisor.rs Show resolved Hide resolved

hu55a1n1 added 9 commits November 5, 2021 20:03

Fix collect_event()

867c42d

Spawn workers based on config mode

cac31a6

Update E2E hermes config

5cbe960

Fix tests

7fbe9a8

Fix E2E tests

86b5e5d

Fix comments that referenced 'strategy' in py files

2f2e496

Spawn workers conditionally depending on mode config

d3bb531

Remove references to strategy in guide and docs

1b2463c

Remove more refs to strategy

ce1bee5

hu55a1n1 marked this pull request as ready for review November 8, 2021 11:03

hu55a1n1 requested review from adizere and ancazamfir as code owners November 8, 2021 11:03

hu55a1n1 added 2 commits November 8, 2021 16:45

Add .changelog entry

47a3c1f

Remove global config options - filter & tx_confirmations

28cae96

romac reviewed Nov 8, 2021

View reviewed changes

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

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

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

hu55a1n1 added 5 commits November 8, 2021 21:21

Fix Packet::default() recursion bug

3551055

cargo fmt

3b61294

Address feedback

a12be44

Add serde(deny_unknown_fields) for mode config

c51992c

Add check for valid mode config

a743ea0

Trck packet worker's first run using a boolean

8912511

romac reviewed Nov 10, 2021

View reviewed changes

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

hu55a1n1 and others added 3 commits November 10, 2021 18:44

Enable connections and channels mode for gm default config

f87720d

Apply suggestion

4dc73ca

Co-authored-by: Romain Ruetschi <romain@informal.systems>

Fix build

df9cc0f

romac changed the title ~~More granular config of op modes~~ Allow more granular configuration of Hermes modes of operation Nov 11, 2021

romac added 2 commits November 11, 2021 10:03

Add comment for Supervisor::collect_events

d09f6a5

Avoid holding the lock on the config for too long in `Supervisor::col…

1b294a1

…lect_events`

romac and others added 5 commits November 11, 2021 10:42

Small refactor in the packet worker

81f96f9

Emit a warning if all operations modes are disabled

2e3bca3

Fix typo

04b501e

Fix case where only packet mode in enabled

4ec957b

Merge branch '1518-config-modes' of github.com:informalsystems/ibc-rs…

e14c8af

… into 1518-config-modes

ancazamfir reviewed Nov 11, 2021

View reviewed changes

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

ancazamfir reviewed Nov 11, 2021

View reviewed changes

romac added 2 commits November 12, 2021 12:20

Merge branch 'master' into 1518-config-modes

2555801

Improve log message in cient worker

db3b6a5

romac approved these changes Nov 12, 2021

View reviewed changes

ancazamfir approved these changes Nov 15, 2021

View reviewed changes

romac merged commit ce432b9 into master Nov 15, 2021

romac deleted the 1518-config-modes branch November 15, 2021 08:18

soareschen added a commit that referenced this pull request Nov 15, 2021

Follow default ModeConfig as specified in #1539

0cbc0f4

soareschen mentioned this pull request Nov 15, 2021

Implement custom Default instance for ModeConfig #1565

Merged

5 tasks

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

Allow more granular configuration of Hermes modes of operation (infor…

29c3515

…malsystems#1539)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow more granular configuration of Hermes modes of operation #1539

Allow more granular configuration of Hermes modes of operation #1539

hu55a1n1 commented Nov 3, 2021 •

edited

Loading

romac left a comment

adizere commented Nov 10, 2021

greg-szabo commented Nov 10, 2021

ancazamfir commented Nov 11, 2021

hu55a1n1 commented Nov 11, 2021

ancazamfir left a comment

romac left a comment

ancazamfir left a comment

Allow more granular configuration of Hermes modes of operation #1539

Allow more granular configuration of Hermes modes of operation #1539

Conversation

hu55a1n1 commented Nov 3, 2021 • edited Loading

Description

TODO (aside from acceptance criteria)

romac left a comment

Choose a reason for hiding this comment

adizere commented Nov 10, 2021

greg-szabo commented Nov 10, 2021

ancazamfir commented Nov 11, 2021

hu55a1n1 commented Nov 11, 2021

ancazamfir left a comment

Choose a reason for hiding this comment

romac left a comment

Choose a reason for hiding this comment

ancazamfir left a comment

Choose a reason for hiding this comment

hu55a1n1 commented Nov 3, 2021 •

edited

Loading