client/eth/rpc: fix HTTP provider panic

[norwnd]

I was fiddling with Arbitrum L2, Ethereum wallet does seem to work with it nicely (with minor adjustments it syncs with arbitrum Provider, and I was able to transfer some Arb ETH/USDC to/from it without any obvious issues!),

but I run into a nasty panic that crashes dexc when I'm adding arbitrum Provider (it could be Ethereum provider, the importaint thing here is to trigger "HTTP fallback"):

2023-03-24 12:30:38.873 [ERR] CORE[eth][RPC]: Connected to websocket, but headers subscription not supported. Attempting HTTP fallback
2023-03-24 12:30:39.330 [DBG] CORE[eth][RPC]: Connected with 1 of 1 RPC servers
2023-03-24 12:30:39.330 [TRC] CORE[eth][RPC]: handling header refreshes for "arbitrum-one.public.blastapi.io"
2023-03-24 12:30:39.330 [TRC] CORE[eth][RPC]: handling websocket subscriptions for "arbitrum-one.public.blastapi.io"
2023-03-24 12:30:39.348 [WRN] CORE: Error getting balance for wallet eth: pending balance error: balance error: context canceled
panic: runtime error: invalid memory address or nil pointer dereference
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x100487734]

goroutine 2874 [running]:
github.com/ethereum/go-ethereum/rpc.(*ClientSubscription).Unsubscribe(0x10008bae5?)
	/Users/norwnd/go/pkg/mod/github.com/ethereum/go-ethereum@v1.10.25/rpc/subscription.go:261 +0x14
panic({0x1011ab700, 0x102816720})
	/usr/local/go/src/runtime/panic.go:884 +0x213
github.com/ethereum/go-ethereum/rpc.(*ClientSubscription).Err(0xc001b230e6?)
	/Users/norwnd/go/pkg/mod/github.com/ethereum/go-ethereum@v1.10.25/rpc/s:255
decred.org/dcrdex/client/asset/eth.(*provider).subscribeHeaders(0xc000d3de00, {0x101dc1e70, 0xc000a6cc80}, {0x101dbdb68, 0x0}, 0xc00215eea0, {0x101dcdef0, 0xc001efe840})
	/Users/norwnd/dcrdex/client/asset/eth/multirpc.go:301 +0x207
decred.org/dcrdex/client/asset/eth.connectProviders.func2.1()
	/Users/norwnd/dcrdex/client/asset/eth/multirpc.go:529 +0x45
created by decred.org/dcrdex/client/asset/eth.connectProviders.func2
	/Users/norwnd/dcrdex/client/asset/eth/multirpc.go:528 +0x11cc
make: *** [l] Error 2

Comparing interface against nil is tricky, this PR fixes the issue.

[chappjc]

Comparing interface against nil is tricky

So then you understand this is also a bug with github.com/ethereum/go-ethereum.(*Client).EthSubscribe? This is a common pitfall with an API provider. There's nothing tricky about the idea that a non-nil interface wraps a concrete type that is nil. That said, your change is necessary since we don't control that package. (EDIT: and yes we should not touch the other returns when the error is non-nil)

@JoeGruffins ^

[norwnd]

This is a common pitfall with the API provider, not the consumer.

This is what I mean by "tricky", it doesn't matter who (producer/consumer) is using it wrong - the end result is same, hence consumer ideally shouldn't rely on producer not screwing up with this particular golang feature.

[chappjc]

The fix is needed. This change is defensive and that's good.

Yet, an issue should still be filed with go-ethererum. An implementation of an exported API with Func() (v V, err error) that returns err != nil AND v != nil is flawed, or at least it does not represent the intent of the function. It's a common pitfall that the provider should rectify.

[norwnd]

Yet, an issue should still be filed with go-ethererum.

I'll raise it there, doubt they'll bother fixing this any time soon though.

[chappjc]

Express yourself clearly and put up a terse PR and they will.

[chappjc]

Here's what I consider a terse fix upstream.

func (ec *Client) SubscribeNewHead(ctx context.Context, ch chan<- *types.Header) (ethereum.Subscription, error) {
	sub, err := ec.c.EthSubscribe(ctx, ch, "newHeads")
	if err != nil {
		return nil, err // nil interface, not ethereum.Subscription(nil)
	}
	return sub, nil
}

As the consumer we should be ignoring the other returns when error is non-nil, but we were being lazy and using it as a flag in a parent scope. ❌
As the provider, ethclient.Client should generally return zero-values for the other outputs when error is non-nil. ❌