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

archive: Implement archive_unstable_storage #1846

Merged
merged 27 commits into from
Jan 15, 2024
Merged

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Oct 11, 2023

This PR implements the archive_unstable_storage method that offers support for:

  • fetching values
  • fetching hashes
  • iterating over keys and values
  • iterating over keys and hashes
  • fetching merkle values from the trie-db

A common component dedicated to RPC-V2 storage queries is created to bridge the gap between chainHead/storage and archive/storage.
Query pagination is supported by paginationStartKey, similar to the old APIs.
Similarly to the chainHead/storage, the archive/storage method accepts a maximum number of queried items.

The design builds upon: paritytech/json-rpc-interface-spec#94.
Closes #1512.

cc @paritytech/subxt-team

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv added A0-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix I5-enhancement An additional feature request. T3-RPC_API This PR/Issue is related to RPC APIs. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. labels Oct 11, 2023
@lexnv lexnv self-assigned this Oct 11, 2023
@lexnv lexnv requested review from skunert and jsdw October 11, 2023 10:43
substrate/client/rpc-spec-v2/src/archive/archive.rs Outdated Show resolved Hide resolved

let mut storage_results = Vec::with_capacity(items.len());
for item in items {
if !is_key_queryable(&item.key.0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not exactly sure about the semantics we want to have here. If the key is not queryable should it be counted as discarded item?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed this is a bit left to interpretation, I had to revisit the spec to get a better idea of it:

discardedItems is an integer indicating the number of items at the back of the array of the items parameters that couldn't be processed.

I'll add a small PR to clarify this behavior, both in chainHead and archive

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've removed the concept of non-querieable keys, as per comment: paritytech/json-rpc-interface-spec#118 (comment)

}
}

let discarded_items = items.len().saturating_sub(self.storage_max_queried_items);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is self.storage_max_queried_items part of the spec? What is the value we use in production for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can understand the spec, there's no explicit mention of an upper limit before the pagination kicks in. This is by design, since smoldot / other implementation might be running under hardware constraints.

For chainHead_storage we are using 5 items, but we haven't collect yet any data to make a better informed decision:

/// The maximum number of items the `chainHead_storage` can return
/// before paginations is required.
const MAX_STORAGE_ITER_ITEMS: usize = 5;

The archive class is not exposed by substrate yet, but would keep both classes in sync

Copy link
Member

Choose a reason for hiding this comment

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

It would be to good to add a prometheus metric for that or some annoying log for us to know whether this limit is exceeded often or not ^^

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv added the R0-silent Changes should not be mentioned in any release notes label Dec 5, 2023
@lexnv
Copy link
Contributor Author

lexnv commented Dec 5, 2023

We have merged: paritytech/json-rpc-interface-spec#94 into the spec, which backports the chainHead_storage to archive_storage as a first-step unstable implementation.

Would love to get some reviews on this 🙏

One other thing that is in flight wrt this PR is: paritytech/json-rpc-interface-spec#118, since we have not enabled the archive methods yet we could tackle that as a follow up (together with some extra error codes)

// cc @jsdw @skunert

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Agree with the comments from niklas, otherwise looks good to me!

lexnv and others added 6 commits January 15, 2024 13:44
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice work

@lexnv lexnv added this pull request to the merge queue Jan 15, 2024
Merged via the queue into master with commit 53bcbb1 Jan 15, 2024
123 of 124 checks passed
@lexnv lexnv deleted the lexnv/archive_storage branch January 15, 2024 14:55
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
This PR implements the `archive_unstable_storage` method that offers
support for:
- fetching values
- fetching hashes
- iterating over keys and values
- iterating over keys and hashes
- fetching merkle values from the trie-db

A common component dedicated to RPC-V2 storage queries is created to
bridge the gap between `chainHead/storage` and `archive/storage`.
Query pagination is supported by `paginationStartKey`, similar to the
old APIs.
Similarly to the `chainHead/storage`, the `archive/storage` method
accepts a maximum number of queried items.

The design builds upon:
paritytech/json-rpc-interface-spec#94.
Closes paritytech#1512.

cc @paritytech/subxt-team

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. R0-silent Changes should not be mentioned in any release notes T3-RPC_API This PR/Issue is related to RPC APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RPC-Spec-V2] archive: Implement archive_unstable_storage
3 participants