Storage changes subscription #464

tomusdrw · 2018-07-31T13:36:29Z

Exposing storage changes via RPC pub-sub
Allows one to subscribe to particular storage keys

jacogr · 2018-07-31T13:55:47Z

substrate/rpc/src/chain/mod.rs

 		#[pubsub(name = "chain_newHead")] {
 			/// New head subscription
-			#[rpc(name = "subscribe_newHead")]
+			#[rpc(name = "subscribe_newHead", alias = ["chain_subscribeNewHead", ])]


Great. Maybe it is better to have the "old" as the alias and the "new" one as the name?

(e.g. chain_subscribeNewHead is actually probably the preferred one to match since it aligns with other RPCs, my gut tells me the preferred one should be the "default")

dvdplm

Good stuff. I can't say I understand it all, but overall very readable and interesting.

dvdplm · 2018-07-31T13:47:48Z

substrate/client/src/client.rs

+
+	/// Get storage changes event stream.
+	///
+	/// Passing `None` as keys subscribes to all possible keys


typo: …as keys should be …as key

Rephrased the whole sentence, hope it's clearer now.

dvdplm · 2018-07-31T13:50:08Z

substrate/client/src/client.rs

-		if let Some(storage_update) = storage_update {
+		if let Some((storage_update, changes)) = storage_update {
 			transaction.update_storage(storage_update)?;
+			// TODO [ToDr] How to handle re-orgs? Should we re-emit all storage changes?


Couldn't we emit a re-org event with some meta data about what changed and let interested subscribers re-fetch?

Yes, that's possible, although it's not easy to get storage changes for blocks that are already imported/executed. Re-fetching changes would mean to re-execute the blocks, but I suppose based on the filter_keys we could just return all the storage values in the re-orged blocks.

dvdplm · 2018-07-31T13:59:33Z

substrate/client/src/notifications.rs

+				subscribers.extend(listeners.iter());
+			}
+
+			if has_wildcard || listeners.is_some() {


Couldn't you check for !susbscribers.is_empty() here? Or is it faster to do it this way?

I'm actually interested only in subscribers for that particular key. So if there is a set of changes:

[(1, Some(2)), (2, None), (3, Some(4))]

but we have no wildcard_listeners and only listener for key=1, changes vector will only contain [(StorageKey(1), Some(StorageData(2))]

dvdplm · 2018-07-31T14:04:20Z

substrate/client/src/notifications.rs

+						filter: filter.clone(),
+					})).is_err()
+				},
+				None => false,


If I read this right, if the subscriber is gone when we get here, then we assume they've been removed properly already, so we're returning false to avoid calling remove_subscriber() again for them? It's a bit unclear to me how they can still be in the subscribers collection though, can you elaborate on that?

Indeed, I could actually .expect() here, since if the structure is consistent the subscribers should always be in self.sinks. The check here is superfluous.
I could refactor to:

let &(ref sink, ref filter) = self.sinks.get(&subscriber).expect("subscribers returned from self.listeners are always in self.sinks; qed"); let result = sink.unbounded_send((hash.clone(), StorageChangeSet { changes: changes.clone(), filter: filter.clone(), })); if result.is_err() { self.remove_subscriber(subscriber); }

or

if let Some(&(ref sink, ref filter)) = match self.sinks.get(&subscriber) { let result = sink.unbounded_send((hash.clone(), StorageChangeSet { changes: changes.clone(), filter: filter.clone(), })); if result.is_err() { self.remove_subscriber(subscriber); } }

Which one do you prefer?

Oh, actually I can't since .get() borrows immutably, so remove_subscriber has to be outside of the scope.

dvdplm · 2018-07-31T14:11:01Z

substrate/client/src/notifications.rs

+			assert_eq!(notifications.listeners.len(), 2);
+			assert_eq!(notifications.wildcard_listeners.len(), 1);
+		}
+


The channels are closed here, correct?

Yes and since the receiving end is dropped, sending to such channel will trigger an error.

dvdplm · 2018-07-31T14:15:35Z

substrate/rpc/src/state/mod.rs

+			sink
+				.sink_map_err(|e| warn!("Error sending notifications: {:?}", e))
+				.send_all(stream)
+				// we ignore the resulting Stream (if the first stream is over we are unsubscribed)


