Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RPC relay #25

Merged
merged 38 commits into from
Jul 24, 2019
Merged

RPC relay #25

merged 38 commits into from
Jul 24, 2019

Conversation

unnawut
Copy link
Contributor

@unnawut unnawut commented Jun 21, 2019

Issue/Task Number: #5, #6
Closes #5
Closes #6

Overview

Basic Ethereum-flavored JSON-RPC request/response relay between Potterhat users and the nodes behind the scene.

Supports Parity for now.

Changes

  • Added PotterhatRPC subapp.

Implementation Details

This is a dumb relay to Ethereum nodes, meaning that discrepancies
between implementations like Geth v.s. Parity are not handled. It is up to the user to make sure
that either:

  1. Potterhat's consumers do not rely on any client-type specific features, or
  2. Use the same client type for all Potterhat backend nodes

Cross-client support maybe considered for Potterhat in the future but it will take some time.

Usage

  1. Configure backend nodes via environment variables, e.g.
export RPC_PORT=8545
...
export POTTERHAT_NODE_1_ID="local_geth"
export POTTERHAT_NODE_1_LABEL="Local Geth"
export POTTERHAT_NODE_1_CLIENT="geth"
export POTTERHAT_NODE_1_RPC="http://localhost:8645"
export POTTERHAT_NODE_1_WS="ws://localhost:8646"
export POTTERHAT_NODE_1_PRIORITY=10
...
export POTTERHAT_NODE_2_ID="cluster_geth"
export POTTERHAT_NODE_2_LABEL="Cluster Geth"
export POTTERHAT_NODE_2_CLIENT="geth"
export POTTERHAT_NODE_2_RPC="http://localhost:8745"
export POTTERHAT_NODE_2_WS="ws://localhost:8746"
export POTTERHAT_NODE_2_PRIORITY=20

Make sure that these nodes are already running.

  1. Run mix run --no-halt on Potterhat

  2. Try make RPC calls to potterhat, e.g.

curl -X POST http://localhost:8545 \
-H "Content-Type: application/json" \
--data '{
        "jsonrpc":"2.0",
        "method":"web3_clientVersion",
        "params":[],
        "id":1
}' | jq

You should see Ethereum-flavored, JSON-RPC response returned.

  1. Bonus point: Try start/stop some or all of your nodes and see what happens.

Impact

Meh.

@unnawut unnawut self-assigned this Jun 21, 2019
@unnawut
Copy link
Contributor Author

unnawut commented Jun 24, 2019

Remaining:

  • Refactor PotterhatRPC.ErrorResponse to eWallet-style error handler code
  • Error response id should reflect the request id
  • Refactor rpc relay implementation out of PotterhatRPC.Router
  • Add tests to cover various RPC relay cases
  • Add tests to cover various failover cases
  • Add usage remarks e.g. Potterhat does not manage node's states e.g. unlocked accounts on the node, etc.

@unnawut unnawut marked this pull request as ready for review June 26, 2019 19:59
@unnawut unnawut requested review from sirn, InoMurko and T-Dnzt June 26, 2019 19:59
@unnawut unnawut changed the title [WIP] RPC relay RPC relay Jun 26, 2019
README.md Show resolved Hide resolved
@unnawut unnawut mentioned this pull request Jul 2, 2019
README.md Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
apps/potterhat_node/lib/node/node.ex Show resolved Hide resolved
apps/potterhat_node/lib/node/node.ex Outdated Show resolved Hide resolved
apps/potterhat_rpc/lib/potterhat_rpc/application.ex Outdated Show resolved Hide resolved
apps/potterhat_rpc/lib/potterhat_rpc/application.ex Outdated Show resolved Hide resolved
apps/potterhat_rpc/lib/potterhat_rpc/error_handler.ex Outdated Show resolved Hide resolved
apps/potterhat_rpc/lib/potterhat_rpc/eth_forwarder.ex Outdated Show resolved Hide resolved
@sirn
Copy link
Contributor

sirn commented Jul 3, 2019

Also instead of calling ActiveNode.deregister directly, should you not have some sort of function where it calls GenServer.stop(pid) where the process handle :stop and remove itself from the registry? So this is consistent with how :EXIT is handled.

@unnawut
Copy link
Contributor Author

unnawut commented Jul 4, 2019

Todo:

  • Make reconnection not blocking the Node process
  • Figure out how to deregister the node without going through the genserver lifecycle (too slow to deregister via genserver lifecycle)

@unnawut
Copy link
Contributor Author

unnawut commented Jul 4, 2019

@sirn Noting down before I forget after the sleep...

So I tried moving the deregistration to the node's exit process but the problem is that:

  1. Currently I'm relying the most up to date list of nodes by always calling ActiveNodes.first/0 before a request.

  2. To delegate deregistration to node's exit lifecycle means that the request forwarder is unavoidably blocked until the exit lifecycle reaches deregistration. Only then it can make another ActiveNodes.first/0, then the request.

  3. This blocking problem could be solved by querying ActiveNodes.all/0 only once at the beginning. Then reuse the list for all subsequent retries until the list is exhausted. But this means it misses newly resurrected nodes, and may make retries to already unhealthy nodes.

So I can think of 2 ways right now:

a) Keep calling ActiveNodes.first/0 for each retry. But triggers node deregistration before and outside the exit lifecycle (similar to current code). Short blocking, timeout penalty only once per occurence of unhealthy node, but code is not as elegant.

b) Call ActiveNodes.all/0 once and reuse until exhausted. No blocking, elevant code, but potentially more timeout penalties from retrying on unhealthy nodes.

c) Or any better ideas?

I'm currently in favor of a) because of the less false positives in exchange of cleaner code 😢

@unnawut
Copy link
Contributor Author

unnawut commented Jul 9, 2019

☝️ Force-pushed to rebase against latest master

@unnawut unnawut changed the base branch from deploy to master July 9, 2019 17:10
@unnawut
Copy link
Contributor Author

unnawut commented Jul 11, 2019

I'm going with a) in the meantime.

a) Keep calling ActiveNodes.first/0 for each retry. But triggers node deregistration before and outside the exit lifecycle (similar to current code). Short blocking, timeout penalty only once per occurence of unhealthy node, but code is not as elegant.

@unnawut
Copy link
Contributor Author

unnawut commented Jul 11, 2019

Changed my mind to b)

b) Call ActiveNodes.all/0 once and reuse until exhausted. No blocking, elevant code, but potentially more timeout penalties from retrying on unhealthy nodes.

Reasons:

  • Cleaner code
  • The time taken and the chance of having to go through all nodes is minimal
  • Less coupling between modules

@unnawut unnawut requested a review from sirn July 11, 2019 19:44
Copy link
Contributor

@sirn sirn left a comment

Choose a reason for hiding this comment

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

Sorry for late review, but I like solution B better because it's cleaner and most of the time we probably don't want to reuse a resurrected node immediately.

.circleci/config.yml Outdated Show resolved Hide resolved
@unnawut
Copy link
Contributor Author

unnawut commented Jul 24, 2019

For some reason the last commit wasn't signed. So here's a forced push

@unnawut unnawut merged commit 75a09a4 into master Jul 24, 2019
@unnawut unnawut deleted the potterhat-rpc branch July 24, 2019 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Basic client failover RPC relay
4 participants