Paginating over ClientStates causes endless requests

[webmaster128]

Summary of Bug

I am trying to use the ClientStates query with pagination. However, most result pages contain no elements and nextKey is always non-empty.

Expected Behaviour

Each page of a ClientStates query contains dozens of results by default and after a few pages, the pagination nextKey is unset.

Version

wasmd 0.29.0-rc2
cosmos-sdk 0.45.8
ibc-go 3.3.0

Steps to Reproduce

I query against nois-testnet-003, a network with 48 clients (https://ibc.nois.network/connections). When I do that in CosmJS, I get the following pages. My script stopped after 60 seconds of requests.

Page 1, response count 1, next key: /07-tendermint-0/consensusStates/1-1422008/processedTime
Page 2, response count 0, next key: /07-tendermint-0/consensusStates/1-1422337
Page 3, response count 0, next key: /07-tendermint-0/consensusStates/1-1422584/processedHeight
Page 4, response count 0, next key: /07-tendermint-0/consensusStates/1-1422771/processedTime
Page 5, response count 0, next key: /07-tendermint-0/consensusStates/1-1423200
Page 6, response count 0, next key: /07-tendermint-0/consensusStates/1-1423446/processedHeight
Page 7, response count 0, next key: /07-tendermint-0/consensusStates/1-1423606/processedTime
Page 8, response count 0, next key: /07-tendermint-0/consensusStates/1-1423812
Page 9, response count 0, next key: /07-tendermint-0/consensusStates/1-1424017/processedHeight
Page 10, response count 0, next key: /07-tendermint-0/consensusStates/1-1424206/processedTime

Full output with 816 pages: test.log.

This output is generated from a CosmJS test: https://github.com/cosmos/cosmjs/pull/1352/files.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[webmaster128]

In a different test case performing the same query it looks better:

# Beginning of the test
Page 1, response count: 7, next key: /07-tendermint-6/consensusStates/0-621
Page 2, response count: 3, next key: 

# Client is added here

# End of the test
Page 1, response count: 8, next key: /07-tendermint-6/consensusStates/0-595/processedHeight
Page 2, response count: 3, next key: 

However, it is still unclear why the first page does not contain a constant number of elements.

This is testing against
wasmd 0.27.0
ibc-go 3.0.0

[crodriguezvega]

Thanks for reporting this issue, @webmaster128. I think it's a bit strange that in the next key you get keys with consensusStates, since you are querying for client states and the queried keys should have the format clients/<client-ID>/clientStates.

Looking at the code, I see that we use the query.Paginate function on a store with the prefix clients, so I guess that's what causing to retrieve also keys with consensusStates, since they also share the same clients prefix with the client state keys... @colin-axner would the solution be to use the query.FilteredPaginate function like we do for the ConsensusStates query and filter out the irrelevant keys?

@webmaster128 what's the impact this bug is causing for you? Just asking so that we can prioritize accordingly. And would you need a fix in the v3 release line or could we just release this fix in the next major release (v7)? Please also note that for v3, only the v3.3.x and v3.4.x branches are currently supported.

[webmaster128]

Thank you for the detailed answer.

@webmaster128 what's the impact this bug is causing for you? Just asking so that we can prioritize accordingly.

I think it is safe to call the query practically unusable. However, I don't know which application really needs a list of clients. I wanted to use it for a new detailed home screen of our IBC visualizer tool but I can live without it and just loop over connections instead. At the end of the day this is a nice-to-have debugging tool.

[crodriguezvega]

Thanks for the quick response, @webmaster128!

I think it is safe to call the query practically unusable

Yeah, I think that's fair to say. :)

However, I don't know which application really needs a list of clients.

I think @womensrights could help us to figure this out, and see if there's anybody else impacted (although I guess not, otherwise they would have already opened an issue...).

I wanted to use it for a new detailed home screen of our IBC visualizer tool but I can live without it and just loop over connections instead. At the end of the day this is a nice-to-have debugging tool.

Understood. Then I think, given the information we have at the moment, that we could try to fix and release this with v7 then.

[adizere]

We would like to use pagination also in Hermes eventually. Not an urgent request. From what I recall we always specify pagination::all because we found out that pagination does not typically work. informalsystems/hermes#1816

[colin-axner]

Looking at the code, I see that we use the query.Paginate function on a store with the prefix clients, so I guess that's what causing to retrieve also keys with consensusStates, since they also share the same clients prefix with the client state keys... @colin-axner would the solution be to use the query.FilteredPaginate function like we do for the ConsensusStates query and filter out the irrelevant keys?

It's a good first step. Without significant changes, we can't optimize the iteration over all client store keys to round up all clients, but the issue you pointed out does exist. Paginate counts every key, while FilteredPaginate only counts keys being included in the response by using the bool return argument

I was unaware of this before, so which one we use throughout the codebase is likely random. I would recommend all our gRPC's to use FilteredPaginate as I'd presume external clients want to limit the response amount and not necessarily the amount of keys that have to be traversed?

Iterating over clients should be the main place we have to iterate over keys we may not care about. Though it looks to be the case here as well

Lets switch to FilteredPaginate by default and use Pagination with specific intention

Thanks @webmaster128 for the detailed bug report!