`try-runtime`::`follow-chain` - keep connection #12167

pmikolajczyk41 · 2022-09-02T10:19:03Z

Problem statement

Currently, in try-runtime::follow-chain for each block we have to reconnect to the node:

$ RUST_LOG=runtime=trace,try-runtime::cli=trace,executor=trace ./try-runtime try-runtime --chain /tmp/chainspec.json follow-chain --uri ws://localhost:9944 > follow.log 2> follow.log

$ grep -E "(established|new block)" follow.log
main jsonrpsee_client_transport::ws:     Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main jsonrpsee_client_transport::ws:     Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0x13f88597de6f70629b5700fbffc723a053f4bb0646840017ea06245f2ac0af5e => 1784, extrinsics: 1
main jsonrpsee_client_transport::ws:     Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main jsonrpsee_client_transport::ws:     Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main jsonrpsee_client_transport::ws:     Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0x0c8e30112707424a1ab65996d9f3b153cd41a4c86e781ba4079bcddfcc1bec4d => 1785, extrinsics: 1
main jsonrpsee_client_transport::ws: 	 Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0x647395cdd8d077333bbfc99db3928fedd3cfcfb0a34f208d156705ff18e3390e => 1786, extrinsics: 1
main jsonrpsee_client_transport::ws: 	 Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0xfd6809f8bc01356a5a4acc7c2cdf9bbcb9bbdd3991be5120394224202ad01775 => 1787, extrinsics: 1
main jsonrpsee_client_transport::ws: 	 Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0x529b7ace4183395b099383abba4515f9e163491fc8fe20debc2927c55444fb80 => 1788, extrinsics: 1
main jsonrpsee_client_transport::ws: 	 Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0x040ccf4b4ffdac6f5370ea3a16f23d0d21c9acc821676ca31b5fa19c1b538e8d => 1789, extrinsics: 1
main jsonrpsee_client_transport::ws: 	 Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0x3bbd8e509780f6b737fec25c806f3f6c0369ba212bf5d2fb854cf0f6612d01f0 => 1790, extrinsics: 1
main jsonrpsee_client_transport::ws: 	 Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0x4943676e77ab66c2d75249918eca3e50c099cb086ba06efa6432851d4f3c65d0 => 1791, extrinsics: 1
main jsonrpsee_client_transport::ws: 	 Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0x6fc8db3403f1f4d128cc39c8b13868e5871eb8b97f585ee708a67194f5f0a1d3 => 1792, extrinsics: 1
main jsonrpsee_client_transport::ws: 	 Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0x4856d7ab44a4df31a24dd9021cc8f36a1fdb57ce50bbc3400bf89ada015d702a => 1793, extrinsics: 1
main jsonrpsee_client_transport::ws: 	 Connection established to target: Target { sockaddrs: [], host: "localhost", host_header: "localhost:9944", _mode: Plain, path_and_query: "/" }
main try-runtime::cli: new block event:  0xdf21d18817a127311b972b02e8321d60cc4105774d9fbec81c23923196a275e3 => 1794, extrinsics: 1

When the block-time is long, it makes sense obviously. However, for a short period (like we have in Aleph and possibly in Polkadot) it would probably be better to keep a single connection for the whole interaction.

Approach

We introduce RpcService which wraps RPC requests provided by remote_externalities crate. It provides the same functionality, but in addition it can reuse the same client (connection) for every request. Additionally, we add a new flag to try-runtime::follow-chain: keep_connection with which we can specify whether the connection should be reused for fetching blocks.

When substituting the new API, we preserved old behavior in all places (i.e. reconnecting).

Notes

One could argue that because of #8988, we shouldn't refactor remote_externalities::rpc_api module, but rather make some steps into reusing RPC utilities from other crate.

However, this would go too far beyond the scope of the main task (reusing connection in follow-chain). Furthermore, we argue that the refactor made in this PR should make future consolidation easier.

This resolves one of the ideas raised in: #12089

cc: @kianenigma

polkadot companion: paritytech/polkadot#5968

Polkadot address: 15fdtPm7jNN9VZk5LokKcmBVfmYZkCXdjPpaPVL2w7WgCgRY

pmikolajczyk41 · 2022-09-02T11:20:21Z

it seems that this PR requires some companion PR, but I will need some assistance with it :|

dmitry-markin · 2022-09-03T15:36:13Z

it seems that this PR requires some companion PR, but I will need some assistance with it :|

Basically, this PR breaks the builds of https://github.com/paritytech/polkadot/ and https://github.com/paritytech/cumulus/, so the two must be updated to match the changes. Once the corresponding changes are made, the companion PRs must be indicated in the PR description as stated here: https://github.com/paritytech/substrate/blob/master/docs/CONTRIBUTING.adoc#updating-polkadot-as-well.

Chances are, updating Polkadot will be enough to fix Cumulus build. It's not mentioned anywhere, but Cumulus companion must be indicated in the Polkadot PR too (if Cumulus changes are required).

