paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.78k stars 643 forks source link

`storage_changes_notification_stream` is not properly documented #1989

Open tdelabro opened 11 months ago

tdelabro commented 11 months ago

Hey,

I'm using an old polkadot-sdk version: git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.43", but I don't think that is the problem.

I'm trying to use storage_changes_notification_stream.

First thing, there is no documentation or example whatsoever. The only indication we have is that None will act as a wildcard.

My goal is to listen to any change in a specific storage map of my pallet. If the value under any key is updated, I want a notification.

I call

storage_changes_notification_stream(
  None,
  Some(&[
    ([twox_128(MY_PALLET_NAME), twox_128(MY_STORAGE_NAME)].concat(), None)]]))

Which, I hoped would express "don't do any filtering on the first arg, but do some with the second".

But it looks like because the first argument is None it just gets me all the storage changes and my second arg is entirely ignored:

StorageChangeSet {
  changes: [<a bunch of storage changes from a lot of different storages, including mine, but not only>],
  child_changes: [], 
  filter: None,
  child_filters: Some({StorageKey([19, 67, 5, 109, 125, 191, 20, 108, 100, 100, 215, 242, 163, 70, 163, 175, 239, 212, 57, 76, 203, 26, 39, 142, 224, 180, 92, 201, 69, 43, 155, 93]): None}) } // <- my key

So, I tried the following:

storage_changes_notification_stream(
  Some(&[]),
  Some(&[
    ([twox_128(MY_PALLET_NAME), twox_128(MY_STORAGE_NAME)].concat(), None)]]))

Which, because the filters args seem to overwrite the child_filters one, seems like the only syntax that could express "don't do any filtering with the first arg, but keep doing some with the second arg".

And I get nothing, no value is ever pushed to the stream.

I don't think I'm messing with my const, they are used on other parts of the node to directly access the storage without going through the runtime. So, either I'm wrong on the way to pass an argument to this method (which is really possible given there is no documentation or examples), or the child_filter part just straight up does not work. Wdyt?

tdelabro commented 11 months ago

Also, I looked at the code.

impl StorageChangeSet {
    /// Convert the change set into iterator over storage items.
    pub fn iter(
        &self,
    ) -> impl Iterator<Item = (Option<&StorageKey>, &StorageKey, Option<&StorageData>)> + '_ {
        let top = self
            .changes
            .iter()
            .filter(move |&(key, _)| match self.filter {
                Some(ref filter) => filter.contains(key),
                None => true,
            })
            .map(move |(k, v)| (None, k, v.as_ref()));
        let children = self
            .child_changes
            .iter()
            .filter_map(move |(sk, changes)| {
                self.child_filters.as_ref().and_then(|cf| {
                    cf.get(sk).map(|filter| {
                        changes
                            .iter()
                            .filter(move |&(key, _)| match filter {
                                Some(ref filter) => filter.contains(key),
                                None => true,
                            })
                            .map(move |(k, v)| (Some(sk), k, v.as_ref()))
                    })
                })
            })
            .flatten();
        top.chain(children)
    }
}

I'm still not sure what it does exactly. It combines the two sources of storage changes, but in a way I cannot understand because the code is not expressive, not commented, and not documented.

The return it a tuple on the form (Option<prefix_storage_key>, full_storage_key, Option<new_data>), but when I run the iterator, the first parameter, the one I wan't to use to later match and take action depending on which storage has been updated, is always None because all values come from changes and none from child_changes

tdelabro commented 11 months ago

Also, I wasn't able to find in the code the place where ChildChangeset is initialized and based on what. So I'm not able to guess what I could change in order to have this field filled.

bkchr commented 11 months ago

Are you using a child trie? If not, you need to pass your filter to filter_keys.

tdelabro commented 11 months ago

Hey @bkchr,

I will need more explanation. From my reading of the code and doc here is what is possible:

Because no doc mention the way those arguments interact, I assumed they were additive. Nowhere in the doc was mentioned something about child trie. Given there was no specific instruction about the context it should be used, I assumed it could be used in any context.

Can you please explain to me precisely how this method works, what are those arguments for, how they interact with each other, and in which context it can be used?

And hopefully, add it to the doc later on.

bkchr commented 11 months ago
  • you can pass the beginning of a StorageKey (like pallet + storage but not the key of the map) to child_filters so you can listen to any storage change that happens below this node in the storage

No. In Substrate there is the main trie and multiple child tries. A child trie is for example used for contracts etc. It is an independent trie outside of the main trie. Normal pallets don't use child tries. So, when you want to filter for your pallet key, you need to pass it to the first parameter of this function.

bkchr commented 11 months ago

https://docs.substrate.io/learn/state-transitions-and-storage/#trie-abstraction maybe this helps.

tdelabro commented 11 months ago

@bkchr so, to confirm the first parameter except you to pass the hash of the name of storage that are sub tries: [twox_128("MyPalletName), twox_128("MyStorageWhichIsASubTrieName)].concat()

What is the second argument for?

bkchr commented 11 months ago

https://docs.substrate.io/learn/state-transitions-and-storage/#trie-abstraction maybe this helps.

Have you read this? About child tries?

tdelabro commented 10 months ago

Yes did. However, it doesn't explain how these function parameters should be used.

bkchr commented 10 months ago

I mean yes, we should update the docs. I'm not arguing against that. I just wanted to help you.

storage_changes_notification_stream(
  Some(&[
    ([twox_128(MY_PALLET_NAME), twox_128(MY_STORAGE_NAME)].concat(), None)]]),
    Some(&[]),
)

Probably for now should work.

tdelabro commented 10 months ago
storage_changes_notification_stream(
  Some(&[
    ([twox_128(MY_PALLET_NAME), twox_128(MY_STORAGE_NAME)].concat(), None)]),
    Some(&[]),
)

The first argument is: filter_keys: Option<&[StorageKey]>, it cannot be

Some(&[
    ([twox_128(MY_PALLET_NAME), twox_128(MY_STORAGE_NAME)].concat(), None)
 ])

This looks more like what the second argument could be. Did you mean:

storage_changes_notification_stream(
  Some(&[]),
  Some(&[
    ([twox_128(MY_PALLET_NAME), twox_128(MY_STORAGE_NAME)].concat(), None)]),
)

The first arg makes sure all storages are filtered out The second one passes the StorageKey of the ChildTree root node, and None to listen to anything in it.

I think I get it now.