…is over…? Do you mean …is closed?

is over as in is finished/is done. Which means that the stream will not emit any more items.

dvdplm · 2018-07-31T14:18:04Z

substrate/state-machine/src/lib.rs

+	/// Drain committed changes to an iterator.
+	///
+	/// Panics:
+	/// Will panic if there are any uncommitted prospective changes.


Is "prospective" sort of like "pending"?

Yes, those are changes that can still be easily discarded. You can see example of usage inside block builder:

We run a transaction

It produces a set of prospective changes

If we detect that it's somehow invalid we discard the prospective changes

If we accept the transaction we commit prospective changes.

svyatonik · 2018-08-01T06:25:56Z

substrate/client/src/client.rs

+		if let Some((storage_update, changes)) = storage_update {
 			transaction.update_storage(storage_update)?;
+			// TODO [ToDr] How to handle re-orgs? Should we re-emit all storage changes?
+			self.storage_notifications.lock()


Are you sure that it should be called before transaction is committed?

Good point. Moved the notification after commit and also guarded by the same if as block import notification.

gavofyork

Aside from the minor comment

gavofyork · 2018-08-01T11:32:18Z

substrate/client/src/notifications.rs

@@ -0,0 +1,267 @@
+// Copyright 2017 Parity Technologies (UK) Ltd.
+// This file is part of Polkadot.


Substrate, not Polkadot :)

* master: Collator for the "adder" (formerly basic-add) parachain and various small fixes (#438) Storage changes subscription (#464) Wasm execution optimizations (#466) Fix the --key generation (#475) Fix typo in service.rs (#472) Fix session phase in early-exit (#453) Make ping unidirectional (#458) Update README.adoc

* Initial implementation of storage events. * Attaching storage events. * Expose storage modification stream over RPC. * Use FNV for hashing small keys. * Fix and add tests. * Swap alias and RPC name. * Fix demo. * Addressing review grumbles. * Fix comment.

Skip plot if it is too small

* Bump release version to v0.18.0 Signed-off-by: Alexandru Vasile <[email protected]> * Update changelog Signed-off-by: Alexandru Vasile <[email protected]> * Update dependency version to v0.18.0 Signed-off-by: Alexandru Vasile <[email protected]> * Modify changelog Signed-off-by: Alexandru Vasile <[email protected]> * Move changelog entries from added to changed

tomusdrw added 6 commits July 25, 2018 15:42

Initial implementation of storage events.

d9ce1f9

Attaching storage events.

59f3a3b

Merge branch 'master' into td-storage-events

f9f8099

Expose storage modification stream over RPC.

b157e1d

Use FNV for hashing small keys.

44e07d6

Fix and add tests.

b6f0967

tomusdrw added A0-please_review Pull request needs code review. M6-rpcapi labels Jul 31, 2018

jacogr reviewed Jul 31, 2018

View reviewed changes

Swap alias and RPC name.

7b3b494

dvdplm approved these changes Jul 31, 2018

View reviewed changes

svyatonik reviewed Aug 1, 2018

View reviewed changes

tomusdrw added 2 commits August 1, 2018 11:29

Fix demo.

79d3507

Addressing review grumbles.

4883259

gavofyork approved these changes Aug 1, 2018

View reviewed changes

gavofyork removed the A0-please_review Pull request needs code review. label Aug 1, 2018

Fix comment.

e5e7c4c

tomusdrw force-pushed the td-storage-events branch from 3d6c0d9 to e5e7c4c Compare August 1, 2018 11:37

svyatonik merged commit 757a721 into master Aug 1, 2018

svyatonik deleted the td-storage-events branch August 1, 2018 12:29

liuchengxu pushed a commit to autonomys/substrate that referenced this pull request Jun 3, 2022

Merge pull request paritytech#464 from subspace/farmer-drop-plot

62c3e19

Skip plot if it is too small

		@@ -0,0 +1,267 @@
		// Copyright 2017 Parity Technologies (UK) Ltd.
		// This file is part of Polkadot.

Storage changes subscription #464

Storage changes subscription #464

Uh oh!

Conversation

tomusdrw commented Jul 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvdplm left a comment

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!

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!

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!

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!

gavofyork left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants