Fix `execute_schedule` method leaking operational data #2118

seanchen1991 · 2022-04-25T15:33:51Z

Closes: #1153

Description

This PR adds an integration test that verifies that the execute_schedule method drops operational data when an error is encountered. It also fixes the leaking of data such that the integration test passes.

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

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

…to execute-schedule-test Merge in upstream changes from master

…edule`

…to execute-schedule-test Merge in changes from master

…to execute-schedule-test

…s into execute-schedule-test

relayer/src/link/relay_path.rs

relayer/src/link/operational_data.rs

adizere · 2022-05-24T08:18:03Z

relayer/src/link/operational_data.rs

    Source,
+    /// The destination chain.


Suggested change

/// The destination chain.

/// The destination chain, i.e., the chain on the receiving side of packets.

The language being proposed for the Source variant vs. the Destination variant has me wondering what the relationship is between packets and operational data; how are the two related? Why is it that the destination chain receives packets, not operational data?

what the relationship is between packets and operational data; how are the two related?

This is also something I've been struggling a lot with. Hope these notes below help. The important concepts are marked in bold:

one chain (source) sends packets

a user (typically) submits transactions with these kind of packets

the other chain (destination) receives packets

a relayer submits these kind of packets to the destination chain

if no relayer acts in time, then the relayer submits a timeout packet to the source chain (instead of receive packet to destination chain)

when a chain processes a send packets, that translates into an event

when a chain processes a receive packet, that also translates into an event

Hermes acts upon these kind of events reactively

Hermes translates events into operational data (a concept internal to Hermes), which encompasses both an event as well as any resulting -- receive or timeout -- packet that Hermes has to submit based on that event

so operational data is just an internal representation that Hermes keeps for pairing together an event with the action that that event entails

aside from the main question, but:

all packets (send, receive, timeout) are IBC messages

beside packets, there are other types of messages: client update messages, notably, and soon relayer fees messages (I think).

connection and channel handshakes are also types of IBC messages

Hermes typically batches multiple IBC messages into a transaction

so the three primitive data types are:

packets, messages, and transactions

I'm thinking we should turn this into a diagram and put in the architecture or docs/ somewhere.

I opted to capture as much of this that is relevant to the OperationalData type in its doc comment. Hopefully it helps clear some of this up a bit.

relayer/src/link/relay_path.rs

tools/integration-test/src/tests/execute_schedule.rs

relayer/src/link/operational_data.rs

adizere

We should fix the comments. Otherwise I tested and looks good!

relayer/src/link/operational_data.rs

relayer/src/link/relay_path.rs

adizere

LGTM, great work with this Sean!

* Release v0.15.0 (informalsystems#2234) * Bump crates to v0.15.0 and ibc-proto to v0.18.0 * Include ibc-test-framework in release * Slightly improve main Cargo doc for ibc-test-framework * Fix components names * Update changelog * Update lockfile Co-authored-by: Soares Chen <soares.chen@maybevoid.com> * Add type ascription to fix build when `telemetry` feature is disabled (informalsystems#2235) * Bump once_cell from 1.11.0 to 1.12.0 (informalsystems#2237) * Add `ibc-test-framework` to the main README (informalsystems#2236) * Add `ibc-test-framework` to the main README * Cleanup * Fix `execute_schedule` method leaking operational data (informalsystems#2118) * Pull in upstream changes * Adding integration test for execute_schedule * Adding integration test for execute_schedule * Finish stubbing out execute_schedule test * Call `chains.shutdown` method * Correctly shut down chain * Shut down node b as well * Debugging execute_schedule test * Add Debug derivations * Update integration test so that its flow correctly tests `execute_schedule` * Attempt to perform a second IBC transfer * Remove info's * Increase sleep timeout duration * Incorportate new test framework features * Remove unnecessary `sleep` call * Correctly use new test framework features * Get assertions passing for now * Send two transactions, one in each direction * Add doc comment for test * Improve panic messages * Refactor test so that it is actually testing the desired behavior * Attempt at fixing `execute_schedule` leaky logic * Flesh out doc comments some more * Remove a duplicate function * Make use of OperationalDataTarget enum * Remove redundant enum * Remove some Debug derives * Remove one more debug derive * Add `try_fetch_scheduled_operational_data` back in * Give `do_execute_schedule` a more descriptive name * Improve `execute_schedule_for_target_chain` method's documentation * Add a bunch of clarifying comments * More clarification of comments * Flesh out `OperationalData` docs * Add changelog entry * Incorporate PR feedback Co-authored-by: Adi Seredinschi <adi@informal.systems> * Add `keys balance` command to query the balance for a key (informalsystems#2232) * Added subcommand 'balance' for command 'keys' to output the account balance associated with a given key * Added changelog entry for new command feature * Updated Hermes guide to include the new keys balance command * Improved error log for the CLI command and added doc comment * fixed fmt by running cargo fmt * Refactored query_balance to take key_name parameter. Added JSON output to keys balance command. * Fixed typo in comment in CosmosSdkChain query_balance method * Updated keys balance command to take the key_name as an optional flag Co-authored-by: Luca Joss <luca.joss@arcanite.ch> Co-authored-by: Romain Ruetschi <romain@informal.systems> * Model based testing on integration tests (informalsystems#2072) * ics20 token transfer spec * fixes for tla functions * full spec * update tla specs * mbt driver code * prepare for itf adoption * itf deserializer * description in comments * fix type * hack for apalache v0.20.2 (apalache-mc/apalache#1304) * updated tla spec * example itf.json * code refactor * prepare for mbt * fix and update * updated example * updated trace types * working mbt driver * fix mbt test * added mbt readme * BinaryChainTest over BinaryChannelTest * wait for established channel * added packet state assertions * improved mbt integration * few fixes * Refactor bootstrap chain and foreign client code * Introduce BootstrapChannel/ConnectionOptions * Increase timeout for assert_eventual_wallet_amount for CI * Upgrade Rust to 1.60.0 * Fix fmt in Rust 1.60.0 * Update Rust MSRV to 1.60 * Try running CI integration test single-threaded * Fix variable mixup * Add MBT to CI * Disable failing parse_itf test * Disable test_self_connected_ibc_transfer * Use gaia6 to run MBT test * Debug log on CI * save itf trace on failure * initialize nested maps * updated mbt code * Use new Nix URL * better fix to empty nested maps * Remove stale comments * Fix merge conflicts Co-authored-by: Soares Chen <soares.chen@maybevoid.com> Co-authored-by: Adi Seredinschi <adi@informal.systems> * Bump uuid from 1.0.0 to 1.1.0 (informalsystems#2247) Bumps [uuid](https://github.com/uuid-rs/uuid) from 1.0.0 to 1.1.0. - [Release notes](https://github.com/uuid-rs/uuid/releases) - [Commits](uuid-rs/uuid@1.0.0...1.1.0) --- updated-dependencies: - dependency-name: uuid dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add `query channel client` command to query a channel's client state (informalsystems#2248) * Added new CLI command 'query channel client' * Updated Hermes guide to include new query channel client command and updated changelog * Added missing subcommand in Hermes guide, section 7.9.3 Co-authored-by: Luca Joss <luca@informal.systems> * Fix wrong `create channel` command in "Create a new path" page in the guide (informalsystems#2245) Fix wrong `create channel` command in guide Co-authored-by: Adi Seredinschi <adi@informal.systems> * Disable MBT tests on CI due to flakiness (informalsystems#2253) * Complete ICS20 Implementation (informalsystems#1989) * Add ics26 Module trait * newtype for Acknowledgement * Define Router interface * Improve Router trait * Update mock router impl * Test for router API * Disallow duplicate module_ids in MockRouter * Add RouterBuilder for seal-style API * Fix failing test * Fix CI after merge * Chainable RouterBuilder::add_route() * Fix test * Add Router::has_route() trait method * Add comments * Separate mutating/non-mutating trait methods for shadow-paging style API * Make most Module trait methods optional * Default impl for MockModule::on_chan_open_try() * Acknowledgement trait * Extend router test * Simplify OnRechPacketResult using FnOnce() * Cleanup * Use Cow<str> for CapabilityName::new() * Use Cow<str> for CapabilityName::new() * Use newtype ModuleId instead of trait assoc type * Module callbacks' args as refs * Fix mock impl * WIP channel callbacks * Fix ModuleId ctor validation * TypedCapability * Use typed capabilities * Add Router::route_mut() * Implement pre-dispatch channel message validation * Avoid cloning message during channel msg dispatch * Add ChannelReader::lookup_module_by_channel() * Complete ics4 dispatch with callbacks * Fix compile errors from rebase * Fix CI test * Improve mock impl for capabilities * cargo fmt * Set channel version returned from on_chan_open_try() * Implement packet handler verification * Add missing check already received packets on ordered channels * Avoid cloning message during packet msg dispatch * Implement RecvPacket NoOp * cargo fmt and remove unused errors * Make on_recv_packet() Ack result optional * Don't return Result from on_recv_packet() and pass GenericAcknowledgement to on_acknowledgement_packet() * Implement packet callbacks * Allow callbacks to write logs and emit events * Fix test * Add .changelog entry * Add a comment for state rollback expectation from dispatch() * ics26_routing::handler::deliver() takes single message as input * ics26_routing::handler::deliver() returns logs as well * Remove ctx_ro * Callbacks return ModuleOutput<T> * Return HandlerOutputBuilder from channel and packet dispatch fn * Revert "Callbacks return ModuleOutput<T>" This reverts commit 1d430c9. * Address review feedback for comments * Extract ChannelMsg::lookup_module() * Add ICS20 Denom type * Add ICS20 TracePrefix & TracePath type * Define Coin and Decimal types * Impl conversions from/to RawDenomTrace * Make better use of derive-more * Impl conversions for Coin type * Add HashedDenom and polish DenomTrace impl * Define PacketData domain type and conversions * Use Coin domain type in MsgTransfer * Fix usage of Coin type in relayer code * Always panic on decimal arith overflow * Polish generic Signer impl * Implement ICS20 signer type * Coin type with generic Denom param * Use IbcCoin in MsgTransfer and PrefixedCoin in PacketData * Minor refactoring * Fix test_util * Impl AsRef<[u8]> for GenericAcknowledgement * Ics20Context is no longer a supertrait of Ics26Context * Add ICS20 Ack type * Add ICS20 event enum placeholder * Define all ICS20 expected keepers and context * Update send_transfer() for recent context changes * Give mut ref to set_channel_escrow_address() * Define ICS20 callback functions * Fix test build * Move denom derive functions to integration tests * Add version to chan_open_try callback * Rename ChannelId::counter() to sequence() * Add AppModule error variant to ChannelError * Implement validation helpers for callbacks * Define Ics20 callback errors * Implement channel handshake callbacks * Fix clippy errors * Impl Deserialize for PacketData * Manually implement AsRef<str> for Signer * Fix ICS20 Ack success from impl * Add more ICS20 errors * Improve ack type ctor * Add ctor for TracePrefix * Provide denom trace methods to remove prefix and check for empty trace path * Implement conversion from PrefixedCoin to IbcCoin * Add Ics20Reader trait methods to check send/receive enabled * Provided trait method get_channel_escrow_address() * Add BankReader trait for is_blocked_account() * Add FromStr bound for Ics20Context::AccountId * Use IbcCoins in Bank traits * Impl on_recv_packet() for cases where receiver chain is source * Fallible OnRecvPacket::write_fn() * Complete on_recv_packet impl * Fix test build * Fix clippy errors * Implement remaining packet callbacks for ICS20 * Make set_denom_trace() fallible * Don't derive AsRef * Complete send_transfer impl * Manual deserialize impl for Acknowledgement * Add ctor for Acknowledgement * Handle packet-data deserialize error separately * Use U256 for Denom and move bigint.rs to modules/src * Rename Denom to Amount * Cleanup * Fix trait definitions * Use source enum * Fix AccountReader trait * Validate port_id * More refactoring * Rename Signer to Address * Use Address instead of Signer where applicable * Fix send_transfer packet creation * Fix clippy warnings * Define ICS20 events * Extract relay code into separate files * Fix clippy warnings * Make HandlerOutput/Builder generic over events * Define ModuleEvent * Add AppModule variant to IbcEvent * Impl Display for Acknowledgement * Derive serde for ModuleId * Add ModuleId to ModuleEvent * Impl conversion from tuple for ModuleEventAttribute * Impl conversions from ICS20 events to ModuleEvent * Add event for transfer * Remove bech32 validation from Address * Change MsgTransfer receiver type to Address * Extract MockContext IbcStore * Improve conversion from Signer to AccountId * Add deliver method to Module trait * Implement conversions for MsgTransfer to/from Protobuf Any * Store packet result directly in send_transfer() * Make all ICS20 mods public * Make all IbsStore fields public to enable access for app modules * Implement Ics20Context for DummyTransferModule * Fix failing test * Manual Clone impl for MockContext * Fix failing test * Revert "Fix failing test" This reverts commit 40aec61. * Fix MockContext Clone impl * Update trait definitions after merge with master * Replace ModuleOutput with ModuleOutputBuilder in callbacks * Add inout param ModuleOutputBuilder to Ics20 callback handlers * Emit ICS20 receive packet events * Module::deliver() must be able to emit IbcEvents * Allow HandlerOutputBuilder to merge HandlerOutput * Emit transfer event * Add AckStatusEvent * Emit ICS20 ack events * Emit ICS20 timeout events * Remove ABCI error code * Remove #[allow(unused)] * Add log for send transfer * MsgReceipt abstraction * Handle missing IbcEvent to AbciEvent conversions for RecvPacket and TimeoutOnClose events * Implement ModuleEvent to AbciEvent conversion * Make send_transfer() public * Allow empty TracePaths * Fix TracePath multiple prefix parse bug * Fix TracePath empty str parse bug * Improve DenomTrace FromStr * Add denom validation test * Add denom trace and serde tests * TracePath tests * Allow Denom with '/' * Fix HashedDenom FromStr impl for empty hash * Minor refactoring * Add IbcCoin tests * Add .changelog entry * Fix clippy errors * Disallow empty Signer and replace Address with it * Use ToString as trait bound for ICS20 AccountId * Change AccountId trait bound from FromStr to TryFrom<Signer> * Remove Signer::new() * Delete OCap related TODO * Remove the PortKeeper * Remove Module::deliver() * Fix clippy warnings * Rename transfer module * Rename mod relay * Fix tests failing due to empty signer * Rename PORT_ID const to PORT_ID_STR * Fix escrow addr gen * Test cosmos escrow addr gen * Rename receiver account * Remove `has_denom_trace()` * Remove `BankReader::is_blocked_account()` * Remove `AccountId::ToString` bound * Remove `AccountReader` * Mint/burn into user accounts directly * Use `chunks_exact()` instead of `windows().step_by()` * Rename `DenomTrace::has_prefix()` to `trace_starts_with()` * Move trace related methods into `TracePath` * Fix add/remove prefix * Add test for add/remove trace prefix * Set denom trace only if not already set * Use truncate instead of drain in cosmos_adr028_escrow_address() * Rename DenomTrace as PrefixedDenom * Modify all `BankKeeper` methods to use `PrefixedCoin` instead of `IbcCoin` * Rename `Denom` as `BaseDenom` * Impl Into<U256> and checked_add/sub() for Amount * Add more TracePath tests * Test TracePath is_empty() * Remove IbcCoin * Remove HashedDenom and HashedCoin * Fix test compilation * Add comment for send_transfer() * Functions for determining source chain * Minor refactoring * cargo fmt * Derive serde for PacketCommitment and AcknowledgementCommitment * docstring and comment Co-authored-by: Philippe Laferriere <plafer@protonmail.com> * Merge commands `keys add` and `keys restore` into a single command. (informalsystems#2251) * Merged commands 'keys add' and 'keys restore' into a single command, 'keys add'. Improved restoring a key with mnemonic by taking a file containing the mnemonic as input instead of taking the mnemonic as command line input * Updated Hermes guide with new merged 'keys add' command. And updated changelog with improvement * Fixed e2e setup script to take correct flag when adding keys * Fixed small errors in Hermes guide and improved comments for changed 'keys add' command. * Improved Hermes guide section 7.2 * Updated the Hermes guide section 7.2 to match actual features * Bump moka from 0.8.4 to 0.8.5 (informalsystems#2246) Bumps [moka](https://github.com/moka-rs/moka) from 0.8.4 to 0.8.5. - [Release notes](https://github.com/moka-rs/moka/releases) - [Changelog](https://github.com/moka-rs/moka/blob/master/CHANGELOG.md) - [Commits](moka-rs/moka@v0.8.4...v0.8.5) --- updated-dependencies: - dependency-name: moka dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Adi Seredinschi <adi@informal.systems> * fmt * rename LightClientContext to ReaderContext * use host hash function in escrow address * minor fix * minor fix Co-authored-by: Romain Ruetschi <romain@informal.systems> Co-authored-by: Soares Chen <soares.chen@maybevoid.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Sean Chen <seanchen11235@gmail.com> Co-authored-by: Adi Seredinschi <adi@informal.systems> Co-authored-by: Luca Joss <43531661+ljoss17@users.noreply.github.com> Co-authored-by: Luca Joss <luca.joss@arcanite.ch> Co-authored-by: Ranadeep Biswas <mail@rnbguy.at> Co-authored-by: Luca Joss <luca@informal.systems> Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com> Co-authored-by: Philippe Laferriere <plafer@protonmail.com>

…ms#2118) * Pull in upstream changes * Adding integration test for execute_schedule * Adding integration test for execute_schedule * Finish stubbing out execute_schedule test * Call `chains.shutdown` method * Correctly shut down chain * Shut down node b as well * Debugging execute_schedule test * Add Debug derivations * Update integration test so that its flow correctly tests `execute_schedule` * Attempt to perform a second IBC transfer * Remove info's * Increase sleep timeout duration * Incorportate new test framework features * Remove unnecessary `sleep` call * Correctly use new test framework features * Get assertions passing for now * Send two transactions, one in each direction * Add doc comment for test * Improve panic messages * Refactor test so that it is actually testing the desired behavior * Attempt at fixing `execute_schedule` leaky logic * Flesh out doc comments some more * Remove a duplicate function * Make use of OperationalDataTarget enum * Remove redundant enum * Remove some Debug derives * Remove one more debug derive * Add `try_fetch_scheduled_operational_data` back in * Give `do_execute_schedule` a more descriptive name * Improve `execute_schedule_for_target_chain` method's documentation * Add a bunch of clarifying comments * More clarification of comments * Flesh out `OperationalData` docs * Add changelog entry * Incorporate PR feedback Co-authored-by: Adi Seredinschi <adi@informal.systems>

seanchen1991 and others added 6 commits April 22, 2022 09:48

Pull in upstream changes

042480e

Merge branch 'informalsystems:master' into master

69a1f8f

Adding integration test for execute_schedule

83b60d7

Adding integration test for execute_schedule

60edac7

Finish stubbing out execute_schedule test

5b26c2f

Call chains.shutdown method

37130a3

seanchen1991 assigned soareschen Apr 25, 2022

seanchen1991 added 18 commits April 25, 2022 11:50

Correctly shut down chain

c032b4f

Shut down node b as well

a561715

Debugging execute_schedule test

fe08ba3

Add Debug derivations

37d96b7

Merge branch 'master' of https://github.com/informalsystems/ibc-rs in…

e58a6e1

…to execute-schedule-test Merge in upstream changes from master

Update integration test so that its flow correctly tests `execute_sch…

