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

add multi-rpc client for server #2104

Merged
merged 3 commits into from Feb 6, 2023
Merged

Conversation

buck54321
Copy link
Member

No description provided.

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

utACK, a few minor things.

I'm going to rebase #2013 on this PR and test today.

dex/testing/dcrdex/harness.sh Outdated Show resolved Hide resolved
server/asset/eth/eth.go Show resolved Hide resolved
server/asset/eth/rpcclient.go Outdated Show resolved Hide resolved
server/asset/eth/rpcclient.go Show resolved Hide resolved
server/asset/eth/rpcclient.go Outdated Show resolved Hide resolved
server/asset/eth/rpcclient.go Outdated Show resolved Hide resolved
server/asset/eth/rpcclient.go Outdated Show resolved Hide resolved
@buck54321
Copy link
Member Author

I addressed your first review, @chappjc. One thing I'm still thinking about doing is advancing the endpoint index if a header appears outdated.

@chappjc
Copy link
Member

chappjc commented Feb 5, 2023 via email

Comment on lines 174 to 179
bn, err := ec.BlockNumber(ctx)
if err != nil {
return err
}
hdr, err = ec.HeaderByNumber(ctx, big.NewInt(int64(bn)))
return err
Copy link
Member

Choose a reason for hiding this comment

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

Doing some testing with Blast (blastapi.io) and I got a method not found, but I can't tell what method:

[ERR] ASSET[eth][RPC]: Unpropagated error from "wss://eth-mainnet.blastapi.io/xxxxx-xyyyy-adsfasdf": Method not found
[INF] ASSET[eth][RPC]: Switching RPC endpoint to "ws://127.0.0.1:8546"

On startup it did the balance method check and as expected it went the dumb way:

[DBG] ASSET[eth]: Parsed 2 endpoints from the ETH config file                                                                                                                      
[WRN] ASSET[eth][RPC]: Error checking required modules at "wss://eth-mainnet.blastapi.io/xxxxx-xyyyy-adsfasdf": unable to check supported modules: Method not found
[WRN] ASSET[eth][RPC]: Will not account for pending transactions in balance calculations at "wss://eth-mainnet.blastapi.io/xxxxx-xyyyy-adsfasdf"                   
[DBG] ASSET[eth][RPC]: API endpoints supported by ws://127.0.0.1:8546: admin:1.0 engine:1.0 eth:1.0 net:1.0 rpc:1.0 txpool:1.0          

So possibly there's another method missing and blast isn't gonna work, or the smart/dumb switch didn't work as expected. I don't think it was checking a balance at the time, so it's probably another method sadly.

Copy link
Member Author

@buck54321 buck54321 Feb 5, 2023

Choose a reason for hiding this comment

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

You could add blastapi to your ~/dextest/providers.json and run go test -tags rpclive,lgpl -run TestMainnetCompliance in client/asset/eth to see if anything pops out. I've seen "Method not found" for SuggestGasTipCap before. Not sure how that aligns with CheckAPIModules.

Copy link
Member

Choose a reason for hiding this comment

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

This turned out to be the eth_syncing method, which is expected (with 06345a5). Given this provider switching that happens as a result, I think I'll update that commit in #2013 to just not even try eth_syncing since it's next to useless anyway.

Copy link
Member

Choose a reason for hiding this comment

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

#2013 revised with 168cb57
No more eth_syncing on client either.

@chappjc
Copy link
Member

chappjc commented Feb 5, 2023

I addressed your first review, @chappjc. One thing I'm still thinking about doing is advancing the endpoint index if a header appears outdated.

That seems like a good idea. Also, on startup when it checks each endpoint it could compare the best header times (even hash and height) to be sure one doesn't look dead or stalled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants