switch to enum for block signing to allow differing types

Author: dknopik

validator_client/lighthouse_validator_store/src/lib.rs

@@ -24,7 +24,8 @@ use types::{
     VoluntaryExit,
 };
 use validator_store::{
-    DoppelgangerStatus, Error as ValidatorStoreError, ProposalData, SignBlock, ValidatorStore,
+    DoppelgangerStatus, Error as ValidatorStoreError, ProposalData, SignedBlock, UnsignedBlock,
+    ValidatorStore,
 };
 
 pub type Error = ValidatorStoreError<SigningError>;
@@ -420,6 +421,102 @@ impl<T: SlotClock + 'static, E: EthSpec> LighthouseValidatorStore<T, E> {
             )
         })
     }
+
+    async fn sign_abstract_block<Payload: AbstractExecPayload<E>>(
+        &self,
+        validator_pubkey: PublicKeyBytes,
+        block: BeaconBlock<E, Payload>,
+        current_slot: Slot,
+    ) -> Result<SignedBeaconBlock<E, Payload>, Error> {
+        // Make sure the block slot is not higher than the current slot to avoid potential attacks.
+        if block.slot() > current_slot {
+            warn!(
+                self.log,
+                "Not signing block with slot greater than current slot";
+                "block_slot" => block.slot().as_u64(),
+                "current_slot" => current_slot.as_u64()
+            );
+            return Err(Error::GreaterThanCurrentSlot {
+                slot: block.slot(),
+                current_slot,
+            });
+        }
+
+        let signing_epoch = block.epoch();
+        let signing_context = self.signing_context(Domain::BeaconProposer, signing_epoch);
+        let domain_hash = signing_context.domain_hash(&self.spec);
+
+        let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?;
+
+        // Check for slashing conditions.
+        let slashing_status = if signing_method
+            .requires_local_slashing_protection(self.enable_web3signer_slashing_protection)
+        {
+            self.slashing_protection.check_and_insert_block_proposal(
+                &validator_pubkey,
+                &block.block_header(),
+                domain_hash,
+            )
+        } else {
+            Ok(Safe::Valid)
+        };
+
+        match slashing_status {
+            // We can safely sign this block without slashing.
+            Ok(Safe::Valid) => {
+                validator_metrics::inc_counter_vec(
+                    &validator_metrics::SIGNED_BLOCKS_TOTAL,
+                    &[validator_metrics::SUCCESS],
+                );
+
+                let signature = signing_method
+                    .get_signature(
+                        SignableMessage::BeaconBlock(&block),
+                        signing_context,
+                        &self.spec,
+                        &self.task_executor,
+                    )
+                    .await?;
+                Ok(SignedBeaconBlock::from_block(block, signature))
+            }
+            Ok(Safe::SameData) => {
+                warn!(
+                    self.log,
+                    "Skipping signing of previously signed block";
+                );
+                validator_metrics::inc_counter_vec(
+                    &validator_metrics::SIGNED_BLOCKS_TOTAL,
+                    &[validator_metrics::SAME_DATA],
+                );
+                Err(Error::SameData)
+            }
+            Err(NotSafe::UnregisteredValidator(pk)) => {
+                warn!(
+                    self.log,
+                    "Not signing block for unregistered validator";
+                    "msg" => "Carefully consider running with --init-slashing-protection (see --help)",
+                    "public_key" => format!("{:?}", pk)
+                );
+                validator_metrics::inc_counter_vec(
+                    &validator_metrics::SIGNED_BLOCKS_TOTAL,
+                    &[validator_metrics::UNREGISTERED],
+                );
+                Err(Error::Slashable(NotSafe::UnregisteredValidator(pk)))
+            }
+            Err(e) => {
+                crit!(
+                    self.log,
+                    "Not signing slashable block";
+                    "error" => format!("{:?}", e)
+                );
+                validator_metrics::inc_counter_vec(
+                    &validator_metrics::SIGNED_BLOCKS_TOTAL,
+                    &[validator_metrics::SLASHABLE],
+                );
+                Err(Error::Slashable(e))
+            }
+        }
+    }
 }
 
 impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore for LighthouseValidatorStore<T, E> {
@@ -578,6 +675,24 @@ impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore for LighthouseValidatorS
             .set_index(validator_pubkey, index);
     }
 
+    async fn sign_block(
+        &self,
+        validator_pubkey: PublicKeyBytes,
+        block: UnsignedBlock<E>,
+        current_slot: Slot,
+    ) -> Result<SignedBlock<E>, Error> {
+        match block {
+            UnsignedBlock::Full(block) => self
+                .sign_abstract_block(validator_pubkey, block, current_slot)
+                .await
+                .map(SignedBlock::Full),
+            UnsignedBlock::Blinded(block) => self
+                .sign_abstract_block(validator_pubkey, block, current_slot)
+                .await
+                .map(SignedBlock::Blinded),
+        }
+    }
+
     async fn sign_attestation(
         &self,
         validator_pubkey: PublicKeyBytes,
@@ -999,103 +1114,3 @@ impl<T: SlotClock + 'static, E: EthSpec> ValidatorStore for LighthouseValidatorS
             })
     }
 }
