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

Light client friendly events #2491

Merged
merged 20 commits into from
May 13, 2019
Merged

Light client friendly events #2491

merged 20 commits into from
May 13, 2019

Conversation

pepyakin
Copy link
Contributor

@pepyakin pepyakin commented May 6, 2019

This PR proposes a solution for light client friendly SRML events.

Each deposited event can specify zero or more topics. A topic is a fixed sized value (atm, is just a hash). When an event is deposited with some topic, the runtime adds the index of this event in the <Events<T>> list to a mapping that maps from topics to lists of indexes.

Light clients can "subscribe" to changes of specific storage values (see #628). The idea is that light clients would subscribe to changes of the storage values for a topic they are interested in. When the light client detects a change, it fetches the contents of the indexes list for the specific topic and with this the light client can query events' data by the indexes.

Marking this as draft, since I don't know if this is the way that we want to go, I didn't test this with light-client nor inspected the storage values and there are some questions left

@pepyakin pepyakin added the A3-in_progress Pull request is in progress. No review needed at this stage. label May 6, 2019
@gavofyork
Copy link
Member

Be good to get an opinion on this @svyatonik

@svyatonik svyatonik self-assigned this May 8, 2019
svyatonik
svyatonik reviewed May 8, 2019
View reviewed changes
Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea seems OK to me. I'll submit some issues soon to not-to-forget what needs to be changed with changes tries. We need (at least) to extend changes tries API (and network messages) to allow fetching changes for several keys (here: topics) at once.

@pepyakin
Copy link
Contributor Author

pepyakin commented May 8, 2019

I've checked the storage entries and add a test. This should be ready for review then.

@pepyakin pepyakin marked this pull request as ready for review May 8, 2019 14:53
@pepyakin
Merge 'origin/master' into ser-topic-events
7d97836 
# Conflicts:
#	node/runtime/src/lib.rs
@pepyakin pepyakin added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 8, 2019
pepyakin
pepyakin commented May 8, 2019
View reviewed changes
srml/system/src/lib.rs Outdated Show resolved Hide resolved
@pepyakin pepyakin added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels May 8, 2019
@pepyakin
Copy link
Contributor Author

pepyakin commented May 8, 2019

There might be a problem with this approach, marking it inprogress until we figure it out.

@gavofyork
Copy link
Member

what might the problem be?

@pepyakin
Copy link
Contributor Author

pepyakin commented May 9, 2019 •
edited
Loading

Yes, there is indeed a problem, although not a significant one.

Consider the case when an event E was deposited with a topic A.

So at the end of the block N the field <EventTopics<T>> will be like:

A => vec![EventIndex(0)] // 0 is the index of `A` in `<Events<T>>`

then, let's suppose that the same event E with the same topic A was deposited at the block N+1. Therefore, <EventTopics<T>> will look the same at the end of the N+1 block:

A => vec![EventIndex(0)]

Because the value at the storage location of A doesn't change there will be no notification of change between for the N+1.

I am still not entirely sure, but it seems to be a problem. If the light-client doesn't follow each block, and say, it "subscribes" to changes at the block N+1 it won't get the update.

To circumvent this problem we could store Vec<(EventIndex, BlockNumber)> in EventTopics instead of just Vec<EventIndex>. Or maybe even something like Option<(BlockNumber, Vec<EventIndex>)>.

@pepyakin
Copy link
Contributor Author

pepyakin commented May 9, 2019

Ok, I put up a commit which fixes this. However, I went with Vec<(EventIndex, BlockNumber)> instead of Option<(BlockNumber, Vec<EventIndex>)>. The reason for that is ::append is not available for this type. See the commit for details:
5f9ca06

Apparently it starts asking for a hand-rolled structure tailored for this particular use-case.

pepyakin added 3 commits May 10, 2019 15:43
@pepyakin
Fix srml-system test.
24b944f
@pepyakin
Merge 'origin/master' into ser-topic-events
f57e32b 
# Conflicts:
#	core/test-runtime/wasm/Cargo.lock
#	node-template/runtime/wasm/Cargo.lock
#	node/executor/src/lib.rs
#	node/runtime/src/lib.rs
#	node/runtime/wasm/Cargo.lock
@pepyakin
Post merge fixes.
40b296e
@pepyakin pepyakin added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels May 10, 2019
@gavofyork
Copy link
Member

ok, but please add a TODO and issue explaining that this is highly inefficient and needs to be altered to Option<(BlockNumber, Vec<EventIndex>)> once we can implement ::append for it. please also assign it 1.x milestone

@pepyakin pepyakin modified the milestone: 1.x series May 13, 2019
@pepyakin pepyakin mentioned this pull request May 13, 2019
Open
@svyatonik svyatonik added A8-looksgood and removed A0-please_review Pull request needs code review. labels May 13, 2019
@pepyakin pepyakin mentioned this pull request May 13, 2019
Merged
@gavofyork gavofyork merged commit 318d2e0 into master May 13, 2019
@gavofyork gavofyork deleted the ser-topic-events branch May 13, 2019 18:56
@xlc
Copy link
Contributor

xlc commented May 13, 2019

This seems like a breaking change and requires JS SDK side change as well.
@jacogr I don't think it is easy to have a JS SDK that compatible with Substrate versions that with and without this change.

@xlc xlc mentioned this pull request May 14, 2019
Closed
@svyatonik svyatonik mentioned this pull request Jun 14, 2019
Closed
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019
@pepyakin @MTDK1
Light client friendly events (paritytech#2491)
df4fba5 
* Sketch of indexed events.

* Get EventIndex by holding another variable.

* Add some docs.

* Use DoubleMap to store reverse topic index

* Implement StorageDoubleMap::append

* Use append for EventTopics.

* Refactor.

* Avoid `mutate`

* Docs.

* Add topics to EventRecord

* Update tests.

* Rebuild.

* Bump version.

* Event topics test.

* Mix in BlockNumber to distinguish updates

* Fix srml-system test.

* Post merge fixes.

* Comments/TODO.
@shawntabrizi shawntabrizi mentioned this pull request Jan 8, 2020
Merged
@dvdplm dvdplm mentioned this pull request Feb 12, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gavofyork gavofyork gavofyork left review comments

@svyatonik svyatonik svyatonik approved these changes

Assignees

@svyatonik svyatonik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pepyakin @gavofyork @xlc @svyatonik @devops-parity