Skip to content

Commit

Permalink
Capture a missed VC error (#2436)
Browse files Browse the repository at this point in the history
## Issue Addressed

Related to #2430, #2394

## Proposed Changes

As per #2430 (comment), ensure that the `ProductionValidatorClient::new` error raises a log and shuts down the VC. Also, I implemened `spawn_ignoring_error`, as per @michaelsproul's suggestion in #2436 (comment).

I got unlucky and CI picked up a [new rustsec vuln](https://rustsec.org/advisories/RUSTSEC-2021-0072). To fix this, I had to update the following crates:

- `tokio`
- `web3`
- `tokio-compat-02`

## Additional Info

NA
  • Loading branch information
paulhauner committed Jul 9, 2021
1 parent 406e392 commit 78e5c0c
Show file tree
Hide file tree
Showing 11 changed files with 367 additions and 543 deletions.
855 changes: 334 additions & 521 deletions Cargo.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ arbitrary-fuzz:
# Runs cargo audit (Audit Cargo.lock files for crates with security vulnerabilities reported to the RustSec Advisory Database)
audit:
cargo install --force cargo-audit
cargo audit
cargo audit --ignore RUSTSEC-2021-0073

# Runs `cargo udeps` to check for unused dependencies
udeps:
Expand Down
4 changes: 2 additions & 2 deletions beacon_node/eth1/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ edition = "2018"
[dev-dependencies]
eth1_test_rig = { path = "../../testing/eth1_test_rig" }
toml = "0.5.6"
web3 = "0.14.0"
web3 = { version = "0.16.0", default-features = false, features = ["http-tls", "signing", "ws-tls-tokio"] }
sloggers = "1.0.1"
environment = { path = "../../lighthouse/environment" }
tokio-compat-02 = "0.1"
tokio-compat-02 = "0.2.0"

[dependencies]
reqwest = { version = "0.11.0", features = ["native-tls-vendored"] }
Expand Down
2 changes: 1 addition & 1 deletion beacon_node/genesis/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ edition = "2018"

[dev-dependencies]
eth1_test_rig = { path = "../../testing/eth1_test_rig" }
tokio-compat-02 = "0.1"
tokio-compat-02 = "0.2.0"
sensitive_url = { path = "../../common/sensitive_url" }

[dependencies]
Expand Down
14 changes: 14 additions & 0 deletions common/task_executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,20 @@ impl TaskExecutor {
}
}

/// A convenience wrapper for `Self::spawn` which ignores a `Result` as long as both `Ok`/`Err`
/// are of type `()`.
///
/// The purpose of this function is to create a compile error if some function which previously
/// returned `()` starts returning something else. Such a case may otherwise result in
/// accidental error suppression.
pub fn spawn_ignoring_error(
&self,
task: impl Future<Output = Result<(), ()>> + Send + 'static,
name: &'static str,
) {
self.spawn(task.map(|_| ()), name)
}

/// Spawn a future on the tokio runtime wrapped in an `exit_future::Exit`. The task is canceled
/// when the corresponding exit_future `Signal` is fired/dropped.
///
Expand Down
2 changes: 1 addition & 1 deletion lcli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,6 @@ lighthouse_version = { path = "../common/lighthouse_version" }
directory = { path = "../common/directory" }
account_utils = { path = "../common/account_utils" }
eth2_wallet = { path = "../crypto/eth2_wallet" }
web3 = "0.14.0"
web3 = { version = "0.16.0", default-features = false, features = ["http-tls", "signing", "ws-tls-tokio"] }
eth1_test_rig = { path = "../testing/eth1_test_rig" }
sensitive_url = { path = "../common/sensitive_url" }
5 changes: 2 additions & 3 deletions lighthouse/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,8 @@ fn run<E: EthSpec>(
if !shutdown_flag {
environment.runtime().spawn(async move {
if let Err(e) = ProductionValidatorClient::new(context, config)
.await?
.start_service()
.await
.and_then(|mut vc| vc.start_service())
{
crit!(log, "Failed to start validator client"; "reason" => e);
// Ignore the error since it always occurs during normal operation when
Expand All @@ -389,7 +389,6 @@ fn run<E: EthSpec>(
.shutdown_sender()
.try_send(ShutdownReason::Failure("Failed to start validator client"));
}
Ok::<(), String>(())
});
} else {
let _ = executor.shutdown_sender().try_send(ShutdownReason::Success(
Expand Down
4 changes: 2 additions & 2 deletions testing/eth1_test_rig/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ edition = "2018"

[dependencies]
tokio = { version = "1.1.0", features = ["time"] }
tokio-compat-02 = "0.1"
web3 = "0.14.0"
tokio-compat-02 = "0.2.0"
web3 = { version = "0.16.0", default-features = false, features = ["http-tls", "signing", "ws-tls-tokio"] }
futures = "0.3.7"
types = { path = "../../consensus/types"}
serde_json = "1.0.58"
Expand Down
2 changes: 2 additions & 0 deletions testing/eth1_test_rig/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ impl DepositContract {
data: encode_eth1_tx_data(&deposit_data).map(Into::into).ok(),
nonce: None,
condition: None,
transaction_type: None,
access_list: None,
};

self.web3
Expand Down
17 changes: 7 additions & 10 deletions validator_client/src/attestation_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use crate::{
validator_store::ValidatorStore,
};
use environment::RuntimeContext;
use futures::future::FutureExt;
use slog::{crit, error, info, trace};
use slot_clock::SlotClock;
use std::collections::HashMap;
Expand Down Expand Up @@ -205,15 +204,13 @@ impl<T: SlotClock + 'static, E: EthSpec> AttestationService<T, E> {
.into_iter()
.for_each(|(committee_index, validator_duties)| {
// Spawn a separate task for each attestation.
self.inner.context.executor.spawn(
self.clone()
.publish_attestations_and_aggregates(
slot,
committee_index,
validator_duties,
aggregate_production_instant,
)
.map(|_| ()),
self.inner.context.executor.spawn_ignoring_error(
self.clone().publish_attestations_and_aggregates(
slot,
committee_index,
validator_duties,
aggregate_production_instant,
),
"attestation publish",
);
});
Expand Down
3 changes: 1 addition & 2 deletions validator_client/src/fork_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use crate::beacon_node_fallback::{BeaconNodeFallback, RequireSynced};
use crate::http_metrics::metrics;
use environment::RuntimeContext;
use eth2::types::StateId;
use futures::future::FutureExt;
use parking_lot::RwLock;
use slog::{debug, trace};
use slog::{error, Logger};
Expand Down Expand Up @@ -142,7 +141,7 @@ impl<T: SlotClock + 'static, E: EthSpec> ForkService<T, E> {
// Run an immediate update before starting the updater service.
context
.executor
.spawn(self.clone().do_update().map(|_| ()), "fork service update");
.spawn_ignoring_error(self.clone().do_update(), "fork service update");

let executor = context.executor.clone();
let log = context.log().clone();
Expand Down

0 comments on commit 78e5c0c

Please sign in to comment.