rpc: new RPC implementation with pub/sub support #2035

bas-vk · 2015-12-03T11:25:14Z

This PR includes a new RPC implementation that is build on top of the new node architecture in the node. It adds support for pub/sub.

The new implementation lacks support for the admin and debug modules which the current interface provides since there is a debate how we can accommodate these in the new architecture.

Therefore a new flag --ipcexp flag is added. This will start the new RPC implementation. By default the old IPC interface is started.

closes #1935

robotally · 2015-12-03T11:25:15Z

Vote	Count	Reviewers
👍	2	@fjl @karalabe
👎	0

Updated: Mon Dec 14 16:28:16 UTC 2015

codecov-io · 2015-12-03T11:30:58Z

Current coverage is `44.21%`

Merging #2035 into develop will decrease coverage by -2.80% as of 8939d7a

Powered by Codecov. Updated on successful CI builds.

karalabe · 2015-12-04T10:21:41Z

A few things I've noticed while adding the admin stuff:

I wouldn't add a special type for rpc.Apis (aliasing []*rpc.Api). The second version is cleaner to understand and the only reason some of the core types have the plurals aliased is because they use custom RLP serialization rules. However I would avoid aliasing as it's just harder to understand looking at the code what it exactly is.
Instead of []*rpc.Api, I would use plain []rpc.Api to stay consistent with the []p2p.Protocol design. An rpc.Api either way contains very little data and is passed around only during construction, so it doesn't cost anything significant. It also helps when printing it as you can just display its contents and not have Go resolve it into a pointer.
Naming wise in the eth, whisper, etc package I don't think it's the best idea to name the file containing the API methods eth_rpc.go or whisper_rpc.go. If there's only a single file, then imho it's cleaner to simply name it rpc.go, or I would even consider api.go instead of RPC, as it actually contains the API that will or will not be exposed over RPC.

karalabe · 2015-12-04T10:26:18Z

Also I think naming these API handlers as XyzService isn't the best idea, because we already have a Service concept in the node (e.g. Whisper, Ethereum, Swarm, etc). I would recommend naming the API method containers as <Name><Public/Private>Api. e.g: EthereumPublicApi, EthereumPrivateApi, BlockchainAdminPrivateApi. This way it would be much cleaner what they contain and what their access scope is meant to be opposed to simply BlockChainService which eou cannot really tie to API/RPC, neither say whether it will be public/private.

karalabe · 2015-12-04T19:07:36Z

Just so that it's not lost, here's my PR against this PR bas-vk#1

bas-vk · 2015-12-07T11:13:04Z

@karalabe, made the rename changes as proposed.

zelig · 2015-12-10T17:57:06Z

I second Peter that Service is better avoided as a field name in rpc.Api
Also rpc.Api#Service is interface{}, but in fact all apis implement a Version method. If this is explicit and the api needs to implement Version() then we do not need Version field on the rpc.Api struct. Alternatively a Version rpc method should automatically insert based on the field Version of the rpc.Api.
However, did we not decide that only services (node.Service) will be versioned, their api just follows.
So we can eliminate both api internal Version methods as well as Version field in the rpc.Api struct and let the node start insert the version method.
What do you think?

zelig · 2015-12-10T18:01:35Z

As for the api naming. I agree that its best to just call it rpc.go within each package.
As for the types, I would not be so rigid as peter suggests, for instance in swarm there will be at least 2 different private api packages, which will be (i think correctly) named based on the funcitonality they provide: one for network test and one for swarm API with file-system access

zelig · 2015-12-10T18:10:19Z

Also, if we all get pedantic about naming of services, protocols then I gotta say, the web3.js selectors aka rpc.Api#Namespace should also be: ethereum, swarm and whisper, not eth, shh, bzz.
But I guess we might be stuck with those ...

zelig · 2015-12-11T01:34:04Z

cmd/gethrpctest/main.go

+
+	web3 := utils.NewPublicWeb3Api(stack)
+	server.RegisterName("web3", web3)
+	net := utils.NewPublicNetApi(stack.Server(), ethereum.NetVersion())


this is the only use of ethereum here. NetVersion should clearly not be there.

fjl · 2015-12-13T21:25:41Z

Found during testing: JSON-RPC notifications sent by the client generate a response with id 0. Please fix.

fjl · 2015-12-13T21:39:03Z

When I was requesting to rename "Api" to "API", I probably should've said that this also applies to things like "PublicEthereumApi".
Please do the renaming for those types, too.

fjl · 2015-12-13T21:49:17Z

Can the core APIs be moved out of package core please?
We really want to avoid adding dependencies core->rpc and core->accounts.
The right place (for now) is package eth, because it is (still) the main entry point around Ethereum consensus.

I tried moving it to package eth and a bunch of work is required to make it work (can't access BlockChain.chainDb, etc.).
Maybe we can merge the PR without the move now and do it afterwards.

Another issue with the APIs in package core is that even though they are supposed to be "public",
they contain methods that do signing. Moving tx signing/submission (and signing in general) to a separate
API can also be done in a follow up PR.

fjl · 2015-12-13T21:56:35Z

rpc/v2/server.go

+			}
+		}
+
+		responses[i] = codec.CreateResponse(req.id, reply[0].Interface())


