[Merged by Bors] - Fix Rust 1.61 clippy lints

[michaelsproul]

Issue Addressed

This fixes the low-hanging Clippy lints introduced in Rust 1.61 (due any hour now). It ignores one lint, because fixing it requires a structural refactor of the validator client that needs to be done delicately. I've started on that refactor and will create another PR that can be reviewed in more depth in the coming days. I think we should merge this PR in the meantime to unblock CI.

[realbigsean]

Looks good! I had a comment on whether we could get around swapping out the lock in the simulator but since it's just the simulator don't think it's a big deal.

In reference to the currently ignored lint: I've read async locks are generally less performant, so I'm wondering if rather than swapping the lock on validator_store.initialized_validators out for an async one, we should instead remove that lock and add one to validator_store.initalized_validators.definitions and validator_store.initialized_validators.validators

Haven't checked it out in depth but I think we could get around holdintg it across an await if we do that

[michaelsproul]

I'll have to dig into the VC locks some more. I think avoiding the async mutex if we can would be nice, although I'm worried there are other sketchy things we're doing that aren't yet being caught by Clippy (just as this issue wasn't caught until 1.61).

When I started the refactor I noticed a lot of other places where we hold a lock across a block_on, which evaluates a future containing awaits:

• Lock:

  lighthouse/validator_client/src/http_api/keystores.rs

  Line 222 in 54b58fd

  let mut initialized_validators = initialized_validators_rwlock.write();

• block_on followed by await:

  lighthouse/validator_client/src/http_api/keystores.rs

  Line 252 in 54b58fd

  .block_on(initialized_validators.update_validators())

Initially I thought these were false negatives for Clippy, but now I think they may actually be fine. As per this issue, Rust is smart enough to mark futures holding locks as !Send, and curiously, unlike spawn, block_on does not require its future to be Send. I think this is because it drives the future to completion on the current thread, and not on a thread from the Tokio async pool. This means there's no risk of a block_on task being paused (when it awaits) and picked up by another thread which doesn't hold the lock. So far so good: that's a static guarantee checked by the type system!

The other way to get a deadlock is described by this comment on that same Rust issue: when the thread holding the lock is re-purposed to run another task/future which tries to obtain the lock. According to std::mutex::Mutex this attempt to obtain the lock from the thread that already holds it will panic (or deadlock?). I think we're safe from this however due to block_on running the task on a dedicated thread that exists outside the Tokio runtime: this thread won't be re-purposed to run any other futures.

[paulhauner]

LGTM!

bors r+

[bors]

Build failed (retrying...):

• cargo-udeps

[bors]

Build failed (retrying...):

• cargo-udeps

[michaelsproul]

bors r-

gotta fix udeps

[bors]

Canceled.

[michaelsproul]

bors r+

[bors]

Pull request successfully merged into unstable.

Build succeeded:

• arbitrary-check
• beacon-chain-tests
• cargo-audit
• cargo-fmt
• cargo-udeps
• cargo-vendor
• check-benchmarks
• check-consensus
• check-msrv
• clippy
• debug-tests-ubuntu
• dockerfile-ubuntu
• doppelganger-protection-test
• ef-tests-ubuntu
• eth1-simulator-ubuntu
• execution-engine-integration-ubuntu
• no-eth1-simulator-ubuntu
• op-pool-tests
• release-tests-ubuntu
• release-tests-windows
• state-transition-vectors-ubuntu