feat: support multiple ethereum rpcs #215

toteki · 2022-02-24T18:38:13Z

struct EthRPCManager supports multiple ethereum RPC endpoints and attempts to rotate through them when one fails

closes #196

toteki · 2022-02-26T23:12:11Z

All functions requiring rpc.Client or ethclient.Client have been wrapped by EthRPCManager, except the following:

cmd/bridge.go L171 in deployGravityCmd

address, tx, _, err := wrappers.DeployGravity(auth, ethRPC, gravityIDBytes32, validators, powers)

cmd/bridge.go L546 in getGravityContract

contract, err := wrappers.NewGravity(ethcmn.HexToAddress(gravityAddr), ethRPC)

cmd/bridge.go L560 in approveERC20

contract, err := wrappers.NewERC20(ethcmn.HexToAddress(erc20AddrStr), ethRPC)

cmd/orchestrator.go L126 in getOrchestratorCmd

ethProvider := provider.NewEVMProvider(ethRPC)

Those functions require structs of the types rpc.Client or ethclient.Client (both structures, not interfaces) to be passed in, so it will be more difficult to wrap them if we want to automatically redial on any rpc errors that occur after the initial client is passed in.

If the commands in bridge.go run instantly and exit on error, then they don't need redial-after-error and the EVMProviderWithRet in orchestrator.go is the only remaining issue. The provider is also used to create other structures like EthCommitter, so it would be difficult to wrap everything.

toteki · 2022-02-26T23:26:29Z

A note: The current behavior of EthRPCManager is as follows:

Stores the following:

type EthRPCManager struct {
	currentEndpoint int // index (in the slice of configured RPC endpoints) of most recent endpoint used
	client          *rpc.Client
	konfig          *koanf.Koanf
}

Has the following methods which return the current client if non-nil, otherwise attempting to dial each configured eth rpc endpoint in a roundrobin fashion, returning the first successfully dialed and only trying each endpoint a maximum of once per call.

// returns the current eth RPC client, dialing one first if nonexistent
func (em *EthRPCManager) GetClient() (*rpc.Client, error)

// returns the current eth RPC client, dialing one first if nonexistent
func (em *EthRPCManager) GetEthClient() (*ethclient.Client, error)

Additionally has the following methods, which call the same-name method on the current eth client, dialing one first if nil, and closing the current eth client if the wrapped function returns an error.

// wraps ethclient.PendingNonceAt, also closing client if PendingNonceAt returns an error
func (em *EthRPCManager) PendingNonceAt(ctx context.Context, addr common.Address) (uint64, error)

// wraps ethclient.ChainID, also closing client if ChainID returns an error
func (em *EthRPCManager) ChainID(ctx context.Context) (*big.Int, error)

// wraps ethclient.SuggestGasPrice, also closing client if SuggestGasPrice returns an error
func (em *EthRPCManager) SuggestGasPrice(ctx context.Context) (*big.Int, error)

toteki · 2022-02-26T23:30:54Z

Remaining things we might want to do:

difficult: figure out how to wrap EVMProviderWithRet and similar functions spawned in orchestrator.go
very optional: modify PendingNonceAt, ChainID, and SuggestGasPrice to retry once with a new client before returning upon receiving an error, thus increasing their reliability.

facundomedica · 2022-02-28T13:54:51Z