a1267f4

…edule`

Attempt to perform a second IBC transfer

1fc709d

Remove info's

cfc7c70

Increase sleep timeout duration

4c327b2

Merge branch 'master' of https://github.com/informalsystems/ibc-rs in…

40a3ef3

…to execute-schedule-test Merge in changes from master

Incorportate new test framework features

dbe86ca

Remove unnecessary sleep call

ac8532d

Correctly use new test framework features

c3b0858

Get assertions passing for now

7b50b16

Send two transactions, one in each direction

de2ea25

Add doc comment for test

543cfa7

Improve panic messages

8cb0aca

Refactor test so that it is actually testing the desired behavior

3ddc4ef

seanchen1991 changed the title ~~Add integration test for execute_schedule method~~ Fix execute_schedule method leaking operational data May 20, 2022

seanchen1991 added 4 commits May 20, 2022 12:25

Attempt at fixing execute_schedule leaky logic

dedce23

Flesh out doc comments some more

a948c3f

Remove a duplicate function

0321f0e

Make use of OperationalDataTarget enum

142feea

seanchen1991 added 2 commits May 20, 2022 14:47

Remove some Debug derives

ef2014c

Remove one more debug derive

ef3f498

seanchen1991 marked this pull request as ready for review May 20, 2022 19:49

seanchen1991 requested review from adizere, romac, ancazamfir and soareschen as code owners May 20, 2022 19:49

seanchen1991 added 5 commits May 20, 2022 14:55

Merge branch 'master' into execute-schedule-test

5afe71e

Merge branch 'master' of https://github.com/informalsystems/ibc-rs in…

e102db1

…to execute-schedule-test

Merge branch 'execute-schedule-test' of github.com:seanchen1991/ibc-r…

8a3ed6b

…s into execute-schedule-test

Add try_fetch_scheduled_operational_data back in

77040d8

Give do_execute_schedule a more descriptive name

6a7bf0e

seanchen1991 commented May 23, 2022

View reviewed changes

relayer/src/link/relay_path.rs Show resolved Hide resolved

Improve execute_schedule_for_target_chain method's documentation

402b2c0

adizere reviewed May 24, 2022

View reviewed changes

seanchen1991 added 3 commits May 24, 2022 11:43

Add a bunch of clarifying comments

26f61d0

More clarification of comments

58d78cf

Flesh out OperationalData docs

746d990

seanchen1991 commented May 24, 2022

View reviewed changes

relayer/src/link/operational_data.rs Outdated Show resolved Hide resolved

Add changelog entry

d4342fe

adizere reviewed May 26, 2022

View reviewed changes

relayer/src/link/operational_data.rs Outdated Show resolved Hide resolved

relayer/src/link/relay_path.rs Outdated Show resolved Hide resolved

Incorporate PR feedback

b9e2af3

adizere assigned adizere and unassigned soareschen May 27, 2022

adizere approved these changes May 27, 2022

View reviewed changes

Merge branch 'master' into execute-schedule-test

e4dedbe

adizere merged commit f13e8f5 into informalsystems:master May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `execute_schedule` method leaking operational data #2118

Fix `execute_schedule` method leaking operational data #2118

seanchen1991 commented Apr 25, 2022 •

edited

Loading

adizere May 24, 2022

seanchen1991 May 24, 2022

adizere May 24, 2022

seanchen1991 May 24, 2022

adizere left a comment

adizere left a comment

	/// The destination chain.
	/// The destination chain, i.e., the chain on the receiving side of packets.

Fix execute_schedule method leaking operational data #2118

Fix execute_schedule method leaking operational data #2118

Conversation

seanchen1991 commented Apr 25, 2022 • edited Loading

Description

PR author checklist:

Reviewer checklist:

adizere May 24, 2022

Choose a reason for hiding this comment

seanchen1991 May 24, 2022

Choose a reason for hiding this comment

adizere May 24, 2022

Choose a reason for hiding this comment

seanchen1991 May 24, 2022

Choose a reason for hiding this comment

adizere left a comment

Choose a reason for hiding this comment

adizere left a comment

Choose a reason for hiding this comment

Fix `execute_schedule` method leaking operational data #2118

Fix `execute_schedule` method leaking operational data #2118

seanchen1991 commented Apr 25, 2022 •

edited

Loading