Skip to content

Commit

Permalink
Remove some "wontfix" TODOs for the merge (#3449)
Browse files Browse the repository at this point in the history
## Issue Addressed

NA

## Proposed Changes

Removes three types of TODOs:

1. `execution_layer/src/lib.rs`: It was [determined](ethereum/consensus-specs#2636 (comment)) that there is no action required here.
2. `beacon_processor/worker/gossip_methods.rs`: Removed TODOs relating to peer scoring that have already been addressed via `epe.penalize_peer()`.
    - It seems `cargo fmt` wanted to adjust some things here as well 🤷 
3. `proto_array_fork_choice.rs`: it would be nice to remove that useless `bool` for cleanliness, but I don't think it's something we need to do and the TODO just makes things look messier IMO.


## Additional Info

There should be no functional changes to the code in this PR.

There are still some TODOs lingering, those ones require actual changes or more thought.
  • Loading branch information
paulhauner committed Aug 10, 2022
1 parent 4e05f19 commit 4fc0cb1
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 14 deletions.
8 changes: 0 additions & 8 deletions beacon_node/execution_layer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1267,14 +1267,6 @@ impl<T: EthSpec> ExecutionLayer<T> {
}

/// Maps to the `eth_getBlockByHash` JSON-RPC call.
///
/// ## TODO(merge)
///
/// This will return an execution block regardless of whether or not it was created by a PoW
/// miner (pre-merge) or a PoS validator (post-merge). It's not immediately clear if this is
/// correct or not, see the discussion here:
///
/// https://github.com/ethereum/consensus-specs/issues/2636
async fn get_pow_block(
&self,
engine: &Engine,
Expand Down
16 changes: 11 additions & 5 deletions beacon_node/network/src/beacon_processor/worker/gossip_methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -771,12 +771,15 @@ impl<T: BeaconChainTypes> Worker<T> {
debug!(self.log, "Could not verify block for gossip, ignoring the block";
"error" => %e);
// Prevent recurring behaviour by penalizing the peer slightly.
self.gossip_penalize_peer(peer_id, PeerAction::HighToleranceError, "gossip_block_high");
self.gossip_penalize_peer(
peer_id,
PeerAction::HighToleranceError,
"gossip_block_high",
);
self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
return None;
}
// TODO(merge): reconsider peer scoring for this event.
Err(ref e @BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => {
Err(ref e @ BlockError::ExecutionPayloadError(ref epe)) if !epe.penalize_peer() => {
debug!(self.log, "Could not verify block for gossip, ignoring the block";
"error" => %e);
self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
Expand All @@ -795,15 +798,18 @@ impl<T: BeaconChainTypes> Worker<T> {
| Err(e @ BlockError::TooManySkippedSlots { .. })
| Err(e @ BlockError::WeakSubjectivityConflict)
| Err(e @ BlockError::InconsistentFork(_))
// TODO(merge): reconsider peer scoring for this event.
| Err(e @ BlockError::ExecutionPayloadError(_))
// TODO(merge): reconsider peer scoring for this event.
| Err(e @ BlockError::ParentExecutionPayloadInvalid { .. })
| Err(e @ BlockError::GenesisBlock) => {
warn!(self.log, "Could not verify block for gossip, rejecting the block";
"error" => %e);
self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject);
self.gossip_penalize_peer(peer_id, PeerAction::LowToleranceError, "gossip_block_low");
self.gossip_penalize_peer(
peer_id,
PeerAction::LowToleranceError,
"gossip_block_low",
);
return None;
}
};
Expand Down
2 changes: 1 addition & 1 deletion consensus/proto_array/src/proto_array_fork_choice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub enum ExecutionStatus {
///
/// This `bool` only exists to satisfy our SSZ implementation which requires all variants
/// to have a value. It can be set to anything.
Irrelevant(bool), // TODO(merge): fix bool.
Irrelevant(bool),
}

impl ExecutionStatus {
Expand Down

0 comments on commit 4fc0cb1

Please sign in to comment.