feat(core-client)!: replace store_update_{time,height} by metadata update

[mina86]

Observe that update time and height are always read and manipulated
together. Replace pairs of methods for reading, updating and deleting
time and height with a single combined method for each operation.

Specifically, replace:
a) client_update_{time,height} by client_update_meta,
b) store_update_{time,height} by store_update_meta and
c) delete_update_{time,height} by delete_update_meta.

This allows better optimised implementations which, for example, keep
both pieces of information in a single map and perform a single lookup
to read or update them.

Closes: #973

---

PR author checklist:

• Added changelog entry, using unclog.
• Added tests.
• Linked to GitHub issue.
• Updated code comments and documentation (e.g., docs/).
• Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

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

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ba7b90c) 66.78% compared to head (5d19309) 66.70%.

Files	Patch %	Lines
ibc-clients/ics07-tendermint/src/client_state.rs	66.66%	1 Missing ⚠️
...stkit/src/testapp/ibc/clients/mock/client_state.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #972      +/-   ##
==========================================
- Coverage   66.78%   66.70%   -0.08%     
==========================================
  Files         204      204              
  Lines       20473    20427      -46     
==========================================
- Hits        13672    13626      -46     
  Misses       6801     6801              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Farhad-Shabani]

Thank you @mina86 for pointing those details out. To progress the PR, here are some points to consider:

• Regarding the store/delete-consensus-state methods, note that the client execution context (b/c of generic V type here) can tap into other methods in our main validation context (And, actually Tendermint clients already have access to host_height and host_timestamp). So, the design allows you to intoruce extra methods by defining additional customized validation context, like we did here for our mock implementation. So then, you can go ahead, and call self.host_timestamp()? and self.host_height()? in your store/delete-consensus-state implementation as you require.
  Therefore, no need to enforce all the ibc-rs downstream projects to pass host height and timestamp. Additionally, *_consensus_state, typically points out to ConsensusState structs in our codebase, and passing height and timestamp along with that can cause confusion among users imho. Let's not touch store/delete-consensus-state methods.

• Though, as for merging store/delete_update_time & store/delete_update_client, since it’s turning to an interface with less assumption about host chains, and vibing well with ibc-go APIs, makes sense.

[Farhad-Shabani]

Though, as for merging store/delete_update_time & store/delete_update_client, since it’s turning to an interface with less assumption about host chains, and vibing well with ibc-go APIs, makes sense

@mina86 If you feel up to finishing this PR as well, we can include the second point (above) in our coming release as well.

[mina86]

The change to store/delete-consensus-state was based on the observation that storing or deleting consensus state is always followed by update to the time and height. You say

So then, you can go ahead, and call self.host_timestamp()? and self.host_height()? in your store/delete-consensus-state implementation as you require.

but that means there’s no need for store_update_time and store_update_height at all?

[Farhad-Shabani]

but that means there’s no need for store_update_time and store_update_height at all?

Great question! The rationale behind having these methods is rooted in the need to avoid making assumptions about how light client developers handle client metadata with each ConsensusState update. A client could be designed to manage multiple update clients in a single run. In such a scenario, it can process and store multiple ConsensusState for each update client message but might only store one pair of update time and height at the end. Therefore, the approach allows for flexibility when handling various update scenarios without imposing strict requirements on metadata storage. Additionally, looking at it from the ibc-rs perspective, this gives us distinct access to those parts of the host's store, so can benefit from that in later applications or clients' development.

Btw, this got me thinking — we might be able to skip passing the host_timestamp and host_height to the store_update_meta. Tendermint clients already have internal access, and other clients potentially could follow a similar trait definition.

Since this is an API-breaking change, let's give it more thought to ensure a robust API, so not imposing gradual adjustments on users.

[mina86]

So if I understand correctly, this also means that implementations can effectively ignore store_update_{time,height} calls and instead perform the updates when handling store_consensus_state?

Btw, this got me thinking — we might be able to skip passing the `host_timestamp` and `host_height` to the `store_update_meta`. Tendermint clients already have internal access, and other clients potentially could follow a similar trait definition.

Makes sense to me. I’ve whipped out an alternative PR at <#981>. I think in the end it also means that implementations could have empty store_update_meta implementations provided that they updated the time and height in store_consensus_state.

…

-- Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving»

[Farhad-Shabani]

Hey @mina86. Sorry it took us a bit to circle back to this PR. Now that we've got #1069 merged, feel free to update the type of arguments with the newly defined paths, so can go ahead with this PR. Thanks!

[mina86]

With this PR we actually want to replace the two new paths ClientUpdateTimePath and ClientUpdateHeight with ClientUpdateMetadataPath, no?

[Farhad-Shabani]

With this PR we actually want to replace the two new paths ClientUpdateTimePath and ClientUpdateHeight with ClientUpdateMetadataPath, no?

You've got a good point. I thought in the wrong direction. Let's stick with the current paths and keep the method signatures as is. So that, inside the implementation of these methods, whoever's working on it can decide whether or not to use the path types.

[mina86]

In that case I think this PR is good as is.

Though I struggle to imagine why any implementation would choose use those paths. There’s no benefit in having separate paths for the the height and timestamp since those two pieces of information are always accessed together.

[Farhad-Shabani]

Left a few comments, then all set 🙏

[Farhad-Shabani]

Though I struggle to imagine why any implementation would choose use those paths. There’s no benefit in having separate paths for the the height and timestamp since those two pieces of information are always accessed together.

Good call. I'll check out what's happening with projects around ibc-rs to get a clearer picture so can before the next release, decide if we should keep the current signatures or define e.g. "ClientUpdateMetadataPath".