client/eth: Remove non compliant providers. #2102

JoeGruffins · 2023-02-03T08:43:45Z

Check all providers that are not known to be good. Check them on
reconfigure as well. Also use WebSocket with full nodes rather than the
light node.

JoeGruffins · 2023-02-03T08:45:30Z

dex/testing/eth/harness.sh

+ALPHA_HTTP_PORT="38556"
+ALPHA_WS_PORT="38557"


Would like to move this to the full nodes, since that is what would be used irl.

JoeGruffins · 2023-02-03T08:47:10Z

dex/testing/eth/create-node.sh

-	  "${NODE_DIR}/${NAME}.log" C-m
+	  "${NODE_DIR}/geth-ancient --verbosity 5 --vmdebug --http --http.port " \
+	  "${HTTP_PORT} --ws --ws.port ${WS_PORT} --ws.api " \
+	  "eth --allow-insecure-unlock " \


Removed all the modules except eth. This should be all we need. Also "db" was apparently not a module? This was for the light client harness node.

ERROR[02-03|11:23:46.895] Unavailable modules in HTTP API list unavailable=[db] available="[admin debug web3 eth txpool personal clique net les vflux engine]"

Noticed this too: #2013 (comment)
No clue why it was ever there. Good to have it here too.

However, didn't we still want txpool? Is the idea that we want it to use the dumbed down fallback path? Or that beta is just used for server?

EDIT: oh I still have the txpool fallback for server in this draft PR: d942900fe10e61e674261e066c5da039d77193f6
Will get that out of draft as soon as I've reviewed the ETH swap gasses and run swaps for both ETH and USDC2 on mainnet ethereum.

However, didn't we still want txpool? Is the idea that we want it to use the dumbed down fallback path? Or that beta is just used for server?

I guess that server operators will use providers, either by choice or because the eth node can not be kept up %100. I also guess that the txpool type operations will not be available for providers. Maybe we can turn txpool on for one and off for the other? So we can use both for testing.

This is the simnet harness, so I mean if you're testing dexc (client, not server) and want to use the beta node? Would that just be pointless and now expected not to work? I'm fine with that, if that's intended.

This is the simnet harness, so I mean if you're testing dexc (client, not server) and want to use the beta node? Would that just be pointless and now expected not to work?

I didn't think the client needed anything but "eth" and only server needed "txpool" (doesn't need now)

I think client is fine with with either "eth" or "eth,txpool"?

Now alpha is eth and beta is eth,txpool

JoeGruffins · 2023-02-03T09:25:21Z

Hmm, actually seems bugged. Doesn't seem to be checking unknown providers either. Will figure it out here.

Oh, it checks on creations, I guess it also needs to check on reconfigure. Maybe not every start up?

chappjc · 2023-02-06T03:08:51Z

Oh. Sorry about that. You're right

…

On Sun, Feb 5, 2023, 8:39 PM JoeGruffins ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dex/testing/eth/create-node.sh <#2102 (comment)>: > @@ -135,8 +132,10 @@ if [ "${SYNC_MODE}" = "snap" ]; then tmux send-keys -t "$TMUX_WIN_ID" "${NODES_ROOT}/harness-ctl/${NAME} --nodiscover " \ "--config ${NODE_DIR}/eth.conf --unlock ${CHAIN_ADDRESS} " \ "--password ${GROUP_DIR}/password --light.serve 25 --datadir.ancient " \ - "${NODE_DIR}/geth-ancient --verbosity 5 --vmdebug 2>&1 | tee " \ - "${NODE_DIR}/${NAME}.log" C-m + "${NODE_DIR}/geth-ancient --verbosity 5 --vmdebug --http --http.port " \ + "${HTTP_PORT} --ws --ws.port ${WS_PORT} --ws.api " \ + "eth --allow-insecure-unlock " \ This is the simnet harness, so I mean if you're testing dexc (client, not server) and want to use the beta node? Would that just be pointless and now expected not to work? I didn't think the client needed anything but "eth" and only server needed "txpool" (doesn't need now) I think client is fine with with either "eth" or "eth,txpool"? — Reply to this email directly, view it on GitHub <#2102 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACHQOSOAWROTGNZABTMUNWLWWBP57ANCNFSM6AAAAAAUP7W3JU> . You are receiving this because you commented.Message ID: ***@***.***>

JoeGruffins · 2023-02-08T09:36:49Z