dmitry-markin · 2022-09-03T16:00:42Z

Looks good to me.

niklasad1

All code looks good and makes sense to me but we already got something similar maybe it's not that obvious but you could actually pass in your own rpc client already in the Builder::OnlineConfig example here. Maybe I am missing something why the RpcService is needed?

Thus, then we don't need this new RpcService and the rest of the RPC methods that are manually implemented could just be moved the RpcApi trait but then you would need to import that trait in try_runtime cli (it's implemented be RPC client by the jsonrpsee macro)

A follow-up to this PR, would be great if we could use the existing rpc API from sc-rpc-api instead of duplicating these rpc methods all over the place but it may cause some extra deps see #12030

The RpcService that you wrote is a bit cleaner but maybe we could merge RpcService and Builder::Onlline or something?

pmikolajczyk41 · 2022-09-05T07:37:05Z

@niklasad1 From what I can see, now, there are a couple of things to consider / fix:

RPC calls are defined / implemented in at least 3 places: sc-rpc*, remote_externalities::Builder and remote_externalities::rpc_api.
The RPC-functionality present in remote_externalities::Builder is very limited, i.e. it serves essentially for scraping the state. Also, it is for internal use only.
The original motivation for keeping the connection comes from a need of continuous block fetching in follow-chain, which happens after and in separation from building the externalities.
remote_externalities::Builder implements (part of) the RPC logic, which is completely irrelevant and violates single-responsibility-principle.

I would propose three-step solution:

[preliminaries] This PR, which introduces the notion of keeping connection alive. We have RpcService which wraps WsClient and is already used outside remote_externalities.
[merging remote_externalities::Builder and remote_externalities::rpc_api] We remove all of the RPC implementation from remote_externalities::Builder into remote_externalities::rpc_api - this can be done with e.g. keeping RpcService in OnlineConfig instead of Transport. All the calls used for scraping are delegated to RpcService. What is more, it can keep the connection while paging through the state. Obviously, it requires extending the interface of RpcService by the missing methods.
[merging remote_externalities::rpc_api and sc-rpc*] We move RpcService into sc-rpc* and remove duplicate code.

Wdyt?

niklasad1 · 2022-09-05T08:12:17Z

I would propose three-step solution:

[preliminaries] This PR, which introduces the notion of keeping connection alive. We have RpcService which wraps WsClient and is already used outside remote_externalities.

[merging remote_externalities::Builder and remote_externalities::rpc_api] We remove all of the RPC implementation from remote_externalities::Builder into remote_externalities::rpc_api - this can be done with e.g. keeping RpcService in OnlineConfig instead of Transport. All the calls used for scraping are delegated to RpcService. What is more, it can keep the connection while paging through the state. Obviously, it requires extending the interface of RpcService by the missing methods.

[merging remote_externalities::rpc_api and sc-rpc*] We move RpcService into sc-rpc* and remove duplicate code.

Wdyt?

Sounds good, I got a bit confused by having the RpcService/rpc_api in remote_externalities because it would fit better in some rpc utils crate/mod but alright I see not really introduced in this PR

…/keep-connection # Conflicts: # utils/frame/try-runtime/cli/src/commands/follow_chain.rs

utils/frame/remote-externalities/src/rpc_api.rs

pmikolajczyk41 · 2022-09-05T15:28:41Z

the cumulus build fails from the error that doesn't seem to be connected in any way...

dmitry-markin · 2022-09-05T15:46:30Z

the cumulus build fails from the error that doesn't seem to be connected in any way...

That's because of the recent changes merged in Substrate/Cumulus. Merging master into your branch should solve the problem.

…/keep-connection

niklasad1 · 2022-09-06T08:01:27Z

bot merge

kianenigma · 2022-09-06T11:14:10Z

@pmikolajczyk41 polkadot address?

pmikolajczyk41 · 2022-09-06T11:17:58Z

@kianenigma added, thank you!

kianenigma · 2022-09-06T12:22:34Z

/tip medium

substrate-tip-bot · 2022-09-06T12:22:39Z

