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

Add archive RPCs to subxt-rpcs #1940

Merged
merged 5 commits into from
Mar 5, 2025
Merged

Add archive RPCs to subxt-rpcs #1940

merged 5 commits into from
Mar 5, 2025

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Mar 3, 2025

  • Add support for archive* V2 RPCs in subxt-rpcs crate.
  • Add basic tests for these.

TODO

  • Check/add archive_storage unsubscribe function to spec so we know what to use

// we can't have a boolean tag to denote variants.
#[derive(Deserialize)]
struct Response {
success: bool,
Copy link
Member

Choose a reason for hiding this comment

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

This success flag is annoying, why do we need it at all?

result/value or error should be sufficient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah true; I just added it as an extra check to catch any weird responses eg success:false but a value!

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't success part of the spec? whats the issue here

Copy link
Contributor

@ryanleecode ryanleecode Mar 4, 2025

Choose a reason for hiding this comment

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

the flag is useful for pattern matching because you can have a union that looks like this

S.Union(
  S.Struct({
    success: S.Literal(true),
    result: HexString,
  }),
  S.Struct({
    success: S.Literal(false),
    error: S.String,
  }),
  S.Null
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep this is quite annoying in rust, IIRC we only left the success flag because it might be easier for js/ts realm interpret the data easier

Copy link
Member

Choose a reason for hiding this comment

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

aight, I see

@jsdw jsdw marked this pull request as ready for review March 4, 2025 17:08
@jsdw jsdw requested a review from a team as a code owner March 4, 2025 17:08
.subscribe(
"archive_unstable_storage",
rpc_params![block_hash, items, child_key.map(to_hex)],
"archive_unstable_storage_unsub", // TODO unsub method undefined in spec: look it up/add to spec
Copy link
Contributor

Choose a reason for hiding this comment

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

how can archive method have subscription? isn't that a chainhead only thing? B/c ur looking in the past right, so how can it change? whereas chainhead is constantly moving forward so it makes sense to have a subscription?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have discussed quite a bit about this in the spec, I ll try to summarize as best as I can remember: Our general approach is that an external party can craft a request that would take the server a considerable time to produce the response. To avoid such cases (DoS), we decided that subscriptions are more appropriate because they can enforce backpressure in an easy way that does not break higher-level tools (like subxt or jsonrpsee). It might be possible to add backpressure by sending chunks of the method response, but then you'd need to implement custom serializer / deserializer on top of the existing tools, and the amount of change that would require leaned towards us using subscriptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

So its basically a subscription that will only ever emit 1 value right? Interesting!

Copy link
Member

@niklasad1 niklasad1 Mar 5, 2025

Choose a reason for hiding this comment

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

Yes, technically, the subscription could emit only one value, but that’s not the intended behavior. The implementation ensures backpressure by sending one "storage-lookup/item" at a time (the substrate impls does this)

Let’s take a step back: this API allows querying multiple storage items in a single request, which could generate a massive response and become a DoS risk. To mitigate this, the system enforces backpressure—ensuring that a client cannot send a few calls that are computationally expensive for the server all at once.

Without a subscription, achieving proper backpressure would require a custom implementation that process the items as long as the client can keep up in a "streaming way" producing a single JSON-RPC method response. However, general JSON-RPC libraries like jsonrpsee don’t natively support such behavior and tricky to implement especially for websocket frames.

So, while it's possible for a subscription to emit just one value, the intended behavior is to send items one by one via subscription, maintaining backpressure and preventing overwhelming responses.

@jsdw jsdw merged commit 23c0651 into master Mar 5, 2025
13 checks passed
@jsdw jsdw deleted the jsdw-archive-rpcs branch March 5, 2025 15:23
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 this pull request may close these issues.

4 participants