all: more linters

[holiman]

This PR enables the following linters

• typecheck
• unused
• staticcheck
• bidichk
• durationcheck
• exportloopref
• gosec

WIth a few exceptions.

• We use a deprecated protobuf in trezor. I didn't want to mess with that, since I cannot meaningfully test any changes there.
• The deprecated TypeMux is used quite a lot. Not sure why it's not been changed to Feed, which supposedly is the new hotness, but I'm sure @fjl has a plan already
• Using string type in context.WithValue is apparently wrong, one should use a custom type, to prevent collisions between different places in the hierarchy of callers. That should be fixed at some point, but may require some attention.
• The warnings for using weak random generator are squashed, since we use a lot of random without need for cryptographic guarantees.

[holiman]

cc @fjl?

[fjl]

Where do we get this warning? I thought I had already removed all context string keys.

[holiman]

It's mainly how I use it to shove some key-values for the clef pipeline

[fjl]

Ahh, I found it. You need to fix this instead of disabling the warning.

[holiman]

Yes, I know

That should be fixed at some point, but may require some attention.

I didn't include it in this PR, because it's so large already. For things which require a bit more effort I thought it better to postpone a bit. When changing that, I need to check that it doesn't break the places where we pick those keys out downstream

[holiman]

Ok fixing the context can be done here

diff --git a/cmd/clef/main.go b/cmd/clef/main.go
index f7c3adebc4..de528982c7 100644
--- a/cmd/clef/main.go
+++ b/cmd/clef/main.go
@@ -819,9 +819,9 @@ func confirm(text string) bool {
 
 func testExternalUI(api *core.SignerAPI) {
 
-	ctx := context.WithValue(context.Background(), "remote", "clef binary")
-	ctx = context.WithValue(ctx, "scheme", "in-proc")
-	ctx = context.WithValue(ctx, "local", "main")
+	connInfo := rpc.PeerInfo{Transport: "in-proc", RemoteAddr: "localhost"}
+	ctx = context.WithValue(context.Background(), peerInfoContextKey{}, connInfo)
+
 	errs := make([]string, 0)
 
 	a := common.HexToAddress("0xdeadbeef000000000000000000000000deadbeef")
@@ -952,7 +952,11 @@ func testExternalUI(api *core.SignerAPI) {
 	{ // Metadata
 		api.UI.ShowInfo("Please check if you see the Origin in next listing (approve or deny)")
 		time.Sleep(delay)
-		api.List(context.WithValue(ctx, "Origin", "origin.com"))
+
+		connInfo := rpc.PeerInfo{Transport: "http"}
+		connInfo.HTTP.Origin = "origin.com"
+
+		api.List(context.WithValue(ctx, peerInfoContextKey{}, connInfo))
 		expectResponse("metadata - origin", "Did you see origin (origin.com)? [yes/no] ", "yes")
 	}
 

However, there are also a couple of these

			ctx = context.WithValue(ctx, "node", node)

in p2p/simulations/http.go

[fjl]

Damn, I did not consider unit testing when adding rpc.PeerInfo. I guess we need to export func WithPeerInfo(context.Context, PeerInfo) context.Context in package rpc.

[fjl]

I now think you are right, we should disable the warning for now and deal with it in another PR.

[holiman]

I now think you are right, we should disable the warning for now and deal with it in another PR.

Then we should be good to go with this PR

[MariusVanDerWijden]

LGTM

It does remove a lot of unused code within LES though (that might be used by @zsfelfoldi metering/auctioning bandwith prs) so better to take a look there @zsfelfoldi

[karalabe]

Wrt the unused stuff, it's a "regression" from a refactor from last year https://github.com/ethereum/go-ethereum/pull/22163/files. I guess @rjl493456442 wantd to split my mega test method into more digestible chunks or variations and some were not necessary at the end.

I'm all for double checking and getting rid of all dead code there, just let's make sure nothing was lost in translation.

[holiman]

@rjl493456442 this comes from https://github.com/ethereum/go-ethereum/pull/22163/files#diff-3ecc1958cb0ed978a0edcf6c43089b130034057420e4ce2e1e0c447cd7cec454R968 -- it's not used, it was originally used in TestRecoverSnapshotFromCrashWithLegacyDiffJournal (which was Skipped) but not is no longer in the codebase. It seems to have been removed here: d6ffa14.

So I'll just remove these structs then.. ?

[rjl493456442]

It's ok to remove

[zsfelfoldi]

The LES related parts LGTM.

[karalabe]

Wut?

[karalabe]

what is unused?

[holiman]

clearBloomBits.
It's used within a test which is Skipped, so technically it's not totally unused, but as far as the linter is concerned, it's not used.

[karalabe]

Wasn't this meant to mean that "we don't know this tx, so it's not an error, simply return nothing"?

[holiman]

Makes sense, ok