-
Notifications
You must be signed in to change notification settings - Fork 257
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
events: Fetch metadata at arbitrary blocks #727
Conversation
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
a6be41f
to
381409b
Compare
One reason I've been pretty hesitant about introducing this sort of change (and instead just warning users) is that it's quite a lot of overhead to download a big metadata for every single block requested in this way. It's also v hard to cache (and I wouldn't want to cache a ton of metadata blobs per block or something). Given the new RPC API that you're working on too, I wonder whether this sort of change would be compatible or not; I can see us eventually going the way of removing the ability to query previous block numbers at all, and providing either "archive" methods to "get anything you want" or "chainHead" things to follow and be able to work with anything at the head of the chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will mark as "request changes" because I'm quite hesitant about this being merged for the above reason (I posted on the issue also to elaborate) :)
subxt/src/events/events_client.rs
Outdated
@@ -73,7 +85,7 @@ where | |||
.map(|e| e.0) | |||
.unwrap_or_else(Vec::new); | |||
|
|||
Ok(Events::new(client.metadata(), block_hash, event_bytes)) | |||
Ok(Events::new(metadata, block_hash, event_bytes)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the solution really is that we allow the user to manually do the above and obtain metadata for the older block if they are in doubt, and ensure that they can construct this Events
object.
It might be nicer then to move the event_bytes
stuff into Events::new
and make that async or something, so that the interface is simpler (they provide metadata and block hash and get events back for it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall that comparing metadata is super expensive as well, maybe there is some simple way to find which runtime version a block belongs to without using some block explorer API. So how would I know if a block is "old" i.e. need to call metadata
? I suppose one could do some simple calculation based on that 14 400 blocks per day is produced * 30 or something.
My point is that comparing metadata is superslow as well IIRC then fetching metadata could make sense to just be naive and always fetch the metadata regardless..... so folks doesn't end up with decoding errors.
However, I agree it's probably better to have different APIs or take Option<Metadata>
as input in fn at
i.e, if None just use the latest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we should try to minimize the overhead of the metadata fetching. To achieve this, I added a new Events::new_from_client
API, the documentation should steer away the users from using this low-level API, but at the same time should provide the level of flexibility a minority of users are after.
Let me know what you think of it! Thanks!
This reverts commit 381409b.
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, looks good to me :)
Co-authored-by: James Wilson <james@jsdw.me>
The events are using the client metadata for dynamic decoding.
However, the users are allowed to inspect the events at an arbitrary block.
If the arbitrary block is a block where a runtime upgrade happened that
changed the metadata content, the dynamic decoding of the events
would fail.
To mitigate this scenario, expose the ability to query the metadata at
an arbitrary block, and fetch the metadata of a block when fetching the
block events.
This ensures that dynamic decoding of events will succeed if an arbitrary
block hash is provided.
The client is encouraged to keep the metadata in sync via subscriptions
to the runtime upgrade.
Closes: #725