diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md new file mode 100644 index 000000000..8bf5af3e2 --- /dev/null +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -0,0 +1,349 @@ +# ADR 009: Revamp IBC integration test framework + +## Changelog + +- 04-03-2024: Initial draft + +## Context + +The current framework in the IBC testkit uses +[existing types and injects state and/or dependencies +manually](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/tests/core/ics02_client/update_client.rs#L574-L578). +Sometimes, it uses +[semantically incorrect data as mock data](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L320). +Because of this, tests with customizable steps and fixed fixtures became ad-hoc +and unmaintainable. + +To overcome this, we need to improve our test framework such that it allows: + +- testing different implementations and traits. +- composition of succinct tests with useful `util` methods. +- validating code paths that were not easily testable before, i.e. Merkle proof + generation. +- simulating a more realistic IBC workflow, using real IBC and relayer + interfaces. + +## Decision + +The main goal of this proposal is to create a test framework that is modular and +closer to a real blockchain environment. This should also make the existing +tests more succinct and readable. Instead of bootstrapping the mock data that +tests use, we should use valid steps to generate it - so that we know the exact +steps to reach a state to reproduce in a real environment. + +To achieve this, we have broken down the proposal into sub-proposals: + +### 1. Adopt a Merkle store for the test framework + +The current framework uses `HashMap` and `HashSet` to store data. This works for +many test scenarios, but it fails to test proof-sensitive scenarios. Because of +this, we don't have any connection, channel handshake, or packet relay tests +that cover the Tendermint light client. + +We generalize +[`MockContext`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L103) +to use a Merkle store which is used for IBC Context's store. For concrete or +default implementations, we can use the IAVL Merkle store implementation from +`informalsystems/basecoin-rs`. + +### 2. Modularize the host environment + +Currently, we are using `Mock` and `SyntheticTendermint` variants of the +[`HostType`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L33-L36) +enum as host environments. To manage these two different environments, we also +introduced +[`HostBlocks`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L72-75) +for encapsulating the possible host variants that need to be covered by +`ibc-testkit`. + +However, this creates friction in the case when we need to add new host +variants. It creates the same problem that the `ibc-derive` crate was designed +to solve for `ClientState` and `ConsensusState` types, namely: dispatching +methods to underlying variants of a top-level enum. But, a concrete +`MockContext` will always have a unique `HostType` and its corresponding +`HostBlocks`. So we can refactor `HostTypes` and `HockBlocks` with a generic +`TestHost` trait that maintains its corresponding types e.g. `Block` types, via +associated types. Finally, we generalize the `MockContext` once more to use this +`TestHost` trait instead of a concrete enum variant. + +This `TestHost` trait should be responsible for generating blocks, headers, +client, and consensus states specific to that host environment. + +```rs +/// TestHost is a trait that defines the interface for a host blockchain. +pub trait TestHost: Default + Debug + Sized { + /// The type of block produced by the host. + type Block: TestBlock; + + /// The type of client state produced by the host. + type ClientState: Into + Debug; + + /// The type of block parameters to produce a block. + type BlockParams: Debug + Default; + + /// The type of light client parameters to produce a light client state. + type LightClientParams: Debug + Default; + + /// The history of blocks produced by the host chain. + fn history(&self) -> &Vec; + + /// Commit a block with commitment root to the blockchain, by extending the history of blocks. + fn commit_block( + &mut self, + commitment_root: Vec, + block_time: Duration, + params: &Self::BlockParams, + ); + + /// Generate a block at the given height and timestamp, using the provided parameters. + fn generate_block( + &self, + commitment_root: Vec, + height: u64, + timestamp: Timestamp, + params: &Self::BlockParams, + ) -> Self::Block; + + /// Generate a client state using the block at the given height and the provided parameters. + fn generate_client_state( + &self, + latest_height: &Height, + params: &Self::LightClientParams, + ) -> Self::ClientState; +} + +/// TestBlock is a trait that defines the interface for a block produced by a host blockchain. +pub trait TestBlock: Clone + Debug { + /// The type of header that can be extracted from the block. + type Header: TestHeader; + + /// The height of the block. + fn height(&self) -> Height; + + /// The timestamp of the block. + fn timestamp(&self) -> Timestamp; + + /// Extract the header from the block. + fn into_header(self) -> Self::Header; +} + +/// TestHeader is a trait that defines the interface for a header +/// submitted by relayer from the host blockchain. +pub trait TestHeader: Clone + Debug + Into { + /// The type of consensus state can be extracted from the header. + type ConsensusState: ConsensusState + Into + From + Clone + Debug; + + /// The height of the block, as recorded in the header. + fn height(&self) -> Height; + + /// The timestamp of the block, as recorded in the header. + fn timestamp(&self) -> Timestamp; + + /// Extract the consensus state from the header. + fn into_consensus_state(self) -> Self::ConsensusState; +} +``` + +### 3. Decoupling IbcContext and Host environment + +Currently, `MockContext` implements the top-level +[validation](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-core/ics25-handler/src/entrypoint.rs#L45) +and +[execution](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-core/ics25-handler/src/entrypoint.rs#L112) +contexts of `ibc-rs`, as opposed to the more granular contexts of each of the +individual handlers. It contains other host-specific data e.g. `host_chain_id`, +`block_time` - that are not directly relevant to the IBC context. If we think of +`MockContext` as a real blockchain context, the `MockContext` represents the +top-level runtime; it contains `MockIbcStore`, which is a more appropriate +candidate to implement the validation and execution contexts for than the +`MockContext` itself. + +With this, the `MockContext` contains two decoupled parts - the host and the IBC +module. + +### 4. Chain-like interface for `MockContext` + +With the above changes, we can refactor the `MockContext` to have +blockchain-like interfaces. + +The `MockContext` should have `end_block`, `produce_block`, and `begin_block` to +mimic real blockchain environments, such as the Cosmos-SDK. + +```rs +impl MockContext +where + S: ProvableStore + Debug, + H: TestHost, +{ + // Access ibc module store + pub fn ibc_store_mut(&mut self) -> &mut MockIbcStore; + + // Advance the first block height. + pub fn advance_genesis_height(&mut self, genesis_time: Timestamp); + // Routine procedures at the beginning of a block + // Just after committing last block state. + pub fn begin_block(&mut self); + // Advance the current block height. + pub fn advance_block_height(&mut self, block_time: Duration); + // Routine procedures at the end of a block + // Just before committing current block state. + pub fn end_block(&mut self); +} +``` + +### 5. ICS23 compatible proof generation + +With the new proof generation capabilities, we can now test the Tendermint light +clients. But we need our proofs to be ICS23 compatible. ICS23 expects the IBC +store root to be committed at a commitment prefix at a top-level store in the +host environment. + +For this, we add an extra store in `MockContext` where the `MockIbcStore` +commits its storage root at its +[commitment prefix](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/core_ctx.rs#L127-L129) +key. + +So the `MockContext` is finalized as: + +```rs +pub struct MockContext +where + S: ProvableStore + Debug, + H: TestHost +{ + pub main_store: S, + pub host: H, + pub ibc_store: MockIbcStore, +} +``` + +Now the `MockIbcStore` can generate proofs that contain the proofs in its store +and commitment prefix. But it has to know the proofs of its commitment prefix of +the previous heights. + +So we add an extra store in `MockIbcStore` to store the proofs from previous +heights. This is similar to storing `HostConsensusState`s of previous heights. + +```rs +#[derive(Debug)] +pub struct MockIbcStore +where + S: ProvableStore + Debug, +{ + ... + /// Map of host consensus states. + pub host_consensus_states: Arc>>, + /// Map of proofs of ibc commitment prefix. + pub ibc_commiment_proofs: Arc>>, +} +``` + +The storing of the IBC store root at the IBC commitment prefix happens in the +`end_block` procedure. `produce_block` commits the main store, produces a block +with its latest root, and pushes the block to the blockchain. The storing of +proofs and host consensus states happens in the `begin_block` of the +`MockContext`. + +### 6. Integration Tests via `RelayerContext` + +With all the above changes, we can now write an integration test that tests the +IBC workflow - client creation, connection handshake, channel handshake, and +packet relaying for any host environment that implements `TestHost`. + +This can be done by reading the +[IBC events](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L95) +from `MockIbcStore` and creating and sending the IBC messages via +[`MockContext::deliver`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L696). + +### Miscellaneous + +To achieve blockchain-like interfaces, we removed `max_history_size` and +`host_chain_id` from `MockContext`. + +- `max_history_size`: We generate all the blocks till a block height. This gives + us reproducibility. If we need to prune some older block data, we use a + dedicated `prune_block_till` to prune older blocks. This makes our tests more + descriptive about the assumption of the test scenarios. +- `host_chain_id`: The IBC runtime does not depend on `host_chain_id` directly. + The `TestHost` trait implementation is responsible for generating the blocks + with the necessary data. + +Also to minimize verbosity while writing tests (as Rust doesn't support default +arguments to function parameters), we want to use some parameter builders. For +that, we can use the [`TypedBuilder`](https://crates.io/crates/typed-builder) +crate. + +## Status + +Proposed + +## Consequences + +This ADR pays the technical debt of the existing testing framework. + +### Positive + +Future tests will be more readable and maintainable. The test framework becomes +modular and leverages Rust's trait system. `ibc-rs` users may benefit from this +framework, which allows them to test their host implementations of `ibc-rs` +components. + +### Negative + +This requires a significant refactoring of the existing tests. Since this may +take some time, the parallel development on the `main` branch may conflict with +this work. + +## References + +This work is being tracked at +[cosmos/ibc-rs#1109](https://github.com/cosmos/ibc-rs/pull/1109). + +The following provides the concrete implementations of the proposed changes: + +#### MockIbcStore + +The modified `MockIbcStore` with Merkle store lives at +[`testapp/ibc/core/types.rs`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/testapp/ibc/core/types.rs#L43-L96). + +#### TestHost + +The Rust trait lives at +[`hosts/mod.rs`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/hosts/mod.rs#L27). +The `Mock` and `Tendermint` host implementations live in +[`hosts/mock.rs`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/hosts/mock.rs#L30) +and +[`hosts/tendermint.rs`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/hosts/tendermint.rs#L42) +respectively. + +#### Renaming `MockContext` to `StoreGenericTestContext` + +There was confusion about what is a _Test_ component and what is a _Mock_ +component. We have `MockContext` with `MockClient` and `TendermintClient`. + +To avoid this confusion, we renamed `MockContext` to `StoreGenericTestContext`. +This means that going forward all our general frameworks and traits should have +`Test` in their name. But a dummy concrete implementation of these traits may +have `Mock` in their name. + +#### StoreGenericTestContext + +[`StoreGenericTestContext`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/context.rs#L34-L52) +is actually what is described as `MockContext` in the ADR. For convenience, we +defined `TestContext` to have a concrete store implementation - +[`MockStore`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/context.rs#L55-L56). + +```rs +// A mock store type using basecoin-storage implementations. +pub type MockStore = RevertibleStore>; + +pub type TestContext = StoreGenericTestContext; +``` + +With this, we can now define `MockContext` which uses `MockStore` and `MockHost` +and `TendermintContext` which uses `MockStore` and `TendermintHost`. + +```rs +pub type MockContext = TestContext; +pub type TendermintContext = TestContext; +``` diff --git a/ibc-testkit/src/context.rs b/ibc-testkit/src/context.rs index 846182e8c..74d141f2e 100644 --- a/ibc-testkit/src/context.rs +++ b/ibc-testkit/src/context.rs @@ -131,7 +131,7 @@ where } else { // Repeatedly advance the host chain height till we hit the desired height while self.host.latest_height().revision_height() < target_height.revision_height() { - self.advance_height() + self.advance_block_height() } } self @@ -143,8 +143,8 @@ where /// But it bootstraps the genesis block by height 1 and `genesis_time`. /// /// The method starts and ends with [`Self::end_block`] and [`Self::begin_block`], just - /// like the [`Self::advance_height_with_params`], so that it can advance to next height - /// i.e. height 2 - just by calling [`Self::advance_height_with_params`]. + /// like the [`Self::advance_block_height_with_params`], so that it can advance to next height + /// i.e. height 2 - just by calling [`Self::advance_block_height_with_params`]. pub fn advance_genesis_height(&mut self, genesis_time: Timestamp, params: &H::BlockParams) { self.end_block(); @@ -166,8 +166,9 @@ where /// Begin a new block on the context. /// - /// This method book keeps the data from last block, - /// and prepares the context for the next block. + /// This method commits the required metadata from the last block generation + /// and consensus, and prepares the context for the next block. This includes + /// the latest consensus state and the latest IBC commitment proof. pub fn begin_block(&mut self) { let consensus_state = self .host @@ -230,15 +231,19 @@ where /// Advances the host chain height by ending the current block, producing a new block, and /// beginning the next block. - pub fn advance_height_with_params(&mut self, block_time: Duration, params: &H::BlockParams) { + pub fn advance_block_height_with_params( + &mut self, + block_time: Duration, + params: &H::BlockParams, + ) { self.end_block(); self.commit_state_to_host(block_time, params); self.begin_block(); } /// Convenience method to advance the host chain height using default parameters. - pub fn advance_height(&mut self) { - self.advance_height_with_params( + pub fn advance_block_height(&mut self) { + self.advance_block_height_with_params( Duration::from_secs(DEFAULT_BLOCK_TIME_SECS), &Default::default(), ) @@ -479,7 +484,7 @@ where self.dispatch(msg) .map_err(RelayerError::TransactionFailed)?; // Create a new block. - self.advance_height(); + self.advance_block_height(); Ok(()) } @@ -570,7 +575,7 @@ mod tests { let current_height = test.ctx.latest_height(); // After advancing the chain's height, the context should still be valid. - test.ctx.advance_height(); + test.ctx.advance_block_height(); assert!( test.ctx.host.validate().is_ok(), "failed in test [{}] {} while validating context {:?}", diff --git a/ibc-testkit/src/fixtures/core/context.rs b/ibc-testkit/src/fixtures/core/context.rs index 7810c6b77..99304f0e0 100644 --- a/ibc-testkit/src/fixtures/core/context.rs +++ b/ibc-testkit/src/fixtures/core/context.rs @@ -80,7 +80,7 @@ where ); for block_params in params.block_params_history { - context.advance_height_with_params(params.block_time, &block_params); + context.advance_block_height_with_params(params.block_time, &block_params); } assert_eq!( diff --git a/ibc-testkit/src/hosts/mod.rs b/ibc-testkit/src/hosts/mod.rs index 3a4183401..426b69066 100644 --- a/ibc-testkit/src/hosts/mod.rs +++ b/ibc-testkit/src/hosts/mod.rs @@ -138,7 +138,8 @@ pub trait TestBlock: Clone + Debug { } } -/// TestHeader is a trait that defines the interface for a header corresponding to a host blockchain. +/// TestHeader is a trait that defines the interface for a header +/// submitted by relayer from the host blockchain. pub trait TestHeader: Clone + Debug + Into { /// The type of consensus state can be extracted from the header. type ConsensusState: ConsensusState + Into + From + Clone + Debug; diff --git a/ibc-testkit/src/relayer/integration.rs b/ibc-testkit/src/relayer/integration.rs index 2a29feb16..85df3cd9c 100644 --- a/ibc-testkit/src/relayer/integration.rs +++ b/ibc-testkit/src/relayer/integration.rs @@ -122,7 +122,7 @@ where .expect("successfully created send_packet"); // send_packet wasn't committed, hence produce a block - relayer.get_ctx_a_mut().advance_height(); + relayer.get_ctx_a_mut().advance_block_height(); // retrieve the send_packet event let Some(IbcEvent::SendPacket(send_packet_event)) = relayer diff --git a/ibc-testkit/src/relayer/utils.rs b/ibc-testkit/src/relayer/utils.rs index af93f6085..9d4c96bd6 100644 --- a/ibc-testkit/src/relayer/utils.rs +++ b/ibc-testkit/src/relayer/utils.rs @@ -103,7 +103,7 @@ where /// Advances the block height on `A` until it catches up with the latest timestamp on `B`. pub fn sync_clock_on_a(ctx_a: &mut TestContext, ctx_b: &TestContext) { while ctx_b.latest_timestamp() > ctx_a.latest_timestamp() { - ctx_a.advance_height(); + ctx_a.advance_block_height(); } } diff --git a/ibc-testkit/tests/core/ics02_client/recover_client.rs b/ibc-testkit/tests/core/ics02_client/recover_client.rs index 8f6f804b0..28f1de076 100644 --- a/ibc-testkit/tests/core/ics02_client/recover_client.rs +++ b/ibc-testkit/tests/core/ics02_client/recover_client.rs @@ -100,7 +100,7 @@ fn setup_client_recovery_fixture( // Let the subject client state expire. while ctx.latest_timestamp() <= subject_client_trusted_timestamp { - ctx.advance_height(); + ctx.advance_block_height(); } // at this point, the subject client should be expired. diff --git a/ibc-testkit/tests/core/ics02_client/update_client.rs b/ibc-testkit/tests/core/ics02_client/update_client.rs index e74f10fa9..9dbc49030 100644 --- a/ibc-testkit/tests/core/ics02_client/update_client.rs +++ b/ibc-testkit/tests/core/ics02_client/update_client.rs @@ -258,7 +258,7 @@ fn test_consensus_state_pruning() { let update_height = ctx.latest_height(); - ctx.advance_height(); + ctx.advance_block_height(); let block = ctx.host_block(&update_height).unwrap().clone(); let mut block = block.into_header(); @@ -1433,7 +1433,7 @@ fn test_expired_client() { while ctx.ibc_store.host_timestamp().expect("no error") < (timestamp + trusting_period).expect("no error") { - ctx.advance_height(); + ctx.advance_block_height(); } let client_state = ctx.ibc_store.client_state(&client_id).unwrap(); @@ -1488,11 +1488,11 @@ fn test_client_update_max_clock_drift() { while ctx_b.ibc_store.host_timestamp().expect("no error") < (ctx_a.ibc_store.host_timestamp().expect("no error") + max_clock_drift).expect("no error") { - ctx_b.advance_height(); + ctx_b.advance_block_height(); } // include current block - ctx_b.advance_height(); + ctx_b.advance_block_height(); let update_height = ctx_b.latest_height();