Allow to pass a bound on StorageCountedMap::remove_all (similarly to StorageMap::removal_all)

[gui1117]

StorageMap::remove_all take as argument the limit of key to remove from backend. This is useful when user want to remove all keys from a map but without using too much of the block weight.

StorageCountedMap::remove_all cannot implement this limit easily. Because when the runtime api clear_prefix return the number of key removed from backend, but we don't know how many keys were actually removed (some key are in the overlay and some are in both overlay and backend), so we can't update the counter.

One solution suggested by Shawn is to look at the value of the counter in the backend. and update the counter with: counter in the backend - number of key removed in the backend by this operation.
Maybe this is a way to implement the limit.

context:

• wrong implementation of the limit is removed here Fix pallet bags list and doc substrate#10231
• first failed attempt to fix it is here Improve sp_io::storage::clear_prefix API to return the number of key deleted substrate#10288

[dharjeezy]

Hello @thiolliere

I saw you mentioned the previous issue you closed(#10288) which I attempted on this current issue.
Can you elaborate the differences between that other issue(#10288) and this current issue you opened?
I am willing to pick this issue up but i need to understand what i am to do and the difference between what i attempted in my PR here(paritytech/substrate#10332) earlier?

[dharjeezy]

you there @thiolliere ?

[gui1117]

you there @thiolliere ?

I didn't work last weekend why did you expect me to answer a low priority question ?

EDIT: I also find the tone of your sentence disrespectful.

[gui1117]

Hello @thiolliere

I saw you mentioned the previous issue you closed(#10288) which I attempted on this current issue. Can you elaborate the differences between that other issue(#10288) and this current issue you opened? I am willing to pick this issue up but i need to understand what i am to do and the difference between what i attempted in my PR here(#10332) earlier?

The solution I suggested in the issue paritytech/substrate#10288 doesn't work.
The goal of this issue is to enhance the the method remove_all for StorageCountedMap type.
We want to be able to give a limit on the number of key to remove from the backend (or both backend and overlay, the important point is to be able to give a limit), similarly to the method remove_all for StorageMap.

There might be a way found by Shawn that I explained in the top message. This consist of looking at the number of key in the backend by looking the value of the counter in the backend (maybe by using the parent block hash or something I don't know).

I don't have a precise idea about how to fetch the value of the counter in the backend. But if we have it then we can implement remove_all with some limit, and update the counter with the value: counter in backend minus number of key removed from backend. Because all keys are removed from overlay this should work.

[dharjeezy]

you there @thiolliere ?

I didn't work last weekend why did you expect me to answer a low priority question ?

EDIT: I also find the tone of your sentence disrespectful.

@thiolliere I am sorry for the tone I used, never knew you'd get offended by it. I will do better next time.

[dharjeezy]

Hello @thiolliere
I saw you mentioned the previous issue you closed(#10288) which I attempted on this current issue. Can you elaborate the differences between that other issue(#10288) and this current issue you opened? I am willing to pick this issue up but i need to understand what i am to do and the difference between what i attempted in my PR here(#10332) earlier?

The solution I suggested in the issue paritytech/substrate#10288 doesn't work. The goal of this issue is to enhance the the method remove_all for StorageCountedMap type. We want to be able to give a limit on the number of key to remove from the backend (or both backend and overlay, the important point is to be able to give a limit), similarly to the method remove_all for StorageMap.

There might be a way found by Shawn that I explained in the top message. This consist of looking at the number of key in the backend by looking the value of the counter in the backend (maybe by using the parent block hash or something I don't know).

I don't have a precise idea about how to fetch the value of the counter in the backend. But if we have it then we can implement remove_all with some limit, and update the counter with the value: counter in backend minus number of key removed from backend. Because all keys are removed from overlay this should work.

If i can get you correctly, we need to implement a limit on remove_all function CountedStorageMap right?

The caller of the function will definitely include the limit while calling this function right? or is the remove_all to determine the limit that is to be removed out of the box inside the function?

More so, will one implement a v2 for remove_all or modify the existing one?

[hirschenberger]

Hey @thiolliere , I skimmed through the code and found this comment in the existing remove_all fn.

But it does not seem to be true anymore, as the sp_io::storage::clear_prefix fn does return a KillStorageResult.

Also the comment of sp_io::clear_prefix seems outdated as it mentiones a boolean return value.

So maybe this can now be implemented straight forward?

[gui1117]

@hirschenberger

I think the comment in the code should be updated, it was a mistake from me to think that having the number of key deleted from overlay would be sufficient to implement this feature.

This comment should explain it paritytech/substrate#10288 (comment)

Basically we can have the same key in both overlay and backend. (when they are overriden for example). So even if we know that N and M keys are removed from overlay and backend we don't know how many key we actually deleted from the counted storage map.

[hirschenberger]

Oh, I see. And the approach @shawntabrizi suggested is using a "counter" for the backend to determine how many backend operations happend during the removal and take this number into account for the limit.

What exactly is this counter, is it the read_write_count or usage_info functions. It seems they are more tailored towards bechmarking or telemetry and seem to be a fragile API?

[gui1117]

Oh, I see. And the approach @shawntabrizi suggested is using a "counter" for the backend to determine how many backend operations happend during the removal and take this number into account for the limit.

What exactly is this counter, is it the read_write_count or usage_info functions. It seems they are more tailored towards bechmarking or telemetry and seem to be a fragile API?

Shawn solution was to fetch the value of the counter in the backend itself as it should hold as it shouldn't take into account the value added in the overlay. I don't think sp_io::storage provides a way to read this value. Maybe we would want to introduce sp_io::storage::get_backend which returns the value for a given key but using only the backend.
In general I don't really like having the backend/overlay architecture exposed in the sp_io::storage API, but clear_prefix is already exposing this architecture in the API.