RPC: Implements eth_subscribe("syncing") by seunlanlege · Pull Request #10311 · openethereum/parity-ethereum

seunlanlege · 2019-02-07T13:58:05Z

closes #10080

parity-cla-bot · 2019-02-07T13:58:09Z

It looks like @seunlanlege signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

parity-cla-bot · 2019-02-07T13:58:09Z

It looks like @seunlanlege signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

axelchalon · 2019-02-07T16:29:23Z

oops, looks like we've been working on the same thing #10312
thought I had communicated this with the assignment on the issue #10080 sorry

tomusdrw

Initial version looks good, a bunch of style grumbles though, but I think this is on a good track.

Would be good to have an RPC test for this as well.

seunlanlege · 2019-02-18T11:51:38Z

@tomusdrw please could you take a look at this again?

tomusdrw

Can we remove the is_major_importing function from block_import helpers?

Also what's the status of #10312 Can you coordinate with Axel about it? I like the fact that this PR uses subscription to actual syncing state instead of blocks, I guess if we want to provide sync details in the subscription events then combining the two approaches would be required.

tomusdrw · 2019-02-18T12:14:59Z

@@ -699,6 +725,10 @@ pub trait SyncInfo {

 	/// Whether major sync is underway.
 	fn is_major_importing(&self) -> bool;


IS this still used?

yes for light clients

Can't light clients use the same mechanism instead? it seems that it triggers notifications as well, no?

axelchalon · 2019-02-18T15:18:22Z

The eth_subscribe(syncing) pubsub isn't really "required" for Fether; currently we're polling the RPC eth_syncing and it does the job. We use the currentBlock, highestBlock, startingBlock returned by the RPC to display the syncing percentage to the user.

I imagine if we want to switch to using the pubsub eth_subscribe(syncing) in Fether, then the pubsub should also return those values (also for the sake of consistency with the RPC eth_syncing).

However I'm not sure if we can switch to the eth_subscribe(syncing) in Fether because (at least on my PR, haven't tested on this one) the pubsub can emit values very many times / very fast; the granularity is a bit too small and I think this could bottleneck the receiving end

tomusdrw · 2019-02-18T15:27:16Z

@axelchalon indeed chain_new_blocks can be triggered quite often during initial sync, we should figure out how to throttle it.
Perhaps we could then have true/false flags in the pubsub subscriptions and let the client poll as often as need to update the status?

axelchalon · 2019-02-18T15:48:27Z

Having eth_subscribe("syncing") return true/false depending on is_major_importing sounds OK to me!

seunlanlege · 2019-02-18T22:53:30Z

However I'm not sure if we can switch to the eth_subscribe(syncing) in Fether because (at least on my PR, haven't tested on this one) the pubsub can emit values very many times / very fast; the granularity is a bit too small and I think this could bottleneck the receiving end

actually in this PR, events are emitted only when SyncState changes, which doesn't occur very frequently during syncing, infact i think you'd still need to poll eth_syncing to consistently show sync progress.

seunlanlege · 2019-02-19T11:48:26Z

@tomusdrw could you look at this again?

tomusdrw

Few more grumbles regarding the sync subscription. We should really think how to make it lightweight. The whole point of:

Having notifications
Storing an AtomicBool for sync status for easy access

was to prevent the need from calling into sync/client status - cause that heavy and requires locks. If we still do that, then it's pretty wasteful. All the info that is required for the subscription should go with the event or should be accessed from lock-free structures.

Also if there is any additional data that we pass with the notification we need to be subscribed to as well. I.e. If we pass starting/current/highest block then people will expect that they are going to be notified about changes of those. This PR doesn't achieve that so it's a bit buggy behaviour. I'd be in favor of providing less data, but in a very reliable way.

tomusdrw · 2019-02-20T09:41:16Z

 	/// Returns propagation count for pending transactions.
 	fn transactions_stats(&self) -> BTreeMap<H256, TransactionStats>;
+
+	/// gets


missing docs

tomusdrw · 2019-02-20T09:44:00Z

