Make RPCModules independent of concrete chain #121

cburgdorf · 2019-01-08T18:56:08Z

What was wrong?

RPC Modules were locked to depend on BaseAsyncChain .

How was it fixed?

RPCModule is genreic over TChain
RPCServer dropped dependency on the chain so that all it cares about is the name which means it is satisfied by the new non-generic BaseRPCModule.
The chain reset mechanism is now event bus based (making it possible to drop the chain dependency on the RPCServer

Cute Animal Picture

RPCModules need to access the chain regulary so it makes sense for them to depend and provide access to the chain. However, the RPCServer doesn't care about the concrete chain type and we can get away with making it generic since Eth1 and Beacon chain don't share a common ancestor.

`chain` and `event_bus` are being used in child classes and should hence not be written with _ (underscore) notation.

pipermerriam

In order to accomplish #79 I believe that we end up needing to completely or almost completely stop using a remote Chain object.

Ignoring the special evm_resetToGenesisFixture method, that implies that all of our RPC modules which need access to the Chain need to be converted to use one of:

A: Event bus based data retrieval
B: A fully local Chain instance using a local database connection (probably a read-only one)

Doing B depends on database architecture that we cannot support with our existing LevelDB backend.

This leaves us with option A which requires us to create a new service that exposes Chain data over the event bus.

I recognize this pull request isn't exactly targeting #79 but I'm curious if it can be reworked to bring us closer to that goal. Thoughts?

cburgdorf · 2019-01-08T23:06:25Z

The JSON-RPC Server already runs in it's own isolated process and the Chain is a local chain with only the DB ending up being proxied through another process as that's a current requirement that comes from using LevelDB.

As I said in #79, I don't think we should funnel all db read/writes through the event bus and instead focus on moving to something like RocksDB to get rid of remaining process hoping (caused by the db proxy to leveldb).

I believe funneling all db access through the eventbus will almost certainly have a worse performance than going through the db proxy, because:

The native Python proxy code is much more likely to be already heavily optimized for performance whereas the EventBus is still super young alpha software
The EventBus is optimized for convenience and e.g. uses multiple process hops internally whereas the db proxy has a more direct connection.

cburgdorf · 2019-01-08T23:13:17Z

Oh on

This leaves us with option A which requires us to create a new service that exposes Chain data over the event bus.

I want to highlight that just exposing the chain data through another service doesn't buy us anything here because that other service would still need to talk to the DbProxy unless you want to create one dedicated service for all db access as a replacement for the DbProxy which I had assumed with my previous comment.

So, if we just add another service to expose Chain access you end up with: JSON-RPC process -> Chain process -> DbProxy whereas in this PR it is just JSON-RPC process -> DbProxy (since the chain is already a local one).

pipermerriam · 2019-01-08T23:17:23Z

👍 on the correction that this is less about the Chain and more about the core database access which basically means my whole comment can be disregarded. There's one section I want to comment on but I'll do it separately.

pipermerriam · 2019-01-08T23:19:26Z

trinity/rpc/modules/main.py

-def initialize_modules(modules: Iterable[Type[RPCModule]],
-                       chain: BaseAsyncChain,
-                       event_bus: Endpoint) -> Iterable[RPCModule]:
+def initialize_modules(modules: Iterable[Type[RPCModule[TChain]]],


I'm curious about us re-working this API to remove the chain component and instead have the signature be something like initialize_modules(module_classes, trinity_config, event_bus). This should allow any module that needs Chain access to connect to the chain itself, which I think also means that we could drop the Generic.

The problem with this is that under testing, we want to inject a TestChain (which obviously we can't resolve through the TrinityConfig). And if we think about it, being able to pass chain instances into RPCModules gives us much more flexibility in general. The chain is what most of the modules want to access anyway.

The chain is what most of the modules want to access anyway.

There are plenty of modules that don't need the chain which is why I want to get the Chain requirement fully removed from the RPCModule concept.

As for testing and needing to inject a TestChain there are plenty of ways we can solve that problem.

RPC APIs that don't need the Chain

net

admin from https://github.com/ethereum/go-ethereum/wiki/Management-APIs

shh (Whisper)

pss (PSS that Swarm uses)

bzz if we ever get swarm implemented.

There are plenty of modules that don't need the chain which is why I want to get the Chain requirement fully removed from the RPCModule concept.

Ah ok. Well, I kinda removed the Chain already by creating an BaseRPCModule that is basically just an interface that defines an abstract name property.

class BaseRPCModule(ABC): @property @abstractmethod def name(self) -> str: pass

That's the only thing RPC modules need to implement to work with the RPCServer because BaseRPCModule is the only thing the server cares about.

But I realize that I missed the opportunity to make that more clear by providing one class RPCModule that uses a chain and using that class even for modules that don't need the chain (e.g. net).

I'm going to iterate on that and will further comment after I refactored some things to be clearer.

@pipermerriam I pushed another commit for some cleanups and added inline comments to explain.

cburgdorf · 2019-01-09T09:44:41Z

trinity/rpc/modules/__init__.py

+    yield Eth(chain, event_bus)
+    yield EVM(chain, event_bus)
+    yield Net(event_bus)
+    yield Web3()


↑
The BaseRPCModule has zero implementation and only defines an abstract name (so more of an interface than a base class). We initialize every module with the dependencies that the specific module needs. But since no one outside of the module cares about these internals the returned value can be treated as Tuple[BaseRPCModule, ...]

cburgdorf · 2019-01-09T09:48:48Z

trinity/rpc/modules/main.py

@@ -40,29 +35,20 @@ def name(self) -> str:
        pass


-class RPCModule(BaseRPCModule, Generic[TChain]):
+class ChainBasedRPCModule(BaseRPCModule, Generic[TChain]):


This is just to have a DRY base class for all the modules that do depend on a chain. No part of the RPC system cares about it except the module itself. Every other code place just deals with BaseRPCModule.

cburgdorf · 2019-01-09T09:49:57Z

trinity/rpc/modules/net.py


-class Net(Eth1RPCModule):
+    def __init__(self, event_bus: Endpoint):


The Net module even defines its very own __init__ as it needs an event bus but no chain.

cburgdorf added 2 commits January 8, 2019 19:52

Rename properties falsly declared as private

7bd9887

`chain` and `event_bus` are being used in child classes and should hence not be written with _ (underscore) notation.

cburgdorf changed the title ~~Christoph/feat/rpc beacon modules~~ Make RPCModules independent of concrete chain Jan 8, 2019

pipermerriam reviewed Jan 8, 2019

View reviewed changes

Use BaseRPCModule whereever possible

4b63be3

cburgdorf commented Jan 9, 2019

View reviewed changes

pipermerriam approved these changes Jan 9, 2019

View reviewed changes

cburgdorf merged commit d14ad79 into ethereum:master Jan 9, 2019

pipermerriam mentioned this pull request Jan 9, 2019

Implement eth_getTransactionByHash, eth_getTransactionReceipt JSON RPC function #122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make RPCModules independent of concrete chain #121

Make RPCModules independent of concrete chain #121

cburgdorf commented Jan 8, 2019 •

edited

Loading

pipermerriam left a comment

cburgdorf commented Jan 8, 2019

cburgdorf commented Jan 8, 2019

pipermerriam commented Jan 8, 2019

pipermerriam Jan 8, 2019

cburgdorf Jan 8, 2019

pipermerriam Jan 8, 2019

cburgdorf Jan 9, 2019 •

edited

Loading

cburgdorf Jan 9, 2019

cburgdorf Jan 9, 2019 •

edited

Loading

cburgdorf Jan 9, 2019

cburgdorf Jan 9, 2019


		class Net(Eth1RPCModule):
		def __init__(self, event_bus: Endpoint):

Make RPCModules independent of concrete chain #121

Make RPCModules independent of concrete chain #121

Conversation

cburgdorf commented Jan 8, 2019 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

pipermerriam left a comment

Choose a reason for hiding this comment

cburgdorf commented Jan 8, 2019

cburgdorf commented Jan 8, 2019

pipermerriam commented Jan 8, 2019

pipermerriam Jan 8, 2019

Choose a reason for hiding this comment

cburgdorf Jan 8, 2019

Choose a reason for hiding this comment

pipermerriam Jan 8, 2019

Choose a reason for hiding this comment

RPC APIs that don't need the Chain

cburgdorf Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

cburgdorf Jan 9, 2019

Choose a reason for hiding this comment

cburgdorf Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

cburgdorf Jan 9, 2019

Choose a reason for hiding this comment

cburgdorf Jan 9, 2019

Choose a reason for hiding this comment

cburgdorf commented Jan 8, 2019 •

edited

Loading

RPC APIs that don't need the `Chain`

cburgdorf Jan 9, 2019 •

edited

Loading

cburgdorf Jan 9, 2019 •

edited

Loading