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

state_getKeysPaged Returns duplicate keys #1692

Closed
lexnv opened this issue Feb 27, 2024 · 4 comments · Fixed by #1762
Closed

state_getKeysPaged Returns duplicate keys #1692

lexnv opened this issue Feb 27, 2024 · 4 comments · Fixed by #1762

Comments

@lexnv
Copy link
Contributor

lexnv commented Feb 27, 2024

Using smoldot 0.16 and smoldot-light 0.14 (Updating to latest blocked by #1638 since subxt uses currently the legacy RPC methods)

Repro case:

logs.txt

cc paritytech/subxt#1453

@tomaka
Copy link
Contributor

tomaka commented Mar 6, 2024

I'm not sure I understand your logs, but as far as I understand:

  • You call state_getKeysPaged with a count parameter equal to 10
  • It returns 10 keys
  • You then call state_getKeysPaged a second time and pass for the start_key parameter a value equal to the last key that was yielded
  • The first key that this second call to state_getKeysPaged yields is identical to the start_key, in other words equal to the last key of the previous call

If so, this behavior is actually "intended" by smoldot.
I put "intended" in quotes because I do this on purpose, but since state_getKeysPaged doesn't have a spec it's not clear to me what it should actually do.

See TODO:

.filter(|k| start_key.as_ref().map_or(true, |start| *k >= start.0)) // TODO: not sure if start should be in the set or not?

@jsdw
Copy link

jsdw commented Apr 15, 2024

FWIW Substrate ignores the start_key provided for this API; I (somewhat painstakingly) traced the logic of the Substrate impl to some code where it constructs some KeysIter thing and configures the args there explicitly to ignore it.

I made a small fix in Subxt to ignore such a key if it's returned, since we had a couple of issues raised about this. Would you be up for a PR in Smoldot to skip over the start_key to line up with Substrate?

tomaka added a commit to tomaka/smoldot that referenced this issue Apr 15, 2024
@tomaka tomaka mentioned this issue Apr 15, 2024
@tomaka
Copy link
Contributor

tomaka commented Apr 15, 2024

I was simply waiting for someone's confirmation that Substrate indeed skips the first key in order to open the PR. You could have simply commented here instead of adding a hack in SubXT (paritytech/subxt#1534).

github-merge-queue bot pushed a commit that referenced this issue Apr 15, 2024
* Fix #1692

* PR link
@jsdw
Copy link

jsdw commented Apr 15, 2024

Awesome, thank you for the quick fix!

You could have simply commented here instead of adding a hack in SubXT

Since the fix was super small in Subxt I figured I may as well apply it there since we are a little out of date on Smoldot version anyway, so it'll give us a little breathing room to update it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants