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

[RPC-Spec-V2] chainHead_unstable_follow shouldn't use pipe_from_stream #3078

Open
Tracked by #1516
niklasad1 opened this issue Jan 26, 2024 · 4 comments
Open
Tracked by #1516
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. T3-RPC_API This PR/Issue is related to RPC APIs.

Comments

@niklasad1
Copy link
Member

niklasad1 commented Jan 26, 2024

After #1313 was merged, pipe_from_stream will drop the subscription completely if the client can't keep up with the server.

From my brief understanding of the spec is that certain notifications is allowed to be dropped if the client can't keep up with server but not drop the subscription entirely.

Thus, polkadot-sdk should provide a custom subscription implementation for chainHead_unstable_follow and overwrite the oldest notifications in buffer with a reasonable size (such as 16).

//cc @paritytech/subxt-team

@niklasad1 niklasad1 added 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 Jan 26, 2024
@jsdw
Copy link
Contributor

jsdw commented Jan 26, 2024

We could make a similar improvement to transactionWatch_unstable_submitAndWatch too (https://paritytech.github.io/json-rpc-interface-spec/api/transactionWatch_unstable_submitAndWatch.html)

JSON-RPC servers are allowed to skip sending events as long as it properly keeps the JSON-RPC client up to date with the state of the transaction. In other words, multiple validated, broadcasted, or bestChainBlockIncluded events in a row might be merged into one.

Note: In order to implement this properly, JSON-RPC servers should maintain a buffer of three notifications (one for each property), and overwrite any unsent notification with a more recent status update.

@niklasad1
Copy link
Member Author

niklasad1 commented Jan 26, 2024

We could make a similar improvement to transactionWatch_unstable_submitAndWatch too (https://paritytech.github.io/json-rpc-interface-spec/api/transactionWatch_unstable_submitAndWatch.html)

Yeah, I opened another issue for it #3076

@jsdw
Copy link
Contributor

jsdw commented Jan 26, 2024

Aha sorry; I saw this one first and missed that one :)

@jsdw jsdw changed the title rpc v2: chainHead_unstable_follow shouldn't use pipe_from_stream [RPC-Spec-V2] chainHead_unstable_follow shouldn't use pipe_from_stream Feb 9, 2024
@niklasad1
Copy link
Member Author

I had a look at the implementation and it doesn't use pipe_from_stream any more so it doesn't have the thingy described in this issue anymore.

Instead the implementation is processing the events sequentially which we need to fix i.e, replace older messages if it can't keep up with the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. T3-RPC_API This PR/Issue is related to RPC APIs.
Projects
None yet
Development

No branches or pull requests

2 participants