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

EventSubscription decodes all events in block #201

Closed
gregdhill opened this issue Dec 9, 2020 · 10 comments · Fixed by #227
Closed

EventSubscription decodes all events in block #201

gregdhill opened this issue Dec 9, 2020 · 10 comments · Fixed by #227
Assignees

Comments

@gregdhill
Copy link
Contributor

Polling next() on the EventSubscription first decodes all events in the StorageChangeSet before filtering by the extrinsic index which means that if it encounters a type it does not know how to decode the method will Err(TypeSizeUnavailable).

The following example will panic:

        let (result1, result2) = futures::future::join(
            provider.clone().set_balance(10000000000 as Balance),
            provider.clone().set_custom_type(1 as CustomType)
        ).await;
        result1.unwrap();
        result2.unwrap();
@sander2
Copy link
Contributor

sander2 commented Dec 23, 2020

The example above uses a single client. However, this also occurs when using different clients, using separate accounts in different processes. Crucially, the more active a blockchain is used, the more likely these decoding errors will occur. This makes scaling up really problematic. This is, in my opinion, a critical bug..

@ascjones ascjones self-assigned this Feb 5, 2021
@ascjones
Copy link
Contributor

ascjones commented Feb 5, 2021

I'm investigating this

@ascjones
Copy link
Contributor

ascjones commented Feb 5, 2021

The fundamental problem is this: the raw data: Vec<u8> of a StorageChangeSet::changes item is an encoded list of EventRecords, which contains the Phase, the event itself and the list of topics.

Because of SCALE encoding we don't know up front the size of each encoded EventRecord so we need to decode or skip over each instance with the metadata we have available, even just to be able to filter by the extrinsic index which is stored in the Phase.

So unless the events related to your extrinsic are first in the list in the storage changeset, and if any of the preceding events contain any types whose size in unknown then we are unable to skip over them to arrive at the events you are interested in.

We are extremely aware of this limitation and in fact the focus of my work currently is the integration of scale-info type metadata into substrate which will solve this problem by having full metadata for all types.

However this is some months away from being ready for production, although we are working flat out to have it ready as soon as possible.

In the meantime, the simple but extremely imperfect workaround is simply to register all the unknown type sizes from your runtime on intialization of the events decoder..

I haven't done this in a while but it is worth looking at the mappings in https://github.com/polkadot-js/api. I can investigate whether there is an easy way to precompute any missing types from this.

@sander2
Copy link
Contributor

sander2 commented Feb 6, 2021

Thanks for looking into this. Embedding metadata (specifically, the length of the event) seems indeed to be the best way forward. As an aside, using some kind of framing (e.g. COBS) would also work to be able to skip over events that fail to decode.

Anyway, I'm afraid your workaround is not sufficient - we do this already for explicit event listeners, but the problem is that subxt calls (the _and_watch functions) implicitly register a new event listener - one that we can not register additional type sizes to.

@ascjones
Copy link
Contributor

ascjones commented Feb 6, 2021

but the problem is that subxt calls (the _and_watch functions) implicitly register a new event listener

Yes that is a problem, I will look at ways of improving the API to allow registering of global type sizes on Monday.

@ascjones
Copy link
Contributor

@gregdhill @sander2 please have a look at #227, and let me know whether that is an acceptable solution for now.

@gregdhill
Copy link
Contributor Author

Looks good @ascjones, when do you think this will be ready?

@ascjones
Copy link
Contributor

It just needs to be reviewed and merged now, so should be this week hopefully.

@dvdplm
Copy link
Contributor

dvdplm commented Feb 16, 2021

Looks good @ascjones, when do you think this will be ready?

@gregdhill It would be good if someone from Interlay could take a look at the proposed code and try it out with your code to make sure it addresses the issues you have reported. Is that possible?

@sander2
Copy link
Contributor

sander2 commented Feb 18, 2021

Hi @ascjones, @dvdplm, I just tested #227 and I can confirm that it indeed fixes the problem described in this issue. Many thanks for making the fix!

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.

4 participants