Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[Features] Reintroduce sub-du #12439

Closed
wants to merge 13 commits into from
Closed

[Features] Reintroduce sub-du #12439

wants to merge 13 commits into from

Conversation

ruseinov
Copy link
Contributor

@ruseinov ruseinov commented Oct 6, 2022

Resolves paritytech/polkadot-sdk#449

At the moment this is just a copy-paste with updated dependencies.

  • follow-up with devops: wrap this in a simple HTTP server application (external repo), deploy it on state.polkadot.network or something like that.

@ruseinov ruseinov requested a review from kianenigma October 6, 2022 16:07
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Oct 6, 2022
@ggwpez
Copy link
Member

ggwpez commented Oct 16, 2022

So this loops through the pallet+storage items and calculates their keys?
I wonder how many keys the first approach missed. There are special keys likes :CODE: which would not be included.
Maybe looping through the trie keys and doing a best-effort reverse lookup could find them.

I also vaguely remember a TS project which could be web-deployed, but cant find it now…

@kianenigma
Copy link
Contributor

I wonder how many keys the first approach missed. There are special keys likes :CODE: which would not be included.
Maybe looping through the trie keys and doing a best-effort reverse lookup could find them.

This only looks at the storage items that are in the metadata, so yeah any custom key is missed, and probably fine. Exposing well_known_keys over metadata seems totally reasonable. Although it won't really fix the problem.

@ruseinov
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@dvdplm
Copy link
Contributor

dvdplm commented Oct 25, 2022

  • wrap this in a simple HTTP server application (external repo), deploy it on state.polkadot.network or something like that.

Perhaps a dumb question but what is the purpose of having this on the internet, as opposed to have people run a local binary? Just as a community utility thing?

@ruseinov
Copy link
Contributor Author

Perhaps a dumb question but what is the purpose of having this on the internet, as opposed to have people run a local binary? Just as a community utility thing?

Yeah, just a remote api that would conveniently show latest storage of a given network. Possibly with some sort of dumb cache to make sure multiple requests within a short period of time are handled without issues.

@ruseinov
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@ruseinov
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@ggwpez
Copy link
Member

ggwpez commented Oct 30, 2022

Last time I tried it it would not connect because of some RPC error.
Did you try it yet @ruseinov ?

.expect("Runtime Metadata failed to decode");
let metadata = prefixed_metadata.1;

if let RuntimeMetadata::V12(inner) = metadata {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

support for metadata v13 and v14? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's the plan. I have not touched it much since reintroduction, on it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niklasad1 do we want to support both v13 and v14? I guess v12 Is probably not needed anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'll start with v14

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to start with v14, gotcha I was just surprised when I spotted v12 :)

No sure might be nice to query on really old blocks to check how it grows over time not sure how use it will be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should at first only support v14. no need to support the old versions initially. If you can do it, sure, but otherwise no need.

@ruseinov
Copy link
Contributor Author

ruseinov commented Nov 1, 2022

Last time I tried it it would not connect because of some RPC error. Did you try it yet @ruseinov ?

Not yet, getting to it now

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@ruseinov
Copy link
Contributor Author

ruseinov commented Nov 1, 2022

bot rebase

@paritytech-processbot
Copy link

Rebased

@@ -0,0 +1 @@
# sub-du
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete this file please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's best to actually have a proper README?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are keen on writing some readme please write it as module doc (//!) it will be more worthwhile and our CI will create the READM from it.

}

#[derive(Debug, Copy, Clone)]
pub enum StorageItem {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have more types now.

Copy link
Contributor

@kianenigma kianenigma Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double/N map, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it hasn't been touched yet, WIP

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's interesting, the StorageEntryType actually contains two types now Map and Plain.

@ruseinov
Copy link
Contributor Author

ruseinov commented Nov 7, 2022

@kianenigma I have removed sub-storage.

In the current implementation I am using wrappers done in remote-externalities builder to achieve faster results. Once all of this works - we can discuss having those helpers someplace else shared by the two.

But actually making a few methods public might not hurt remote-externalities much.

This is still a WIP, any suggestions welcome.

@kianenigma kianenigma requested a review from niklasad1 November 7, 2022 15:55
@kianenigma
Copy link
Contributor

kianenigma commented Nov 9, 2022

In the current implementation I am using wrappers done in remote-externalities builder to achieve faster results.

This is not really correct. All you need is:

1. an RPC call to get state_getMetadata
2. an RPC call to get state_storageSize (or similar)

Both of which are provided neatly by substrate RPC utilities.

I should first review the code, which I will do asap.


println!("Scraping at block {:?} of {}({})", at, runtime.spec_name, runtime.spec_version,);

let raw_metadata = rpc_client.metadata(at).await.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this fn metadata coming from? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, then using the RPC stuff directly is almost an identical code, you just use this client:

pub async fn ws_client(uri: impl AsRef<str>) -> Result<WsClient, String> {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also consider parallelizing this like my #12537

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, then using the RPC stuff directly is almost an identical code, you just use this client:

pub async fn ws_client(uri: impl AsRef<str>) -> Result<WsClient, String> {

Makes sense, indeed remote_externalities use seems a bit hacky, especially if it does not yield all that many benefits.

cfg.transport.map_uri().await.unwrap();

// connect to a node.
let ext = remote_externalities::Builder::<Block>::new().mode(Mode::Online(cfg));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I agree with Kian that we shouldn't use remote_externalities here instead use

let rpc_client = substrate_rpc_client::ws_client(opt.uri).await.unwrap()

Then we don't need to have this sp_testing::Block which isn't really nice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the sp_testing::Block did not feel like an amazing approach. It's also kinda cryptic.

let mut modules: Vec<Pallet> = vec![];

// potentially replace head with the given hash
let head = ext.rpc_get_head().await.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, you need to add some ugly stuff here:

let head = ChainApi::<(), _, Header, ()>::finalized_head(&rpc_client).await.unwrap();

@kianenigma
Copy link
Contributor

@niklasad1 it might not be bad idea if you entirely take this over, as Roman is busy with other stuff (that's way more high priority).

@stale
Copy link

stale bot commented Jan 11, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jan 11, 2023
@ruseinov
Copy link
Contributor Author

@niklasad1 it might not be bad idea if you entirely take this over, as Roman is busy with other stuff (that's way more high priority).

Yeah, not possible for me to proceed with this atm. But I'll get back to this if it's not done by the time I'm finished with the other stuff.

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jan 11, 2023
@ruseinov ruseinov added the Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. label Jan 28, 2023
@kianenigma kianenigma closed this Feb 14, 2023
@kianenigma
Copy link
Contributor

@harrysolovay would the idea behind this be a good tool to build with CAPI?

@harrysolovay
Copy link

This is absolutely something that could be implemented as a Capi pattern lib. Let's try it out & follow up here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
Status: 👀 Might Revisit 👀
Development

Successfully merging this pull request may close these issues.

revive sub-du in utils/frame
6 participants