Relay all block data + statediffs over full node websocket subscription #8

i-norden · 2019-04-15T16:39:19Z

i-norden commented

2019-04-15 16:39:19 +00:00

Statediffing service API to relay payloads over a web socket subscription
Adjusting statediffs to be closer to the form needed for converting to and publishing as IPLDs
Adjusting statediffs to carry their proofs and paths out with them
Flattening out the statediff directory to try to closer match the organization of Geth

This build is focused on the seed node and I've gotten rid of the CSV stuff, because I think it is more likely to get accepted into Geth this way (there's no precedent for writing out CSVs anywhere else in Geth), so this might not be ideal for all purposes.

This is the branch that syncAndPublish needs to work with.

Also, I made a point to move files in the same commit but for some reason it looks like some history was still lost...

* Statediffing service API to relay payloads over a web socket subscription * Adjusting statediffs to be closer to the form needed for converting to and publishing as IPLDs * Adjusting statediffs to carry their proofs and paths out with them * Flattening out the statediff directory to try to closer match the organization of Geth This build is focused on the seed node and I've gotten rid of the CSV stuff, because I think it is more likely to get accepted into Geth this way (there's no precedent for writing out CSVs anywhere else in Geth), so this might not be ideal for all purposes. This is the branch that [syncAndPublish](https://github.com/vulcanize/vulcanizedb/tree/syncAndPublish) needs to work with. Also, I made a point to move files in the same commit but for some reason it looks like some history was still lost...

AFDudley reviewed 2019-04-15 16:39:19 +00:00

ana0 (Migrated from github.com) reviewed 2019-04-15 16:39:19 +00:00

dtkong (Migrated from github.com) reviewed 2019-04-15 16:39:19 +00:00

Gslaughl (Migrated from github.com) reviewed 2019-04-15 16:39:19 +00:00

m0ar (Migrated from github.com) reviewed 2019-04-15 16:39:19 +00:00

rmulhol (Migrated from github.com) reviewed 2019-04-15 16:39:19 +00:00

yaoandrew (Migrated from github.com) reviewed 2019-04-15 16:39:19 +00:00

elizabethengelman (Migrated from github.com) reviewed 2019-04-18 19:58:01 +00:00

elizabethengelman (Migrated from github.com) left a comment

This is looking super cool, thanks for doing all this work! 🎉 🏅 💯

I wonder if we should open this PR against staging or statediffing? From what I remember, the statediffing branch was a bit more up-to-date than staging - was current with geth v1.8.19 (I think). It also makes sense to me that we include the state diff work separate from the work that gets this repo up-to-date.

This is looking super cool, thanks for doing all this work! 🎉 🏅 💯 I wonder if we should open this PR against `staging` or [`statediffing`](https://github.com/vulcanize/go-ethereum/tree/statediffing)? From what I remember, the `statediffing` branch was a bit more up-to-date than `staging` - was current with geth v1.8.19 (_I think_). It also makes sense to me that we include the state diff work separate from the work that gets this repo up-to-date.

statediff/api.go

						
				@ -0,0 +51,4 @@

				}

				// Subscribe is the public method to setup a subscription that fires off state-diff payloads as they are created

				func (api *PublicStateDiffAPI) Subscribe(ctx context.Context) (*rpc.Subscription, error) {

elizabethengelman (Migrated from github.com) commented

2019-04-18 19:44:54 +00:00

I keep confusing the two concepts of subscription that we have within the statediff pkg:

I wonder if there's a way to distinguish these a bit more? Perhaps I am getting it confused because it's new, and I need to sit with the code a bit more. So this may be a non-issue 🙃

I keep confusing the two concepts of subscription that we have within the statediff pkg: - [subscription to the blockchain's chain events](https://github.com/vulcanize/go-ethereum/blob/edf001e1d2296951e7e592c55e66ce074bd62807/statediff/service.go#L113) - [rpc connection subscription](https://github.com/vulcanize/go-ethereum/blob/edf001e1d2296951e7e592c55e66ce074bd62807/statediff/api.go#L53) I wonder if there's a way to distinguish these a bit more? Perhaps I am getting it confused because it's new, and I need to sit with the code a bit more. So this may be a non-issue 🙃

statediff/api.go

						
				@ -0,0 +80,4 @@

									log.Error("Failed to unsubscribe from the state diff service", err)

								}

								return

							case <-notifier.Closed():

elizabethengelman (Migrated from github.com) commented

2019-04-18 19:46:28 +00:00

It looks like this Closed() method is deprecated in favor of an error channel

It looks like this `Closed()` method is [deprecated](https://github.com/vulcanize/go-ethereum/blob/master/rpc/subscription.go#L142) in favor of an error channel

statediff/builder/builder_test.go

elizabethengelman (Migrated from github.com) commented

2019-04-16 21:36:26 +00:00

💯

elizabethengelman (Migrated from github.com) commented

2019-04-16 21:39:40 +00:00

Should this be updated to 2019?

statediff/builder/struct.go

elizabethengelman (Migrated from github.com) commented

2019-04-16 21:38:19 +00:00

These notes are helpful, but I'm wondering if we want to check them in here.

statediff/service/api.go

elizabethengelman (Migrated from github.com) commented

2019-04-18 18:04:59 +00:00

same here - 2019?

i-norden reviewed 2019-04-19 14:34:43 +00:00

statediff/builder/builder_test.go

i-norden commented

2019-04-19 14:34:42 +00:00

Oh yeah, it probably should! The license date is different across a lot of the files but I think new files should have the year they were created.

i-norden reviewed 2019-04-19 14:39:11 +00:00

statediff/api.go

						
				@ -0,0 +51,4 @@

				}

				// Subscribe is the public method to setup a subscription that fires off state-diff payloads as they are created

				func (api *PublicStateDiffAPI) Subscribe(ctx context.Context) (*rpc.Subscription, error) {

i-norden commented

2019-04-19 14:39:10 +00:00

I'm definitely not opposed to making this distinction more clear, the issue is that the rpc connection subscription method needs to be named Subscribe to work with this rpcClient method.

I'm definitely not opposed to making this distinction more clear, the issue is that the rpc connection subscription method needs to be named `Subscribe` to work with [this rpcClient method](https://github.com/vulcanize/go-ethereum/blob/rpc_statediffs_at_head/rpc/client.go#L405).

i-norden reviewed 2019-04-19 14:40:57 +00:00

statediff/api.go

						
				@ -0,0 +80,4 @@

									log.Error("Failed to unsubscribe from the state diff service", err)

								}

								return

							case <-notifier.Closed():

i-norden commented

2019-04-19 14:40:57 +00:00

Nice catch! So that means we can just drop this notifier.Closed() and the rpcSub.Err() should take care of everything?

Nice catch! So that means we can just drop this `notifier.Closed()` and the `rpcSub.Err()` should take care of everything?

i-norden commented

2019-04-19 14:45:48 +00:00

Hey @elizabethengelman thank you for the review! You're right about it being better to open up the PR against statediffing, although staging is now at a higher head state closer to where we would want to merge back into the original repo. So I'm going to rebase statediffing to the same state before opening this PR against it, if that is okay. I was also a bit concerned about overwriting work on statediffing since I've gotten rid of the CSV stuff here and I'm not sure if that is still relevant to mcd_transformers or something else.

Hey @elizabethengelman thank you for the review! You're right about it being better to open up the PR against `statediffing`, although `staging` is now at a higher head state closer to where we would want to merge back into the original repo. So I'm going to rebase `statediffing` to the same state before opening this PR against it, if that is okay. I was also a bit concerned about overwriting work on `statediffing` since I've gotten rid of the CSV stuff here and I'm not sure if that is still relevant to mcd_transformers or something else.

elizabethengelman (Migrated from github.com) reviewed 2019-04-22 21:01:21 +00:00

statediff/api.go

						
				@ -0,0 +80,4 @@

									log.Error("Failed to unsubscribe from the state diff service", err)

								}

								return

							case <-notifier.Closed():

elizabethengelman (Migrated from github.com) commented

2019-04-22 21:01:21 +00:00

Yep, that's how I'm interpreting it!

elizabethengelman commented

2019-04-22 21:04:37 +00:00

(Migrated from github.com)

@i-norden Good call, I wonder if it makes sense keeping a branch around with the CSV state diff work, just in case we want to go back to that in the future. 🤔

Update: just pushed up this branch, just in case we'd like to go back an look at the csv work, once this socket work is merged into statediffing.

@i-norden Good call, I wonder if it makes sense keeping a branch around with the CSV state diff work, just in case we want to go back to that in the future. 🤔 Update: just pushed up [this branch](https://github.com/vulcanize/go-ethereum/tree/statediffing-writing-out-to-csv-old), just in case we'd like to go back an look at the csv work, once this socket work is merged into `statediffing`.

👍 1