Web3Signer support for VC (#2522)

[EIP-3030]: https://eips.ethereum.org/EIPS/eip-3030
[Web3Signer]: https://consensys.github.io/web3signer/web3signer-eth2.html

## Issue Addressed

Resolves #2498

## Proposed Changes

Allows the VC to call out to a [Web3Signer] remote signer to obtain signatures.


## Additional Info

### Making Signing Functions `async`

To allow remote signing, I needed to make all the signing functions `async`. This caused a bit of noise where I had to convert iterators into `for` loops.

In `duties_service.rs` there was a particularly tricky case where we couldn't hold a write-lock across an `await`, so I had to first take a read-lock, then grab a write-lock.

### Move Signing from Core Executor

Whilst implementing this feature, I noticed that we signing was happening on the core tokio executor. I suspect this was causing the executor to temporarily lock and occasionally trigger some HTTP timeouts (and potentially SQL pool timeouts, but I can't verify this). Since moving all signing into blocking tokio tasks, I noticed a distinct drop in the "atttestations_http_get" metric on a Prater node:

![http_get_times](https://user-images.githubusercontent.com/6660660/132143737-82fd3836-2e7e-445b-a143-cb347783baad.png)

I think this graph indicates that freeing the core executor allows the VC to operate more smoothly.

### Refactor TaskExecutor

I noticed that the `TaskExecutor::spawn_blocking_handle` function would fail to spawn tasks if it were unable to obtain handles to some metrics (this can happen if the same metric is defined twice). It seemed that a more sensible approach would be to keep spawning tasks, but without metrics. To that end, I refactored the function so that it would still function without metrics. There are no other changes made.

## TODO

- [x] Restructure to support multiple signing methods.
- [x] Add calls to remote signer from VC.
- [x] Documentation
- [x] Test all endpoints
- [x] Test HTTPS certificate
- [x] Allow adding remote signer validators via the API
- [x] Add Altair support via [21.8.1-rc1](https://github.com/ConsenSys/web3signer/releases/tag/21.8.1-rc1)
- [x] Create issue to start using latest version of web3signer. (See #2570)

## Notes

- ~~Web3Signer doesn't yet support the Altair fork for Prater. See https://github.com/ConsenSys/web3signer/issues/423.~~
- ~~There is not yet a release of Web3Signer which supports Altair blocks. See https://github.com/ConsenSys/web3signer/issues/391.~~
This commit is contained in:
Paul Hauner
2021-09-16 03:26:33 +00:00
parent 58012f85e1
commit c5c7476518
37 changed files with 2236 additions and 478 deletions

View File

@@ -16,6 +16,7 @@ use crate::{
};
use environment::RuntimeContext;
use eth2::types::{AttesterData, BeaconCommitteeSubscription, ProposerData, StateId, ValidatorId};
use futures::future::join_all;
use parking_lot::RwLock;
use safe_arith::ArithError;
use slog::{debug, error, info, warn, Logger};
@@ -64,13 +65,14 @@ pub struct DutyAndProof {
impl DutyAndProof {
/// Instantiate `Self`, computing the selection proof as well.
pub fn new<T: SlotClock + 'static, E: EthSpec>(
pub async fn new<T: SlotClock + 'static, E: EthSpec>(
duty: AttesterData,
validator_store: &ValidatorStore<T, E>,
spec: &ChainSpec,
) -> Result<Self, Error> {
let selection_proof = validator_store
.produce_selection_proof(duty.pubkey, duty.slot)
.await
.map_err(Error::FailedToProduceSelectionProof)?;
let selection_proof = selection_proof
@@ -637,56 +639,77 @@ async fn poll_beacon_attesters_for_epoch<T: SlotClock + 'static, E: EthSpec>(
let dependent_root = response.dependent_root;
let relevant_duties = response
.data
.into_iter()
.filter(|attester_duty| local_pubkeys.contains(&attester_duty.pubkey))
.collect::<Vec<_>>();
// Filter any duties that are not relevant or already known.
let new_duties = {
// Avoid holding the read-lock for any longer than required.
let attesters = duties_service.attesters.read();
response
.data
.into_iter()
.filter(|duty| local_pubkeys.contains(&duty.pubkey))
.filter(|duty| {
// Only update the duties if either is true:
//
// - There were no known duties for this epoch.
// - The dependent root has changed, signalling a re-org.
attesters.get(&duty.pubkey).map_or(true, |duties| {
duties
.get(&epoch)
.map_or(true, |(prior, _)| *prior != dependent_root)
})
})
.collect::<Vec<_>>()
};
debug!(
log,
"Downloaded attester duties";
"dependent_root" => %dependent_root,
"num_relevant_duties" => relevant_duties.len(),
"num_new_duties" => new_duties.len(),
);
// Produce the `DutyAndProof` messages in parallel.
let duty_and_proof_results = join_all(new_duties.into_iter().map(|duty| {
DutyAndProof::new(duty, &duties_service.validator_store, &duties_service.spec)
}))
.await;
// Update the duties service with the new `DutyAndProof` messages.
let mut attesters = duties_service.attesters.write();
let mut already_warned = Some(());
let mut attesters_map = duties_service.attesters.write();
for duty in relevant_duties {
let attesters_map = attesters_map.entry(duty.pubkey).or_default();
for result in duty_and_proof_results {
let duty_and_proof = match result {
Ok(duty_and_proof) => duty_and_proof,
Err(e) => {
error!(
log,
"Failed to produce duty and proof";
"error" => ?e,
"msg" => "may impair attestation duties"
);
// Do not abort the entire batch for a single failure.
continue;
}
};
// Only update the duties if either is true:
//
// - There were no known duties for this epoch.
// - The dependent root has changed, signalling a re-org.
if attesters_map
.get(&epoch)
.map_or(true, |(prior, _)| *prior != dependent_root)
let attester_map = attesters.entry(duty_and_proof.duty.pubkey).or_default();
if let Some((prior_dependent_root, _)) =
attester_map.insert(epoch, (dependent_root, duty_and_proof))
{
let duty_and_proof =
DutyAndProof::new(duty, &duties_service.validator_store, &duties_service.spec)?;
if let Some((prior_dependent_root, _)) =
attesters_map.insert(epoch, (dependent_root, duty_and_proof))
{
// Using `already_warned` avoids excessive logs.
if dependent_root != prior_dependent_root && already_warned.take().is_some() {
warn!(
log,
"Attester duties re-org";
"prior_dependent_root" => %prior_dependent_root,
"dependent_root" => %dependent_root,
"msg" => "this may happen from time to time"
)
}
// Using `already_warned` avoids excessive logs.
if dependent_root != prior_dependent_root && already_warned.take().is_some() {
warn!(
log,
"Attester duties re-org";
"prior_dependent_root" => %prior_dependent_root,
"dependent_root" => %dependent_root,
"msg" => "this may happen from time to time"
)
}
}
}
// Drop the write-lock.
//
// This is strictly unnecessary since the function ends immediately afterwards, but we remain
// defensive regardless.
drop(attesters_map);
drop(attesters);
Ok(())
}