I like this :)
nit: cmd/peggo/grpcclient.go I think grpcclient is a typo, right? Should be something more like ethrpc_manager or any other name (I'm terrible at naming)

…to adam/multiple-rpcs

alexanderbez · 2022-02-28T15:01:56Z

@toteki excellent summary and I like the approach.

Regarding to the remaining (optional?) items:

difficult: figure out how to wrap EVMProviderWithRet and similar functions spawned in orchestrator.go

Can you elaborate a bit more on this? Which functions in particular? You mean calls to provider.* package? If so, I don't think there's much we can do about that since we can't control what that function accepts, unless we want to wrap all the provider.*` calls, which may or may not be overkill.

Thoughts @facundomedica ?

I'll be reviewing this PR shortly :)

alexanderbez · 2022-02-28T15:03:04Z

CHANGELOG.md

@@ -43,11 +43,19 @@ Ref: https://keepachangelog.com/en/1.0.0/

 ## [Unreleased]

+### Features
+
+- [#215](https://github.com/umee-network/peggo/pull/215) Add the flag `--eth-rpcs` and support multiple ethereum rpc endpoints


Suggested change

- [#215](https://github.com/umee-network/peggo/pull/215) Add the flag `--eth-rpcs` and support multiple ethereum rpc endpoints

- [#215](https://github.com/umee-network/peggo/pull/215) Add the flag `--eth-rpcs` and support multiple ethereum rpc endpoints.

alexanderbez · 2022-02-28T15:08:07Z

cmd/peggo/orchestrator.go

@@ -123,13 +122,12 @@ func getOrchestratorCmd() *cobra.Command {
 				return fmt.Errorf("failed to initialize Ethereum account: %w", err)
 			}

-			ethRPCEndpoint := konfig.String(flagEthRPC)
-			ethRPC, err := ethrpc.Dial(ethRPCEndpoint)
+			em := NewEthRPCManager(konfig)


I see what you mean @toteki. I think the biggest win for peggo would be to have ETH RPC rotation at the orchestrator level, but it seems in order to do that, we'd have to do way too much wrapping :-/

alexanderbez · 2022-02-28T15:08:54Z

cmd/peggo/ethrpc_manager.go

+	"github.com/pkg/errors"
+)
+
+type EthRPCManager struct {


nit: add a godoc :)

alexanderbez · 2022-02-28T15:09:25Z

cmd/peggo/ethrpc_manager.go

+}
+
+// creates an instance of EthRPCManager with a given konfig.
+func NewEthRPCManager(konfig *koanf.Koanf) *EthRPCManager {


Would it be possible for there to be concurrent use of an EthRPCManager instance?

Use of the embedded clients themselves should be concurrency-safe, but concurrent redialing / closing would need to be worked on - for example if one goroutine closed the client (perhaps as a result of an error in a grpc call) while another was doing something, the latter could geta very unexpected "connection was closed" type of error.

Or if two concurrent redialing loops were active, a whole lot could happen. It should be possible to add a lock on DialNext() in particular before merging if we move forward with this.

alexanderbez · 2022-02-28T15:10:30Z

cmd/peggo/ethrpc_manager.go

+			em.client = cli
+			return true
+		}
+		fmt.Fprintf(os.Stderr, "Failed to dial to Ethereum RPC: %s\n", rpcs[i])


We should probably pass the core logger to NewEthRPCManager instead of using fmt.*.

alexanderbez · 2022-02-28T15:11:35Z

cmd/peggo/ethrpc_manager.go

+		return false
+	}
+
+	// first tries all endpoints in the slice after the current index


We could probably combine both these loops into one using the modulo operator.

facundomedica · 2022-02-28T15:37:13Z

Just started looking deeper into this one. I'd consider this an "all or nothing" situation given that if just a single endpoint isn't covered, then we'll be back at where we started.

For example see this one: https://github.com/umee-network/peggo/blob/main/orchestrator/eth_event_watcher_test.go#L185-L192

If any of those endpoints are not covered by this, then sadly the whole effort was in vain.
I think we either wrap all these functions or add an external helper (like those nginx proxies people were sharing).

type EVMProvider interface {
	bind.ContractCaller // <- Must remove these and add their functions here too (probably)
	bind.ContractFilterer

	PendingNonceAt(ctx context.Context, account ethcmn.Address) (uint64, error)
	PendingCodeAt(ctx context.Context, account ethcmn.Address) ([]byte, error)
	EstimateGas(ctx context.Context, msg ethereum.CallMsg) (uint64, error)
	SuggestGasPrice(ctx context.Context) (*big.Int, error)
	TransactionByHash(ctx context.Context, hash ethcmn.Hash) (tx *types.Transaction, isPending bool, err error)
	TransactionReceipt(ctx context.Context, txHash ethcmn.Hash) (*types.Receipt, error)
	SendTransaction(ctx context.Context, tx *types.Transaction) error
	HeaderByNumber(ctx context.Context, number *big.Int) (*types.Header, error)
	SuggestGasTipCap(ctx context.Context) (*big.Int, error)
}

We could even change EVMProvider/EVMProviderWithRet to be EthRPCManager?

Now, is it worth it? I'm not sure...

it gives us the best plug&play experience ever but in exchange we have a lot of work to do
I'm also very tempted to just provide some help with an Nginx config, which looks extremely simple (AFAIK it will handle health checks and all that stuff)

EDIT: I feel like I gave this text a harsh tone, so want to clarify that I love what you did here @toteki and it was quite eye-opening to see how hard it can be to add something that said in conversation sounds easy to do ("just add a backup lol").
I missed the "how to be polite in writing" English class, so imagine me saying this while smiling 😅

alexanderbez · 2022-02-28T15:56:26Z

I do agree that if we don't have RPC rotation at the orchestrator level, we don't get much of a win.

@facundomedica are you suggesting we just scrap this in favor of providing some general nginx configuration to peggo operators?

toteki · 2022-02-28T16:32:24Z

If nginx can do it generally without us having to go into the provider package, then I agree that would be a more effective solution then reinventing it here

alexanderbez · 2022-02-28T17:02:53Z

@facundomedica can you share what you have in mind re nginx?

facundomedica · 2022-02-28T19:49:06Z

A valoper shared this in Discord: https://github.com/stakingcare/node-tools/blob/main/eth-api-failover/default
Haven't tried it, but looks pretty neat

toteki · 2022-03-17T03:55:32Z

Closing for now

toteki added 9 commits February 24, 2022 11:35

first add flag without deprecating

dd43a41

lint

749bf6d

++

d123a1b

add back grpcclient.go

c196f12

switch orchestrator to ethManager

059e8d9

e2e fixed locally

94c2ba6

switch remaining commands to ethManager

bc97243

errors, comments, typo, and readme

f8d7e7a

wrapper functions using ethManager

f1499cb

toteki mentioned this pull request Feb 26, 2022

feat: support multiple ethereum grpc endpoints #206

Closed

comment++

4c63561

toteki marked this pull request as ready for review February 27, 2022 00:04

toteki requested review from alexanderbez and facundomedica as code owners February 27, 2022 00:04

Update CHANGELOG.md

a0d7bea

toteki added 3 commits February 28, 2022 05:56

rename file

e130f39

Merge branch 'adam/multiple-rpcs' of github.com:umee-network/peggo in…

9bb177e

…to adam/multiple-rpcs

rename file

0a51103

alexanderbez reviewed Feb 28, 2022

View reviewed changes

toteki closed this Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support multiple ethereum rpcs #215

feat: support multiple ethereum rpcs #215

toteki commented Feb 24, 2022 •

edited

Loading

toteki commented Feb 26, 2022 •

edited

Loading

toteki commented Feb 26, 2022 •

edited

Loading

toteki commented Feb 26, 2022

facundomedica commented Feb 28, 2022

alexanderbez commented Feb 28, 2022 •

edited

Loading

alexanderbez Feb 28, 2022

alexanderbez Feb 28, 2022

alexanderbez Feb 28, 2022

alexanderbez Feb 28, 2022

toteki Feb 28, 2022

alexanderbez Feb 28, 2022

alexanderbez Feb 28, 2022

facundomedica commented Feb 28, 2022 •

edited

Loading

alexanderbez commented Feb 28, 2022

toteki commented Feb 28, 2022

alexanderbez commented Feb 28, 2022

facundomedica commented Feb 28, 2022

toteki commented Mar 17, 2022

	- [#215](https://github.com/umee-network/peggo/pull/215) Add the flag `--eth-rpcs` and support multiple ethereum rpc endpoints
	- [#215](https://github.com/umee-network/peggo/pull/215) Add the flag `--eth-rpcs` and support multiple ethereum rpc endpoints.

feat: support multiple ethereum rpcs #215

feat: support multiple ethereum rpcs #215

Conversation

toteki commented Feb 24, 2022 • edited Loading

toteki commented Feb 26, 2022 • edited Loading

toteki commented Feb 26, 2022 • edited Loading

toteki commented Feb 26, 2022

facundomedica commented Feb 28, 2022

alexanderbez commented Feb 28, 2022 • edited Loading

alexanderbez Feb 28, 2022

Choose a reason for hiding this comment

alexanderbez Feb 28, 2022

Choose a reason for hiding this comment

alexanderbez Feb 28, 2022

Choose a reason for hiding this comment

alexanderbez Feb 28, 2022

Choose a reason for hiding this comment

toteki Feb 28, 2022

Choose a reason for hiding this comment

alexanderbez Feb 28, 2022

Choose a reason for hiding this comment

alexanderbez Feb 28, 2022

Choose a reason for hiding this comment

facundomedica commented Feb 28, 2022 • edited Loading

alexanderbez commented Feb 28, 2022

toteki commented Feb 28, 2022

alexanderbez commented Feb 28, 2022

facundomedica commented Feb 28, 2022

toteki commented Mar 17, 2022

toteki commented Feb 24, 2022 •

edited

Loading

toteki commented Feb 26, 2022 •

edited

Loading

toteki commented Feb 26, 2022 •

edited

Loading

alexanderbez commented Feb 28, 2022 •

edited

Loading

facundomedica commented Feb 28, 2022 •

edited

Loading