@@ -699,6 +725,10 @@ pub trait SyncInfo {

 	/// Whether major sync is underway.
 	fn is_major_importing(&self) -> bool;


Can't light clients use the same mechanism instead? it seems that it triggers notifications as well, no?

tomusdrw · 2019-02-20T09:49:00Z

 			Result::Header(ref header) => header.serialize(serializer),
 			Result::Log(ref log) => log.serialize(serializer),
 			Result::TransactionHash(ref hash) => hash.serialize(serializer),
+			Result::SyncState(ref sync) => sync.serialize(serializer)


missing comma

tomusdrw · 2019-02-20T09:49:06Z

+#[serde(rename_all="camelCase")]
+pub struct PubSubSyncStatus {
+	/// starting block number
+	pub	starting_block: U64,


funky ident

tomusdrw · 2019-02-20T09:51:22Z

+		C: 'static + LightSyncProvider + LightNetworkDispatcher + ManageNetwork
+{
+	/// adds a sync notification channel to the pubsub client
+	pub fn add_light_sync_notifier(&mut self, receiver: Notification<SyncState>, sync: Arc<LightSyncInfo>) {


I thought that we extract QueueInfo trait to unify the add_light_sync_notifier and add_sync_notifier, if we don't do that, then the other changes are just unnecessary.
Can we unify this completely the logic seems to be exactly the same?

If there is something that I'm missing we can also pass a closure that produces PubSubSyncStatus and then the mode-specific logic can be there.

tomusdrw · 2019-02-20T09:56:10Z

+				let sync_state = pubsub::PubSubSyncStatus {
+					is_syncing: is_verifying || is_syncing_state,
+					starting_block: status.start_block_number.into(),
+					current_block: client.chain_info().best_block_number.into(),


So this is being triggered only when sync changes status right? Then IMHO returning this additional data here doesn't make sense, cause it's just misleading for users - that stuff won't change at all, since we only notify about sync/notsync.

Either we need to trigger this notification more often (after every imported block for instance), but we discussed with @axelchalon that it might be overwhelming for subscribers.

I'd go for true/false returned from subscription and then the clients can poll at their discretion for the proper sync status. Note that we also have subscription for newHeads so the two subscriptions combined give you pretty good understanding what happens with the node.

Calling queue_info an chain_info from client seems pretty heavy, I'd prefer to not do it at all.
Regarding sync.status() - why can't we just pass all the details in the state already? It's just redundant call that requires some locks to be acquired while we can provide all necessary data with every event.

soc1c · 2019-03-31T08:40:05Z

Please rebase

* master: fix(light cull): poll light cull instead of timer (#10559) Update Issue Template to direct security issue to email (#10562) RPC: Implements eth_subscribe("syncing") (#10311) Explicitly enable or disable Stratum in config file (Issue 9785) (#10521) version: bump master to 2.6 (#10560) tx-pool: check transaction readiness before replacing (#10526) fix(light account response): update `tx_queue` (#10545) Update light client harcoded headers (#10547) fix(light eth_gasPrice): ask network if not in cache (#10535) Implement caching for service transactions checker (#10088) build android with cache, win fixes (#10546) clique: make state backfill time measurement more accurate (#10551) updated lru-cache to 0.1.2 (#10542)

aawaken · 2019-05-02T16:44:17Z

Hello! I'm using latest parity ethereum client 2.0.5-beta on ubuntu 18:

Parity Ethereum
version Parity-Ethereum/v2.5.0-beta-b52ac20-20190408/x86_64-linux-gnu/rustc1.33.0

Unfortunately, I still get the error "eth_subscribe(["syncing"]): -32000: This request is not implemented yet. Please create an issue on Github repo."

My js code:

provider.js:

import Api from "@parity/api";

const provider = new Api.Provider.Ws("ws://localhost:8450");

export default provider;

index.js:

import React from "react";
import ReactDOM from "react-dom";

import App from "./App";
import provider from "./provider";

import light, {accounts$, balanceOf$, blockNumber$, post$, transactionCountOf$} from "@parity/light.js";
import Api from '@parity/api';

light.setProvider(provider);

const api = new Api(provider);

accounts$().subscribe(accounts => {
    console.log(accounts);
});

package.json:

{
  "name": "new",
  "version": "1.0.0",
  "description": "",
  "keywords": [],
  "main": "src/index.js",
  "dependencies": {
    "@parity/api": "^5.1.3",
    "@parity/light.js": "5.1.3",
    "ajv": "^6.10.0",
    "react": "16.4.2",
    "react-dom": "16.4.2",
    "react-scripts": "^3.0.0",
    "rxjs": "^6.2.2",
    "typescript": "^3.4.5"
  },
  "devDependencies": {},
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test --env=jsdom",
    "eject": "react-scripts eject"
  },
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  }
}

Missing something?

ordian · 2019-05-03T08:12:59Z

@aawaken

Missing something?

This PR was merged into master (currently 2.6) and wasn't backported to 2.5 (usually, we only backport bug fixes).

aawaken · 2019-05-03T16:43:54Z

@aawaken

Missing something?

This PR was merged into master (currently 2.6) and wasn't backported to 2.5 (usually, we only backport bug fixes).

Thanks @ordian Any suggestions when 2.6 will be released?

Best

seunlanlege added A0-pleasereview 🤓 Pull request needs code review. M6-rpcapi 📣 RPC API. labels Feb 7, 2019

5chdn added this to the 2.4 milestone Feb 7, 2019

axelchalon mentioned this pull request Feb 7, 2019

Implement parity_versionInfo & parity_setChain on LC; fix parity_setChain #10312

Merged

tomusdrw reviewed Feb 8, 2019

View reviewed changes

seunlanlege commented Feb 11, 2019

View reviewed changes

Comment thread ethcore/sync/src/api.rs

seunlanlege commented Feb 11, 2019

View reviewed changes

Comment thread ethcore/sync/src/api.rs

seunlanlege force-pushed the rpc-eth_subscribe-syncing branch 2 times, most recently from fc2e948 to 4868692 Compare February 12, 2019 11:27

seunlanlege added the M4-core ⛓ Core client code / Rust. label Feb 14, 2019

tomusdrw reviewed Feb 18, 2019

View reviewed changes

tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 18, 2019

seunlanlege force-pushed the rpc-eth_subscribe-syncing branch 2 times, most recently from 5fecdc9 to 41cf48b Compare February 19, 2019 11:19

tomusdrw suggested changes Feb 20, 2019

View reviewed changes

5chdn modified the milestones: 2.4, 2.5, 2.6 Feb 21, 2019

soc1c added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Mar 29, 2019

seunlanlege force-pushed the rpc-eth_subscribe-syncing branch from 5eca317 to 53e0c4b Compare March 30, 2019 08:16

seunlanlege added 14 commits April 1, 2019 07:53

implement eth_subscibe syncing

e7ee667

fix imports

77c247e

add is_major_syncing to SyncProvider

6dcecd2

add eth_subscribe(syncing) support for light clients

270f1f5

tests

1c255b8

fix TestSyncClient

1ff4c57

correct LightFetch impl

79cadfa

added currentBlock, startingBlock, highestBlock to PubSubSyncStatus

0077a89

fixed tests

f0674b5

fix PR grumbles

d64640e

improve code style

0055554

is_syncing -> syncing

c0d4568

code style

16a3aba

use ethereum types

fb2a504

seunlanlege force-pushed the rpc-eth_subscribe-syncing branch from 53e0c4b to fb2a504 Compare April 1, 2019 06:56

soc1c modified the milestones: 2.5, 2.6 Apr 2, 2019

soc1c merged commit fba63de into master Apr 2, 2019

soc1c deleted the rpc-eth_subscribe-syncing branch April 2, 2019 15:14

ghost mentioned this pull request Apr 4, 2019

Use eth_subscribe("syncing") for tools/sync.js ethereumclassic/explorer#306

Open

		@@ -699,6 +725,10 @@ pub trait SyncInfo {

		/// Whether major sync is underway.
		fn is_major_importing(&self) -> bool;

Conversation

seunlanlege commented Feb 7, 2019 • edited by axelchalon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Feb 7, 2019

Uh oh!

parity-cla-bot commented Feb 7, 2019

Uh oh!

axelchalon commented Feb 7, 2019

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seunlanlege commented Feb 18, 2019

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

axelchalon commented Feb 18, 2019

Uh oh!

tomusdrw commented Feb 18, 2019

Uh oh!

axelchalon commented Feb 18, 2019

Uh oh!

seunlanlege commented Feb 18, 2019

Uh oh!

seunlanlege commented Feb 19, 2019

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soc1c commented Mar 31, 2019

Uh oh!

aawaken commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ordian commented May 3, 2019

Uh oh!

aawaken commented May 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

seunlanlege commented Feb 7, 2019 •

edited by axelchalon

Loading

aawaken commented May 2, 2019 •

edited

Loading