Deterministic host timestamp #1771
Conversation
hu55a1n1
force-pushed
the
hu55a1n1/1770-deterministic-host-timestamp
branch
from
e1441a8
to
82d84e9
Compare
|
A number of tests are currently failing with |
| let latest_consensus_state = self | ||
| .host_consensus_state(Height::zero()) | ||
| .expect("host must have pending consensus state"); | ||
| latest_consensus_state.timestamp() |
There was a problem hiding this comment.
Do we need a pending_block field just to mock the host timestamp? Would it work if we just get the latest block and add a fixed offset to the timestamp of the latest block?
There was a problem hiding this comment.
Thanks for reviewing @Soares! This is an interesting observation. 👍 IMHO it is still helpful to keep a pending_block. I think by having a pending_block, the mock impl is closer to a real chain context impl and serves as a reference implementation. This makes it easier to convey the expectation that the chain context is in a state where it is ready to execute a pending block and also allows us to use the ClientReader::host_timestamp() provided trait method (just as it is expected to be used by a light client impl).
There was a problem hiding this comment.
I realized that we don't treat the mock context as a ref impl, so your suggestions make sense. 👍 3b93cb8
modules/src/mock/context.rs
Outdated
| ); | ||
|
|
||
| let old_pending_block = replace(&mut self.pending_block, new_block); |
There was a problem hiding this comment.
It does not seem like we need the pending_block to have a cached next block. The only place I see it is used is in host_timestamp, which should be able to mock the time in other way. Here we can just generate the next block on the fly using the same time offset as used by host_timestamp.
There was a problem hiding this comment.
Done! 👍 I also noticed that latest_height wasn't required since we can get the latest height from the history, and we expect the history to be non-empty -> https://github.com/informalsystems/ibc-rs/pull/1771/files#diff-a0183026217183d9147ad2c8fc0827f60c1e4354e9a2bed9da05f5f20fc501b6L842
So, I removed that as well. 04b2ff0
modules/src/mock/context.rs
Outdated
| @@ -991,6 +1016,16 @@ impl ClientReader for MockContext { | |||
| self.latest_height | |||
| } | |||
|
|
|||
| fn host_consensus_state(&self, height: Height) -> Result<AnyConsensusState, Ics02Error> { | |||
| if height.is_zero() { | |||
| return Ok(self.pending_block.clone().into()); | |||
There was a problem hiding this comment.
We should avoid using Height::zero() as the magic parameter for getting the latest height. If that is needed, it is better to define a separate method latest_host_consensus_state. There is also no need for the pending_block here. If it is needed, it looks like the mock context should be able to deterministically generate the next block, so no caching should be needed.
I also don't see host_consensus_state(Height::zero()) being used anywhere other than getting the host timestamp. So perhaps this is not needed if host_timestamp can be improved as suggested.
* Get host timestamp from latest host consensus state * Add .changelog entry * Fix store_update_time/height() impls * Update mock context impl * Use host_timestamp() instead of now * Clarify the meaning of Height::zero() in host_consensus_state() * Implement deterministic timestamps for Mock context * Fix failing test * Fix clippy warnings * Avoid cloning MockContext pending_block * Minor reformating * Address review feedback * Update ConnectionReader and ChannelReader traits for recent timestamp changes * Remove latest_height from mock context
Closes: cosmos/ibc-rs#57
Description
ChannelReader::host_timestamp()is now a provided method that fetches the latest host consensus state usingChannelReader::host_consensus_state()and uses it's timestamp.PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.