-
-impl<T: SlotClock + 'static, E: EthSpec, P: AbstractExecPayload<E>>
-    SignBlock<E, P, signing_method::Error> for LighthouseValidatorStore<T, E>
-{
-    async fn sign_block(
-        &self,
-        validator_pubkey: PublicKeyBytes,
-        block: BeaconBlock<E, P>,
-        current_slot: Slot,
-    ) -> Result<SignedBeaconBlock<E, P>, Error> {
-        // Make sure the block slot is not higher than the current slot to avoid potential attacks.
-        if block.slot() > current_slot {
-            warn!(
-                self.log,
-                "Not signing block with slot greater than current slot";
-                "block_slot" => block.slot().as_u64(),
-                "current_slot" => current_slot.as_u64()
-            );
-            return Err(Error::GreaterThanCurrentSlot {
-                slot: block.slot(),
-                current_slot,
-            });
-        }
-
-        let signing_epoch = block.epoch();
-        let signing_context = self.signing_context(Domain::BeaconProposer, signing_epoch);
-        let domain_hash = signing_context.domain_hash(&self.spec);
-
-        let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?;
-
-        // Check for slashing conditions.
-        let slashing_status = if signing_method
-            .requires_local_slashing_protection(self.enable_web3signer_slashing_protection)
-        {
-            self.slashing_protection.check_and_insert_block_proposal(
-                &validator_pubkey,
-                &block.block_header(),
-                domain_hash,
-            )
-        } else {
-            Ok(Safe::Valid)
-        };
-
-        match slashing_status {
-            // We can safely sign this block without slashing.
-            Ok(Safe::Valid) => {
-                validator_metrics::inc_counter_vec(
-                    &validator_metrics::SIGNED_BLOCKS_TOTAL,
-                    &[validator_metrics::SUCCESS],
-                );
-
-                let signature = signing_method
-                    .get_signature(
-                        SignableMessage::BeaconBlock(&block),
-                        signing_context,
-                        &self.spec,
-                        &self.task_executor,
-                    )
-                    .await?;
-                Ok(SignedBeaconBlock::from_block(block, signature))
-            }
-            Ok(Safe::SameData) => {
-                warn!(
-                    self.log,
-                    "Skipping signing of previously signed block";
-                );
-                validator_metrics::inc_counter_vec(
-                    &validator_metrics::SIGNED_BLOCKS_TOTAL,
-                    &[validator_metrics::SAME_DATA],
-                );
-                Err(Error::SameData)
-            }
-            Err(NotSafe::UnregisteredValidator(pk)) => {
-                warn!(
-                    self.log,
-                    "Not signing block for unregistered validator";
-                    "msg" => "Carefully consider running with --init-slashing-protection (see --help)",
-                    "public_key" => format!("{:?}", pk)
-                );
-                validator_metrics::inc_counter_vec(
-                    &validator_metrics::SIGNED_BLOCKS_TOTAL,
-                    &[validator_metrics::UNREGISTERED],
-                );
-                Err(Error::Slashable(NotSafe::UnregisteredValidator(pk)))
-            }
-            Err(e) => {
-                crit!(
-                    self.log,
-                    "Not signing slashable block";
-                    "error" => format!("{:?}", e)
-                );
-                validator_metrics::inc_counter_vec(
-                    &validator_metrics::SIGNED_BLOCKS_TOTAL,
-                    &[validator_metrics::SLASHABLE],
-                );
-                Err(Error::Slashable(e))
-            }
-        }
-    }
-}

validator_client/validator_services/src/block_service.rs