@kianenigma A medium tip was successfully submitted for pmikolajczyk41 (15fdtPm7jNN9VZk5LokKcmBVfmYZkCXdjPpaPVL2w7WgCgRY on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

* Refactor RPC module * Add flag to `follow-chain` * Multithreading remark * fmt * O_O * unused import * cmon * accidental removal reverted * remove RpcHeaderProvider * mut refs * fmt * no mutability * now? * now? * arc mutex * async mutex * async mutex * uhm * connect in constructor * remove dep * old import * another take * trigger polkadot pipeline * trigger pipeline

pmikolajczyk41 added 7 commits September 2, 2022 11:46

Refactor RPC module

9ed2a3d

Add flag to follow-chain

c2e7009

Multithreading remark

ba6c503

fmt

469f60c

O_O

9a4dbf9

unused import

d358bfe

cmon

33991b3

dmitry-markin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-breaksapi labels Sep 3, 2022

dmitry-markin requested review from kianenigma and niklasad1 September 3, 2022 15:45

dmitry-markin approved these changes Sep 3, 2022

View reviewed changes

dmitry-markin added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Sep 3, 2022

niklasad1 reviewed Sep 4, 2022

View reviewed changes

niklasad1 approved these changes Sep 5, 2022

View reviewed changes

pmikolajczyk41 added 2 commits September 5, 2022 10:34

Merge remote-tracking branch 'origin/master' into piomiko/try-runtime…

315948c

…/keep-connection # Conflicts: # utils/frame/try-runtime/cli/src/commands/follow_chain.rs

accidental removal reverted

b903d97

pmikolajczyk41 force-pushed the piomiko/try-runtime/keep-connection branch from 507d0a3 to b903d97 Compare September 5, 2022 08:58

pmikolajczyk41 added 5 commits September 5, 2022 12:10

remove RpcHeaderProvider

9a1862c

mut refs

3d0191b

fmt

6fd11da

no mutability

53607bc

now?

848003f

pmikolajczyk41 added 4 commits September 5, 2022 13:45

now?

6bcfbd7

arc mutex

0648e6f

async mutex

757eedd

async mutex

6c248df

niklasad1 reviewed Sep 5, 2022

View reviewed changes

utils/frame/remote-externalities/src/rpc_api.rs Outdated Show resolved Hide resolved

pmikolajczyk41 added 6 commits September 5, 2022 15:14

uhm

0276f84

connect in constructor

707fbbb

remove dep

96bd047

old import

c7d22f2

another take

b93e4a6

trigger polkadot pipeline

9c0ce68

pmikolajczyk41 added 2 commits September 5, 2022 17:59

Merge remote-tracking branch 'origin/master' into piomiko/try-runtime…

cdb5a89

…/keep-connection

trigger pipeline

305318c

paritytech-processbot bot merged commit 20037d5 into paritytech:master Sep 6, 2022

pmikolajczyk41 deleted the piomiko/try-runtime/keep-connection branch September 6, 2022 08:06

pmikolajczyk41 mentioned this pull request Sep 9, 2022

frame/utils: introduce substrate-rpc-client crate for RPC utils #12212

Merged

librelois mentioned this pull request Sep 15, 2022

Update substrate/polkadot/cumulus from v0.9.26 to v0.9.29 moonbeam-foundation/moonbeam#1814

Closed

2 tasks

notlesh mentioned this pull request Oct 5, 2022

v0.9.29 Dependency Update moonbeam-foundation/moonbeam#1825

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`try-runtime`::`follow-chain` - keep connection #12167

`try-runtime`::`follow-chain` - keep connection #12167

pmikolajczyk41 commented Sep 2, 2022 •

edited

Loading

pmikolajczyk41 commented Sep 2, 2022

dmitry-markin commented Sep 3, 2022 •

edited

Loading

dmitry-markin commented Sep 3, 2022

niklasad1 left a comment •

edited

Loading

pmikolajczyk41 commented Sep 5, 2022

niklasad1 commented Sep 5, 2022

pmikolajczyk41 commented Sep 5, 2022

dmitry-markin commented Sep 5, 2022 •

edited

Loading

niklasad1 commented Sep 6, 2022

kianenigma commented Sep 6, 2022

pmikolajczyk41 commented Sep 6, 2022

kianenigma commented Sep 6, 2022

substrate-tip-bot bot commented Sep 6, 2022

try-runtime::follow-chain - keep connection #12167

try-runtime::follow-chain - keep connection #12167

Conversation

pmikolajczyk41 commented Sep 2, 2022 • edited Loading

Problem statement

Approach

Notes

pmikolajczyk41 commented Sep 2, 2022

dmitry-markin commented Sep 3, 2022 • edited Loading

dmitry-markin commented Sep 3, 2022

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

pmikolajczyk41 commented Sep 5, 2022

niklasad1 commented Sep 5, 2022

pmikolajczyk41 commented Sep 5, 2022

dmitry-markin commented Sep 5, 2022 • edited Loading

niklasad1 commented Sep 6, 2022

kianenigma commented Sep 6, 2022

pmikolajczyk41 commented Sep 6, 2022

kianenigma commented Sep 6, 2022

substrate-tip-bot bot commented Sep 6, 2022

`try-runtime`::`follow-chain` - keep connection #12167

`try-runtime`::`follow-chain` - keep connection #12167

pmikolajczyk41 commented Sep 2, 2022 •

edited

Loading

dmitry-markin commented Sep 3, 2022 •

edited

Loading

niklasad1 left a comment •

edited

Loading

dmitry-markin commented Sep 5, 2022 •

edited

Loading