client/asset/eth/eth.go


 	if rpc, is := w.node.(*multiRPCClient); is {
-		if err := rpc.reconfigure(ctx, cfg.Settings); err != nil {
+		walletDir := getWalletDir(w.dir, w.net)


The getWalletDir path seems unnecessary. It will end up being something like $HOME/.dexc/simnet/assetdb/eth/simnet and I don't think the last siment is necessary. The keystore folder already lives there though, is it worth changing now?

I've noticed that too. Not sure if we should change it though. However, nobody should be using this on mainnet, and we can move our testnet folders around if we do change it.
Maybe someone else has a more decisive opinion.

This has always bugged me too. On one hand, the asset package shouldn't make assumptions about what the caller is going to give. On the other hand, as you pointed out, it's stupid with the extra directory.

Anyway, won't this change bork existing wallets?

Anyway, won't this change bork existing wallets?

Yes. Existing testnet eth multiprc wallets.

Just leaving for now, can be addressed in another pr if necessary.

client/asset/eth/multirpc.go

JoeGruffins · 2023-02-09T03:13:30Z

Just rebased.

JoeGruffins · 2023-02-09T05:33:58Z

Use per funciton contexts https://github.com/decred/dcrdex/compare/fb3e0e3bf030c7a704417dfd75b3a66aa7f911c8..1fd0935a1ab32e48bb4bfad9a7d5da3123d2526c

norwnd

Minor suggestions, just possible improvements.

norwnd · 2023-02-08T12:23:05Z

client/asset/eth/multirpc.go

+			}
+		}()
+		if len(providers) != len(unknownEndpoints) {
+			return errors.New("Could not connect to all unknown providers.")


Maybe we could point out the exact provider(s) that's not working properly ? Trying to add a new one looks like this now, which seems a bit cryptic to me:

There are logs in the connectProviders function, but yeah, they don't currently make it to the UI.

It's not possible for assets to send UI notifications right? Only core?

Is it easiest to stick the failed providers in that error?

Is it easiest to stick the failed providers in that error?

As a user, by looking at that error in UI I'd want to know which providers I should remove/adjust. Might be easier to "fail fast" and show only one bad provider at a time, so user can repeat adjusting until all supplied configuration is acceptable.

Sticking the failing provider addresses in the error.

client/asset/eth/eth.go

client/asset/eth/multirpc.go

norwnd · 2023-02-13T04:39:08Z

client/asset/eth/multirpc.go

+		if err := os.WriteFile(path, b, 0644); err != nil {
+			log.Errorf("Failed to write compliant providers file: %v", err)
+		}


Maybe return error here instead of just logging it ? So the user can retry saving his configuration (in case it's temporary failure), because he'll have to refill it upon restart ... I can't think of a valid use case to log here instead of erroring right away, thougths ?

He won't need to refill it, the providers are saved somewhere else. This file is only for a list of user added providers we didn't know about, like random full nodes. If it fails writing, it would mean the provider needs to be checked again if unknown if doing reconfigure. So, it only saves a little time and not every start up. I can't imagine why this would error here under normal circumstances.

I misunderstood it then, thanks for clarifying.

client/asset/eth/multirpc.go

client/asset/eth/eth.go

JoeGruffins · 2023-02-17T02:54:09Z

Just rebased.

JoeGruffins · 2023-02-17T08:36:49Z

Put all the failed providers in the error so it makes it to the frontend https://github.com/decred/dcrdex/compare/6ff317b8054100e06e059373e230680ab0979217..eae7c4f018735fd1214eefd814a70655bfc27824

JoeGruffins · 2023-02-21T03:25:34Z

Was a hairy rebase.

JoeGruffins · 2023-02-21T03:34:23Z

Removed unneeded timed context. Both connectProviders and checkProvidersCompliance will use their own timed contexts https://github.com/decred/dcrdex/compare/15c6d1d974d643cb55d2652000417febcdbde14d..5bd1190a3b1b9471ad0474d3bb6895e4627a3acd

chappjc · 2023-02-21T14:52:37Z

Removed unneeded timed context. Both connectProviders and checkProvidersCompliance will use their own timed contexts https://github.com/decred/dcrdex/compare/15c6d1d974d643cb55d2652000417febcdbde14d..5bd1190a3b1b9471ad0474d3bb6895e4627a3acd

But you're calling with context.Background() in createWallet:

dcrdex/client/asset/eth/eth.go

Line 609 in 5bd1190

if err := createAndCheckProviders(context.Background(), walletDir, endpoints,

Probably OK, but it's not timed on that path.

EDIT: Oh, you mean connectProviders has it's own timed context... alright.

chappjc · 2023-02-21T22:34:20Z

git rm client/asset/eth/cmd/getgas/getgas pls

Check all providers that are not known to be good. Check them on reconfigure as well. Also use WebSocket with full nodes rather than the light node.

chappjc · 2023-02-21T23:00:30Z

Merged, but can you clarify your comment about light node for me?

JoeGruffins · 2023-02-22T00:46:16Z

git rm client/asset/eth/cmd/getgas/getgas pls

Ah srry, this is like twice now. I guess you removed it.

JoeGruffins mentioned this pull request Feb 3, 2023

client/eth: Check provider header times. #2074

Merged

JoeGruffins commented Feb 3, 2023

View reviewed changes

chappjc added this to the 0.6 milestone Feb 6, 2023

JoeGruffins force-pushed the multirpcchanges branch 2 times, most recently from 8d1cbe9 to af2eebe Compare February 8, 2023 09:32

JoeGruffins commented Feb 8, 2023

View reviewed changes

JoeGruffins force-pushed the multirpcchanges branch from af2eebe to 0856e0b Compare February 8, 2023 09:49

JoeGruffins changed the title ~~client/eth: Check non compliant providers.~~ client/eth: Remove non compliant providers. Feb 8, 2023

JoeGruffins marked this pull request as ready for review February 8, 2023 09:52

JoeGruffins commented Feb 8, 2023

View reviewed changes

client/asset/eth/multirpc.go Outdated Show resolved Hide resolved

chappjc reviewed Feb 8, 2023

View reviewed changes

client/asset/eth/multirpc.go Outdated Show resolved Hide resolved

buck54321 reviewed Feb 8, 2023

View reviewed changes

client/asset/eth/multirpc.go Outdated Show resolved Hide resolved

client/asset/eth/multirpc.go Outdated Show resolved Hide resolved

JoeGruffins force-pushed the multirpcchanges branch from 0856e0b to fb3e0e3 Compare February 9, 2023 03:13

JoeGruffins force-pushed the multirpcchanges branch from fb3e0e3 to 1fd0935 Compare February 9, 2023 05:33

norwnd reviewed Feb 13, 2023

View reviewed changes

chappjc self-requested a review February 14, 2023 13:26

buck54321 reviewed Feb 16, 2023

View reviewed changes

client/asset/eth/multirpc.go Outdated Show resolved Hide resolved

chappjc reviewed Feb 16, 2023

View reviewed changes

client/asset/eth/eth.go Outdated Show resolved Hide resolved

JoeGruffins force-pushed the multirpcchanges branch from 1fd0935 to 6ff317b Compare February 17, 2023 02:53

JoeGruffins force-pushed the multirpcchanges branch from 6ff317b to eae7c4f Compare February 17, 2023 08:34

norwnd approved these changes Feb 17, 2023

View reviewed changes

chappjc added the ETH label Feb 20, 2023

JoeGruffins force-pushed the multirpcchanges branch from eae7c4f to 15c6d1d Compare February 21, 2023 03:25

JoeGruffins force-pushed the multirpcchanges branch from 15c6d1d to 5bd1190 Compare February 21, 2023 03:31

client/eth: Remove non compliant providers.

66013a0

Check all providers that are not known to be good. Check them on reconfigure as well. Also use WebSocket with full nodes rather than the light node.

chappjc force-pushed the multirpcchanges branch from 5bd1190 to 66013a0 Compare February 21, 2023 22:53

chappjc approved these changes Feb 21, 2023

View reviewed changes

chappjc merged commit 68b8623 into decred:master Feb 21, 2023

buck54321 mentioned this pull request Feb 22, 2023

client/asset/eth: less provider check spam #2159

Merged

		ALPHA_HTTP_PORT="38556"
		ALPHA_WS_PORT="38557"

client/eth: Remove non compliant providers. #2102

client/eth: Remove non compliant providers. #2102

Uh oh!

Conversation

JoeGruffins commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chappjc Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chappjc Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeGruffins commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chappjc commented Feb 6, 2023 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buck54321 Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoeGruffins commented Feb 9, 2023

Uh oh!

JoeGruffins commented Feb 9, 2023

Uh oh!

norwnd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeGruffins Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

norwnd Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeGruffins Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoeGruffins commented Feb 17, 2023

JoeGruffins commented Feb 3, 2023 •

edited

Loading

chappjc Feb 3, 2023 •

edited

Loading

chappjc Feb 6, 2023 •

edited

Loading

JoeGruffins commented Feb 3, 2023 •

edited

Loading

buck54321 Feb 8, 2023 •

edited

Loading

JoeGruffins Feb 17, 2023 •

edited

Loading

norwnd Feb 17, 2023 •

edited

Loading

JoeGruffins Feb 17, 2023 •

edited

Loading

chappjc commented Feb 21, 2023 •

edited

Loading

chappjc commented Feb 21, 2023 •

edited

Loading