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

staking miner: reuse ws conn for remote-ext #4849

Merged

paritytech-processbot merged 30 commits into master from na-staking-miner-reuse-ws-conn

Feb 18, 2022

Member

niklasad1 commented Feb 4, 2022 •

edited

Loading

Builds on top of #4716

It passes the already established WebSocket connection down to remote externalities to re-use the already established connection for that part.

In addition this PR introduces a SharedRpcClient abstraction with a RPC API with macros to make things a little bit cleaner and easier to understand I hope.

niklasad1 added 15 commits

January 11, 2022 14:51


          staking miner: use config for emergency solution

feb0a7b

Fixes #4678


          bump jsonrpsee

633ee73


          Merge remote-tracking branch 'origin/master' into na-staking-miner-pl…

4c2dbd6

…ayground


          run monitor_cmd_for until the connection is closed

4d9644e


          new tokio task for submit_and_watch xt

21546b1


          re-use header subscription

00ff014


          Merge remote-tracking branch 'origin/master' into HEAD

54f3bf1


          update jsonrpsee + simplify code

ecfe004


          revert polkadot runtime changes

5fddbbe


          fix grumbles

8ea11a3


          Update utils/staking-miner/src/monitor.rs

f4a3b74


          Merge remote-tracking branch 'origin/master' into na-staking-miner-pl…

6ad6106

…ayground


          Merge remote-tracking branch 'origin/master' into na-staking-miner-pl…

4ed8e80

…ayground


          staking miner: reuse ws conn for remote-ext

ee66153


          Revert "revert polkadot runtime changes"

2c47dad

This reverts commit 5fddbbe.

github-actions bot added the A0-please_review label

niklasad1 added B0-silent C1-low labels


          cargo fmt

2d1cf42

niklasad1 requested review from emostov and kianenigma

February 4, 2022 14:07

niklasad1 added 3 commits

February 4, 2022 18:01


          revert unintentional change

a5a5cf8


          revert unintentional change

f600be8


          Merge remote-tracking branch 'origin/na-staking-miner-reuse-ws-conn' …

c57ee57

…into na-staking-miner-reuse-ws-conn

Contributor

emostov commented Feb 4, 2022 •

edited

Loading

From what I understand the changes lgtm. I looked at the diff via https://github.com/paritytech/polkadot/compare/na-staking-miner-playground..na-staking-miner-reuse-ws-conn#

To make sure I understand please let me know if the following is correct:

&*client => deref's to the inner value and then we get a simple pointer it. This is used when the compiler knows client won't get dropped before the function it is getting passed too executes.

client.clone() => gives us a new Arc, increasing the reference counter. We need to use this when we spawn a new thread that is not guaranteed to finish before client goes out of scope in the current function or we need to create some struct (like Ext) that will need to have a thread safe ref to the client

emostov approved these changes

View reviewed changes

Member Author

niklasad1 commented Feb 4, 2022 •

edited

Loading

You got everything correct Zeke again :)

Yeah, we could wrap Arc<WsClient> in a new type and impl Deref trait to get rid of that horrible syntax

niklasad1 added 3 commits

February 7, 2022 15:18


          Merge remote-tracking branch 'origin/master' into na-staking-miner-re…

a54173a

…use-ws-conn


          add type SharedRpcClient

1b44965


          cargo fmt

d8a551d

niklasad1 added 5 commits

February 7, 2022 16:16


          fix nits

24b738a


          spelling

25a2781


          use tracing_subcriber and cleanup

761d538


          remove some needless clones

1e02980


          fix some nits

niklasad1 commented

View reviewed changes

utils/staking-miner/Cargo.toml Show resolved Hide resolved

kianenigma reviewed

View reviewed changes

utils/staking-miner/src/rpc.rs Outdated Show resolved Hide resolved

kianenigma reviewed

View reviewed changes

utils/staking-miner/src/rpc.rs Outdated Show resolved Hide resolved

kianenigma reviewed

View reviewed changes

utils/staking-miner/src/rpc.rs Outdated Show resolved Hide resolved

kianenigma self-requested a review

February 15, 2022 09:54

kianenigma reviewed

View reviewed changes

utils/staking-miner/src/rpc.rs Outdated

+              	/// Dry run an extrinsic at a given block. Return SCALE encoded ApplyExtrinsicResult.
+              	#[method(name = "system_dryRun", aliases = ["system_dryRunAt"])]
+              	async fn dry_run(&self, extrinsic: &Bytes, at: Option<&Hash>) -> RpcResult<Bytes>;

Contributor

kianenigma Feb 15, 2022

I think Hash is Copy and making it a ref doesn't make sense

Member Author

niklasad1 Feb 15, 2022

fair enough, it was more to have a uniform API since all other methods takes that params by ref but sure I can change that.

kianenigma reviewed

View reviewed changes

utils/staking-miner/src/rpc.rs

+              #[derive(Clone, Debug)]
+              pub(crate) struct SharedRpcClient(Arc<WsClient>);
+              impl Deref for SharedRpcClient {

Contributor

kianenigma Feb 15, 2022

noice

kianenigma reviewed

View reviewed changes

utils/staking-miner/src/rpc.rs Outdated Show resolved Hide resolved

kianenigma reviewed

View reviewed changes

utils/staking-miner/src/rpc.rs Outdated

+              		Ok(Self(Arc::new(client)))
+              	}
+              	/// Get the storage item.

Contributor

kianenigma Feb 15, 2022

I would expect this doc to explain how this is different than self.storage. In the same spirit, I would name it decode_storage or get_storage_decoded.

kianenigma reviewed

View reviewed changes

utils/staking-miner/src/rpc.rs Outdated Show resolved Hide resolved

kianenigma approved these changes

View reviewed changes

Contributor

kianenigma left a comment

very nice refactors, appreciated!

niklasad1 added 3 commits

February 15, 2022 17:10


          address grumbles

fb2bca3


          Merge remote-tracking branch 'origin/master' into na-staking-miner-re…

0d9d95f

…use-ws-conn


          fix spellcheck

Member Author

niklasad1 commented Feb 18, 2022

bot merge

paritytech-processbot bot merged commit 75dd6c7 into master

paritytech-processbot bot deleted the na-staking-miner-reuse-ws-conn branch

February 18, 2022 11:41

ordian added a commit that referenced this pull request


          Merge branch 'master' into ao-approval-distribution-fix

b611ab8

* master: (27 commits)
  Bump `tokio` to 1.17.0 (#4965)
  bump transaction_version (#4956)
  Bump tracing-subscriber from 0.3.8 to 0.3.9 (#4954)
  staking miner: reuse ws conn for remote-ext (#4849)
  Revert "collator-protocol: short-term fixes for connectivity (#4640)" (#4914)
  Bump tracing from 0.1.30 to 0.1.31 (#4941)
  Better spam slots handling (#4845)
  Bump proc-macro-crate from 1.1.0 to 1.1.2 (#4936)
  corrected paras code validation event comments (#4932)
  Companion for refactor election score #10834 (#4927)
  Companion CI: Make sure to pass the update crates properly (#4928)
  update digest to v0.10.2 (#4907)
  Companion for #10832 (#4918)
  Companion for `Remove u32_trait` (#4920)
  Bump serde_json from 1.0.78 to 1.0.79 (#4916)
  Bump rand from 0.8.4 to 0.8.5 (#4917)
  Remove stale migrations post 9.16 release (#4848)
  Add proxy type for Kappa Sigma Mu (#4851)
  Baseline weights for `force_apply_min_commission` (#4896)
  Allow two Parachains to swap (#4772)
  ...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review B0-silent C1-low