Various code inspection resolutions

[prestonvanloon]

What type of PR is this?

Other

What does this PR do? Why is it needed?

In the spirit of improving code health, this PR resolves the following:

• Remove unused/dead code
• Remove use of defer within a loop
• Remove unused constants
• Check for nil slice before accessing element
• Remove unused args in methods
• Replace empty slice declarations (prefer var s []stuff over s := []stuff{})
• Rename pointer receivers to be consistent with others in the package
• Remove bootnode query tool for libp2p based bootnodes (obsolete by discv5)
• Remove //test directory (unused)
• Remove old k8s script
• Remove old gcp script

Which issues(s) does this PR fix?

N/A

Other notes for review

Normally I would break these up into smaller PRs, but there are so many that it would bog down our CI pipeline and review process.

[prestonvanloon]

Tests failing... fixing

[prestonvanloon]

Flaky test, passed on second run.

[codecov]

Codecov Report

Merging #7438 into master will increase coverage by 0.09%.
The diff coverage is 89.40%.

@@            Coverage Diff             @@
##           master    #7438      +/-   ##
==========================================
+ Coverage   60.43%   60.52%   +0.09%     
==========================================
  Files         427      427              
  Lines       30654    30351     -303     
==========================================
- Hits        18526    18371     -155     
+ Misses       9100     8983     -117     
+ Partials     3028     2997      -31     

[rkapka]

This is great stuff. My two cents:

Replace empty slice declarations (prefer var s []stuff over s := []stuff{})

GoLand complains that this might change semantics (example). Just making sure that you're aware of this.

Please also take a look at comments from @nisdas in #6864 - some code was purposefully not removed even though it was unused. We better verify whether we still want to keep it.

[prestonvanloon]

@nisdas can you please review? I am not sure which comments to be concerned about from #6864. If there is deadcode that we want to keep around, it must be used or have some comment to keep it. Otherwise it's just code rot, in my opinion.

[terencechain]

I glanced through most and it looked good. Will be good to get another 👀 on this

[terencechain]

@prestonvanloon this is collecting conflicts.

bump @nisdas for the questions with #6864

[prestonvanloon]

Yeah, this is going to get nasty real fast with conflicts.

If there is no concern over deleting unused code by EOD, then we should move forward on this PR

[nisdas]

The PR looks good, although this is technically 'dead' code, capturing block propagation times is pretty useful from a metrics point of view. We just need to hook it up through our gossip validators.
@prestonvanloon

[nisdas]

This is a pretty useful metric, can we continue using it ?
We just need to hook captureSentTimeMetric for blocks received through gossip

[terencechain]

What you are looking for is block_arrival_latency_milliseconds

It's hooked up and used today (captureArrivalTimeMetric): https://github.com/prysmaticlabs/prysm/blob/master/beacon-chain/sync/validate_beacon_blocks.go#L100

[farazdagi]

This PR will rot quickly. So, approving to have these changes merged.
The only important code that might be useful (as pointed out by Nishant) is block propagation metric, which can be restored at a later stage, should we need it.

NB: there were couple of minor issues reported by DS blocking the merging, so pushed an update to them too.