@@ -333,22 +333,27 @@ impl<S: ValidatorStore + 'static, T: SlotClock + 'static> BlockService<S, T> {
     ) -> Result<(), BlockError> {
         let signing_timer = validator_metrics::start_timer(&validator_metrics::BLOCK_SIGNING_TIMES);
 
-        let res = match unsigned_block {
+        let (block, maybe_blobs) = match unsigned_block {
             UnsignedBlock::Full(block_contents) => {
                 let (block, maybe_blobs) = block_contents.deconstruct();
-                self.validator_store
-                    .sign_block(*validator_pubkey, block, slot)
-                    .await
-                    .map(|b| SignedBlock::Full(PublishBlockRequest::new(Arc::new(b), maybe_blobs)))
+                (block.into(), maybe_blobs)
             }
-            UnsignedBlock::Blinded(block) => self
-                .validator_store
-                .sign_block(*validator_pubkey, block, slot)
-                .await
-                .map(Arc::new)
-                .map(SignedBlock::Blinded),
+            UnsignedBlock::Blinded(block) => (block.into(), None),
         };
 
+        let res = self
+            .validator_store
+            .sign_block(*validator_pubkey, block, slot)
+            .await
+            .map(|block| match block {
+                validator_store::SignedBlock::Full(block) => {
+                    SignedBlock::Full(PublishBlockRequest::new(Arc::new(block), maybe_blobs))
+                }
+                validator_store::SignedBlock::Blinded(block) => {
+                    SignedBlock::Blinded(Arc::new(block))
+                }
+            });
+
         let signed_block = match res {
             Ok(block) => block,
             Err(ValidatorStoreError::UnknownPubkey(pubkey)) => {

validator_client/validator_store/src/lib.rs

@@ -2,9 +2,9 @@ use slashing_protection::NotSafe;
 use std::fmt::Debug;
 use std::future::Future;
 use types::{
-    AbstractExecPayload, Address, Attestation, AttestationError, BeaconBlock, BlindedPayload,
-    Epoch, EthSpec, FullPayload, Graffiti, Hash256, PublicKeyBytes, SelectionProof, Signature,
-    SignedAggregateAndProof, SignedBeaconBlock, SignedContributionAndProof,
+    Address, Attestation, AttestationError, BeaconBlock, BlindedBeaconBlock, Epoch, EthSpec,
+    Graffiti, Hash256, PublicKeyBytes, SelectionProof, Signature, SignedAggregateAndProof,
+    SignedBeaconBlock, SignedBlindedBeaconBlock, SignedContributionAndProof,
     SignedValidatorRegistrationData, SignedVoluntaryExit, Slot, SyncCommitteeContribution,
     SyncCommitteeMessage, SyncSelectionProof, SyncSubnetId, ValidatorRegistrationData,
     VoluntaryExit,
@@ -37,12 +37,7 @@ pub struct ProposalData {
     pub builder_proposals: bool,
 }
 
-pub trait ValidatorStore:
-    SignBlock<Self::E, FullPayload<Self::E>, Self::Error>
-    + SignBlock<Self::E, BlindedPayload<Self::E>, Self::Error>
-    + Send
-    + Sync
-{
+pub trait ValidatorStore: Send + Sync {
     type Error: Debug + Send + Sync;
     type E: EthSpec;
 
@@ -99,6 +94,13 @@ pub trait ValidatorStore:
 
     fn set_validator_index(&self, validator_pubkey: &PublicKeyBytes, index: u64);
 
+    fn sign_block(
+        &self,
+        validator_pubkey: PublicKeyBytes,
+        block: UnsignedBlock<Self::E>,
+        current_slot: Slot,
+    ) -> impl Future<Output = Result<SignedBlock<Self::E>, Error<Self::Error>>> + Send;
+
     fn sign_attestation(
         &self,
         validator_pubkey: PublicKeyBytes,
@@ -175,17 +177,38 @@ pub trait ValidatorStore:
     fn proposal_data(&self, pubkey: &PublicKeyBytes) -> Option<ProposalData>;
 }
 
-// This is a workaround: directly adding this fn into `ValidatorStore`, abstract over P, causes
-// weird compiler errors which I suspect are compiler bugs
-// Another advantage of this separate trait is to allow implementors separate implementations for
-// different payload
-pub trait SignBlock<E: EthSpec, P: AbstractExecPayload<E>, Err> {
-    fn sign_block(
-        &self,
-        validator_pubkey: PublicKeyBytes,
-        block: BeaconBlock<E, P>,
-        current_slot: Slot,
-    ) -> impl Future<Output = Result<SignedBeaconBlock<E, P>, Error<Err>>> + Send;
+pub enum UnsignedBlock<E: EthSpec> {
+    Full(BeaconBlock<E>),
+    Blinded(BlindedBeaconBlock<E>),
+}
+
+impl<E: EthSpec> From<BeaconBlock<E>> for UnsignedBlock<E> {
+    fn from(block: BeaconBlock<E>) -> Self {
+        UnsignedBlock::Full(block)
+    }
+}
+
+impl<E: EthSpec> From<BlindedBeaconBlock<E>> for UnsignedBlock<E> {
+    fn from(block: BlindedBeaconBlock<E>) -> Self {
+        UnsignedBlock::Blinded(block)
+    }
+}
+
+pub enum SignedBlock<E: EthSpec> {
+    Full(SignedBeaconBlock<E>),
+    Blinded(SignedBlindedBeaconBlock<E>),
+}
+
+impl<E: EthSpec> From<SignedBeaconBlock<E>> for SignedBlock<E> {
+    fn from(block: SignedBeaconBlock<E>) -> Self {
+        SignedBlock::Full(block)
+    }
+}
+
+impl<E: EthSpec> From<SignedBlindedBeaconBlock<E>> for SignedBlock<E> {
+    fn from(block: SignedBlindedBeaconBlock<E>) -> Self {
+        SignedBlock::Blinded(block)
+    }
 }
 
 /// A wrapper around `PublicKeyBytes` which encodes information about the status of a validator