Please find a way to share this code (inside the for loop) with exec.

fjl · 2015-12-14T14:43:03Z

core/vm/log.go

+		"logIndex":         rpc.NewHexNumber(r.Index),
+		"blockHash":        r.BlockHash,
+		"transactionHash":  r.TxHash,
+		"transactionIndex": rpc.NewHexNumber(r.TxIndex),


rpc.NewHexNumber(...) here should be replaced by fmt.Sprintf("%x", ...) to avoid the dependency on rpc.

fjl · 2015-12-14T16:18:15Z

👍

karalabe · 2015-12-14T16:28:15Z

I've skimmed through the PR before building my own admin/debug ports on top of it and it seemed solid. Apart from a few issues I've mentioned above and were already sorted out; and a few cleaning refactoring that will either way get done as we're progressing with the RPC updates, it seems ok for me. 👍

rpc: new RPC implementation with pub/sub support

* integrate #2035 via latest develop. fixes #2041 * refactor apis * fix tests for windows, add simple download test * separate dns api and chequebook api * http proxy api in separate subpackage * remove legacy rpc code partially, adding only legacy to make v2 work * TODO: add dns tests see #2048 * TODO: further refactor due to #2040

all: pull snap sync PRs from upstream v1.13.5

obscuren added the in progress label Dec 3, 2015

bas-vk mentioned this pull request Dec 3, 2015

rpc/v2: new RPC implementation #1912

Closed

bas-vk added review and removed in progress labels Dec 3, 2015

bas-vk force-pushed the rcp-v2-rebase branch from 1c704f2 to 32fefdb Compare December 7, 2015 11:03

zelig mentioned this pull request Dec 9, 2015

[swarm] swarm api integration with rpc v2 + node services #2041

Closed

2 tasks

zelig reviewed Dec 11, 2015
View reviewed changes

zelig mentioned this pull request Dec 11, 2015

Swarm/rpc v2 #2066

Closed

bas-vk force-pushed the rcp-v2-rebase branch from 32fefdb to 4b28705 Compare December 11, 2015 14:34

fjl reviewed Dec 13, 2015
View reviewed changes

bas-vk force-pushed the rcp-v2-rebase branch from 4b28705 to f3fcf67 Compare December 14, 2015 12:49

fjl reviewed Dec 14, 2015
View reviewed changes

bas-vk force-pushed the rcp-v2-rebase branch 2 times, most recently from d1e2312 to eae8146 Compare December 14, 2015 15:33

rpc: new RPC implementation with pub/sub support

eae8146

fjl added a commit that referenced this pull request Dec 14, 2015

Merge pull request #2035 from bas-vk/rcp-v2-rebase

fa187a3

rpc: new RPC implementation with pub/sub support

fjl merged commit fa187a3 into ethereum:develop Dec 14, 2015

fjl removed the review label Dec 14, 2015

zelig mentioned this pull request Dec 14, 2015

integrate RPC v2 via develop #2073

Merged

5 tasks

obscuren modified the milestone: 1.4.0 Feb 6, 2016

sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Dec 19, 2023

Merge pull request ethereum#2035 from weiihann/v1.3.4-snapsync

474860e

all: pull snap sync PRs from upstream v1.13.5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: new RPC implementation with pub/sub support #2035

rpc: new RPC implementation with pub/sub support #2035

bas-vk commented Dec 3, 2015

robotally commented Dec 3, 2015

codecov-io commented Dec 3, 2015

karalabe commented Dec 4, 2015

karalabe commented Dec 4, 2015

karalabe commented Dec 4, 2015

bas-vk commented Dec 7, 2015

zelig commented Dec 10, 2015

zelig commented Dec 10, 2015

zelig commented Dec 10, 2015

zelig Dec 11, 2015

fjl commented Dec 13, 2015

fjl commented Dec 13, 2015

fjl commented Dec 13, 2015

fjl Dec 13, 2015

fjl Dec 14, 2015

fjl commented Dec 14, 2015

karalabe commented Dec 14, 2015

rpc: new RPC implementation with pub/sub support #2035

rpc: new RPC implementation with pub/sub support #2035

Conversation

bas-vk commented Dec 3, 2015

robotally commented Dec 3, 2015

codecov-io commented Dec 3, 2015

Current coverage is 44.21%

karalabe commented Dec 4, 2015

karalabe commented Dec 4, 2015

karalabe commented Dec 4, 2015

bas-vk commented Dec 7, 2015

zelig commented Dec 10, 2015

zelig commented Dec 10, 2015

zelig commented Dec 10, 2015

zelig Dec 11, 2015

Choose a reason for hiding this comment

fjl commented Dec 13, 2015

fjl commented Dec 13, 2015

fjl commented Dec 13, 2015

fjl Dec 13, 2015

Choose a reason for hiding this comment

fjl Dec 14, 2015

Choose a reason for hiding this comment

fjl commented Dec 14, 2015

karalabe commented Dec 14, 2015

Current coverage is `44.21%`