cerc-io/lighthouse
16
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Remove some "wontfix" TODOs for the merge (#3449)

Browse Source 
## Issue Addressed

NA

## Proposed Changes

Removes three types of TODOs:

1. `execution_layer/src/lib.rs`: It was [determined](https://github.com/ethereum/consensus-specs/issues/2636#issuecomment-988688742) 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.
This commit is contained in:
Paul Hauner 2022-08-10 13:06:46 +00:00
parent 4e05f19fb5
commit 4fc0cb121c
3 changed files with 12 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
beacon_node/execution_layer/src/lib.rs
View File

@ -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,

16
beacon_node/network/src/beacon_processor/worker/gossip_methods.rs
View File

@ -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);
@ -795,7 +798,6 @@ 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 { .. })
@ -803,7 +805,11 @@ impl<T: BeaconChainTypes> Worker<T> {
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;
}
};

2
consensus/proto_array/src/proto_array_fork_choice.rs
View File

@ -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 {