Fix mistake with attestation skip slots (#1539)

## Issue Addressed

NA

## Proposed Changes

- Fixes a mistake I made in #1530 which resulted us in *not* rejecting attestations that we intended to reject.
- Adds skip-slot checks for blocks earlier in import process, so it rejects gossip and RPC blocks.

## Additional Info

NA
This commit is contained in:
Paul Hauner 2020-08-18 06:28:26 +00:00
parent e1e5002d3c
commit d4f763bbae
2 changed files with 33 additions and 9 deletions

View File

@ -549,7 +549,7 @@ fn verify_head_block_is_known<T: BeaconChainTypes>(
{
// Reject any block that exceeds our limit on skipped slots.
if let Some(max_skip_slots) = max_skip_slots {
if block.slot > attestation.data.slot + max_skip_slots {
if attestation.data.slot > block.slot + max_skip_slots {
return Err(Error::TooManySkippedSlots {
head_block_slot: block.slot,
attestation_slot: attestation.data.slot,

View File

@ -431,6 +431,9 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
let (mut parent, block) = load_parent(block, chain)?;
// Reject any block that exceeds our limit on skipped slots.
check_block_skip_slots(chain, &parent.beacon_block.message, &block.message)?;
let state = cheap_state_advance_to_obtain_committees(
&mut parent.beacon_state,
block.slot(),
@ -517,6 +520,10 @@ impl<T: BeaconChainTypes> SignatureVerifiedBlock<T> {
chain: &BeaconChain<T>,
) -> Result<Self, BlockError<T::EthSpec>> {
let (mut parent, block) = load_parent(block, chain)?;
// Reject any block that exceeds our limit on skipped slots.
check_block_skip_slots(chain, &parent.beacon_block.message, &block.message)?;
let block_root = get_block_root(&block);
let state = cheap_state_advance_to_obtain_committees(
@ -648,14 +655,7 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
}
// Reject any block that exceeds our limit on skipped slots.
if let Some(max_skip_slots) = chain.config.import_max_skip_slots {
if block.slot() > parent.beacon_block.slot() + max_skip_slots {
return Err(BlockError::TooManySkippedSlots {
parent_slot: parent.beacon_block.slot(),
block_slot: block.slot(),
});
}
}
check_block_skip_slots(chain, &parent.beacon_block.message, &block.message)?;
/*
* Perform cursory checks to see if the block is even worth processing.
@ -799,6 +799,30 @@ impl<'a, T: BeaconChainTypes> FullyVerifiedBlock<'a, T> {
}
}
/// Check that the count of skip slots between the block and its parent does not exceed our maximum
/// value.
///
/// Whilst this is not part of the specification, we include this to help prevent us from DoS
/// attacks. In times of dire network circumstance, the user can configure the
/// `import_max_skip_slots` value.
fn check_block_skip_slots<T: BeaconChainTypes>(
chain: &BeaconChain<T>,
parent: &BeaconBlock<T::EthSpec>,
block: &BeaconBlock<T::EthSpec>,
) -> Result<(), BlockError<T::EthSpec>> {
// Reject any block that exceeds our limit on skipped slots.
if let Some(max_skip_slots) = chain.config.import_max_skip_slots {
if block.slot > parent.slot + max_skip_slots {
return Err(BlockError::TooManySkippedSlots {
parent_slot: parent.slot,
block_slot: block.slot,
});
}
}
Ok(())
}
/// Returns `Ok(())` if the block is later than the finalized slot on `chain`.
///
/// Returns an error if the block is earlier or equal to the finalized slot, or there was an error