Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make localhost a stateless implementation #5959

Closed
colin-axner opened this issue Mar 11, 2024 · 2 comments · Fixed by #6683
Closed

make localhost a stateless implementation #5959

colin-axner opened this issue Mar 11, 2024 · 2 comments · Fixed by #6683
Assignees
Labels
09-localhost type: performance Performance or storage optimizations
Milestone

Comments

@colin-axner
Copy link
Contributor

colin-axner commented Mar 11, 2024

Now that the light client module has the context for all its interface functions, the localhost implementation can be stateless as it should only read off the existing ibc store and not need to store any information under its client store

We should probably add a migration to prune unnecessary state and addition we should remove the additional disc writes that happen on every block to update the height in the localhost client state (set in the client store)

Can be removed:

Originally posted by @colin-axner in #5866 (comment)

@gjermundgaraba
Copy link
Contributor

gjermundgaraba commented Jun 10, 2024

I started looking into this and I found a couple of things that I wanted to discuss around the implementation.

1 Client state verification

After making localhost stateless a direct membership proof of the client state merklePath := commitmenttypes.NewMerklePath(host.FullClientStatePath(exported.LocalhostClientID)) is not possible (since it doesn't exist in state anymore).

To get tests to pass I currently just put a check in to return early with no error if we are trying to verify that.

// The localhost client is stateless, so if we need to verify the localhost client state, we can just return nil to verify
if merklePath.KeyPath[1] == host.FullClientStatePath(ModuleName) {
	return nil
}

Is this the way to do this, or is this actually something we don't need to support and just adjust the tests?

2 Client state queries in 02-client

In 02-client there is also some consequences for quering client states. For getting a single client state, this is easy enough with a clientID check:

if clientID == exported.LocalhostClientID {
	return localhost.NewClientState(types.GetSelfHeight(ctx)), true // Or route the call to the light client module directly
}

But for the paginated call to get ClientStates we don't get the localhost client state anymore from the store prefix.NewStore(ctx.KVStore(k.storeKey), host.KeyClientStorePrefix). My current solution to get tests passing was to just always add it to the list in that query:

clientStates := types.IdentifiedClientStates{types.NewIdentifiedClientState(exported.LocalhostClientID, localhost.NewClientState(types.GetSelfHeight(ctx)))}
// then do the actually query
pageRes, err := query.FilteredPaginate(store, req.Pagination, func(key, value []byte, accumulate bool) (bool, error) {
...

@gjermundgaraba gjermundgaraba self-assigned this Jun 10, 2024
@colin-axner
Copy link
Contributor Author

Great questions

  1. This should not be necessary. Client state membership proofs are currently used during the connection handshake, but the localhost is a special connection which is pre-created. Doing a membership proof on a localhost client, is equivalent to calling localhost.Status() (so I think it's safe to allow it return an error if someone called VerifyMembership on the localhost client state

  2. I think the solution you went with makes sense 👍 Localhost always exists on a chain, it can simply be disabled, but should still be returned in client state queries in the same way non-active clients will be

@gjermundgaraba gjermundgaraba linked a pull request Jun 24, 2024 that will close this issue
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
09-localhost type: performance Performance or storage optimizations
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants