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

Allow online client to initialise at a particular block height #793

Closed
kylezs opened this issue Jan 18, 2023 · 6 comments · Fixed by #794
Closed

Allow online client to initialise at a particular block height #793

kylezs opened this issue Jan 18, 2023 · 6 comments · Fixed by #794

Comments

@kylezs
Copy link
Contributor

kylezs commented Jan 18, 2023

It's not necessarily always the case that we only want to query things from the point after the latest runtime upgrade.

One use-case would be to scan all blocks from a particular block height until the latest. Let's take events in particular.

Currently the way to do this would be to use something like: #727 and pass in the metadata to the events query. In order to use this to scan from a particular block, you would probably wrap the OnlineClient and store the metadata alongside it, updating it when you hit a runtime upgrade boundary.

However, this design is a little clunkier than it needs to be. For one, we're already storing metadata in the OnlineClient. Metadata we aren't using because we're behind in block number. And in the case our scanning reaches the latest runtime, we are now duplicating storage of the metadata.

Instead of doing this, simply allowing the initiation of the online client from a particular provided block height, means we can use apply_upgrade on the ClientRuntimeUpdater to bump the runtime version only in one place (and no need to introduce external state in the wrapper). It would also be necessary to introduce some constructor for Update, new_from_raw() as the name perhaps, to allude to the fact it's a little less safe than using the subscription util, which will always pair the runtime_version and metadata for you. See #789 for the beginning of that discussion.

Code changes would be pretty simple. Just another method that can construct an OnlineClient that would wrap from_rpc_client. Orrrr could just have the one method, that takes an Option<BlockHash> (open to ideas here).

I'd be happy to do this PR, once there's buy in.

@jsdw
Copy link
Collaborator

jsdw commented Jan 18, 2023

I ultimately agree that we should at least make it possible to initialise an older client to help with decoding older blocks etc, though we also need to be careful to make clear the potential gotchas:

  • We still won't be able to decode blocks prior to V14 metadata, so there is a hard limit on how far back Subxt can look at the moment (and likely for the forseeable future)
  • If you initialise a client at an older block, the subscription related functions (which rely on current metadata) won't work properly. We may be able to work around this by adding support for the idea of loading multiple metadata/runtimeversion pairs alongside a block range, so that when asking for blocks, Subxt can use the appropriate metadata to decode.
  • You'll need to be running an archive node to be able to access older blocks (this will be even more true when the new chainHead style RPC API is more widely supported and becomes the one we use).

I opened #794 as a suggestion for the APIs that we could add here (with appropriate warnings on each of them); lemme know what you think!

@jsdw
Copy link
Collaborator

jsdw commented Jan 18, 2023

Instead of doing this, simply allowing the initiation of the online client from a particular provided block height, means we can use apply_upgrade on the ClientRuntimeUpdater to bump the runtime version only in one place (and no need to introduce external state in the wrapper). It would also be necessary to introduce some constructor for Update, new_from_raw() as the name perhaps, to allude to the fact it's a little less safe than using the subscription util, which will always pair the runtime_version and metadata for you. See #789 for the beginning of that discussion.

I'm not sure what you really mean here, but I'd be curious to see what you think to my PR suggestion :)

The way I see it is if you can instantiate an OnlineClient at a specific block hash, there is no point in fiddling with the runtime updater stuff (and if you want to instantiate an "uptodate" client again you can just create a new one to work with the chain head and use the updater to keep that uptodate.

@kylezs
Copy link
Contributor Author

kylezs commented Jan 18, 2023

we also need to be careful to make clear the potential gotchas

Agreed, thanks for writing those up.

and if you want to instantiate an "uptodate" client again you can just create a new one to work with

Yeah so this is what I want to avoid. Currently the only way to get an Update object is to pull it from the runtime-update stream. As you pointed out, this would only be a stream of updates from the latest block at the time you subscribed, regardless of whether you initiated the client at an earlier block number, so it's not useful when scanning from older blocks.

What makes it nicer to allow for updating in-place rather than creating a new client, is that you don't need any of the client initialisers again, and you don't need to track what block number/hash you're on. By forcing the creation of a new OnlineClient, if you don't want to rescan again from the earlier block, you'd have to save it (as well as ensuring you have the url at that level of the code too, which might not be true if you've passed the OnlineClient down some levels), and then pass that into the constructor of the new client, which means you need a wrapper again, which is what we were trying to avoid in the first place.

@kylezs
Copy link
Contributor Author

kylezs commented Jan 18, 2023

Related to the above, creating a new OnlineClient means you also have to replace it everywhere it's cloned too, like if a clone has been moved into some stream or future, so you'd have to terminate all those, and start them all again, or use a Mutex... which we already have at the lower level, so we'd be duplicating the Mutex to work around it

@kylezs
Copy link
Contributor Author

kylezs commented Jan 18, 2023

Not sure if github buggy or you deleted your comment (because I can no longer see it):

I see, so your goal is to have a single OnlineClient, and to be able to update that with newer metadata/runtime info manually as you use it to scan through blocks?
I think that the Updater is the wrong approach in geenral for this, as it's purpose is to subscribe to runtime updates (at chain head) and go from there. I think it'd make sense just to allow you to set the metadata/runtime version of the OnlineClient (albeit with warnings). Since I have added the functionality to instantiate the thing with arbitrary metadata/runtime info I don't mind exposing the set functionality too I think. Though this API is all one which I would generally want to discourage people from using.

I agree with this approach, just thought I was more likely to get buy in by using the updater approach haha. Updating directly with a set that then writes through the RwLock would be ideal 👌

@jsdw
Copy link
Collaborator

jsdw commented Jan 18, 2023

Oh whoops! Yup so I pushed an update to my PR to expose set_ functions for those things.

As a tradeoff though, I've removed most of the _at functions I added in the first cut (I don't want to expose too many methods that come with warnings and are generally not on the "happy path" of Subxt). So you can now construct an OnlineClient via OnlineClient::from_rpc_client_with (and I have exposed a default_rpc_client() function you can use, but otherwise you can fetch the metadata etc you'd like to init with, so it's a little more verbose but I like this because I want to naturally discourage people a little from it). You can then update the metadata etc via set_* functions directly on the OnlineClient. And the updater stuff remains purely "happy path" logic.

@jsdw jsdw closed this as completed in #794 Jan 19, 2023
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.

2 participants