Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add block ban flag --invalid-block-roots #7042

Open
wants to merge 6 commits into
base: release-v7.0.0
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions beacon_node/beacon_chain/src/block_verification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ use std::borrow::Cow;
use std::fmt::Debug;
use std::fs;
use std::io::Write;
use std::str::FromStr;
use std::sync::Arc;
use store::{Error as DBError, HotStateSummary, KeyValueStore, StoreOp};
use strum::AsRefStr;
Expand Down Expand Up @@ -282,6 +283,9 @@ pub enum BlockError {
/// problems to worry about than losing peers, and we're doing the network a favour by
/// disconnecting.
ParentExecutionPayloadInvalid { parent_root: Hash256 },
/// This is a known invalid block that was listed in Lighthouses configuration.
/// At the moment this error is only relevant as part of the Holesky network recovery efforts.
KnownInvalidExecutionPayload(Hash256),
/// The block is a slashable equivocation from the proposer.
///
/// ## Peer scoring
Expand Down Expand Up @@ -1326,6 +1330,19 @@ impl<T: BeaconChainTypes> ExecutionPendingBlock<T> {
chain: &Arc<BeaconChain<T>>,
notify_execution_layer: NotifyExecutionLayer,
) -> Result<Self, BlockError> {
let invalid_holesky_block = {
if let Ok(invalid_block_root) = Hash256::from_str(
"2db899881ed8546476d0b92c6aa9110bea9a4cd0dbeb5519eb0ea69575f1f359",
) {
block_root == invalid_block_root && chain.spec.deposit_chain_id == 17000
} else {
false
}
};
if chain.config.invalid_block_roots.contains(&block_root) || invalid_holesky_block {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag can be exploited for censoring, you should restrict usage to a specific network

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ive constrained this flag to holesky only

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with both of Lion's suggested changes, sorry 😅

I think we need the flag to be not limited to Holesky so that we can potentially use it to get out of trouble on other catastrophically failing networks (without having to patch). We don't have another solution for preventing optimistic sync of the wrong chain at the moment.

I think this also means we should keep the hardcoded Holesky block as a separate check. It doesn't make sense as the flag default if the flag works on multiple networks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, I had assumed that this was strictly a holesky thing. If the plan is to keep this flag until we have a better solution for optimistic sync of the wrong chain, I'm cool with that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've reverted the changes back to what they were originally

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get rid of this when we implement checkpoint sync from an unfinalized checkpoint

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@realbigsean would that prevent us from importing an invalid block though? wouldn't sync still attempt to sync with the invalid chain?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you’d have to use a checkpoint ahead of the invalid block, and since we’d backfill from there we’d avoid it. Part of the complexity of unfinalized checkpoint sync would be making sure you ignore chain tips older than the checkpoint

return Err(BlockError::KnownInvalidExecutionPayload(block_root));
}

chain
.observed_slashable
.write()
Expand Down
6 changes: 4 additions & 2 deletions beacon_node/beacon_chain/src/chain_config.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
pub use proto_array::{DisallowedReOrgOffsets, ReOrgThreshold};
use serde::{Deserialize, Serialize};
use std::time::Duration;
use types::{Checkpoint, Epoch};
use std::{collections::HashSet, time::Duration};
use types::{Checkpoint, Epoch, Hash256};

pub const DEFAULT_RE_ORG_HEAD_THRESHOLD: ReOrgThreshold = ReOrgThreshold(20);
pub const DEFAULT_RE_ORG_PARENT_THRESHOLD: ReOrgThreshold = ReOrgThreshold(160);
Expand Down Expand Up @@ -94,6 +94,7 @@ pub struct ChainConfig {
/// The delay in milliseconds applied by the node between sending each blob or data column batch.
/// This doesn't apply if the node is the block proposer.
pub blob_publication_batch_interval: Duration,
pub invalid_block_roots: HashSet<Hash256>,
}

impl Default for ChainConfig {
Expand Down Expand Up @@ -129,6 +130,7 @@ impl Default for ChainConfig {
enable_sampling: false,
blob_publication_batches: 4,
blob_publication_batch_interval: Duration::from_millis(300),
invalid_block_roots: HashSet::new(),
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1457,6 +1457,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
| Err(e @ BlockError::InconsistentFork(_))
| Err(e @ BlockError::ExecutionPayloadError(_))
| Err(e @ BlockError::ParentExecutionPayloadInvalid { .. })
| Err(e @ BlockError::KnownInvalidExecutionPayload(_))
| Err(e @ BlockError::GenesisBlock) => {
warn!(self.log, "Could not verify block for gossip. Rejecting the block";
"error" => %e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -800,6 +800,18 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
peer_action: Some(PeerAction::LowToleranceError),
})
}
// Penalise peers for sending us banned blocks.
BlockError::KnownInvalidExecutionPayload(block_root) => {
warn!(
self.log,
"Received block known to be invalid";
"block_root" => ?block_root,
);
Err(ChainSegmentFailed {
message: format!("Banned block: {block_root:?}"),
peer_action: Some(PeerAction::LowToleranceError),
})
}
other => {
debug!(
self.log, "Invalid block received";
Expand Down
8 changes: 8 additions & 0 deletions beacon_node/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1608,5 +1608,13 @@ pub fn cli_app() -> Command {
.action(ArgAction::Set)
.display_order(0)
)
.arg(
Arg::new("invalid-block-roots")
.long("invalid-block-roots")
.value_name("FILE")
.help("Path to a comma separated file containing block roots that should be treated as invalid during block verification.")
.action(ArgAction::Set)
.hide(true)
)
.group(ArgGroup::new("enable_http").args(["http", "gui", "staking"]).multiple(true))
}
31 changes: 30 additions & 1 deletion beacon_node/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ use lighthouse_network::{multiaddr::Protocol, Enr, Multiaddr, NetworkConfig, Pee
use sensitive_url::SensitiveUrl;
use slog::{info, warn, Logger};
use std::cmp::max;
use std::collections::HashSet;
use std::fmt::Debug;
use std::fs;
use std::io::IsTerminal;
use std::io::{IsTerminal, Read};
use std::net::Ipv6Addr;
use std::net::{IpAddr, Ipv4Addr, ToSocketAddrs};
use std::num::NonZeroU16;
Expand Down Expand Up @@ -897,6 +898,34 @@ pub fn get_config<E: EthSpec>(
.max_gossip_aggregate_batch_size =
clap_utils::parse_required(cli_args, "beacon-processor-aggregate-batch-size")?;

if let Some(invalid_block_roots_file_path) =
clap_utils::parse_optional::<String>(cli_args, "invalid-block-roots")?
{
let mut file = std::fs::File::open(invalid_block_roots_file_path)
.map_err(|e| format!("Failed to open invalid-block-roots file: {}", e))?;
let mut contents = String::new();
file.read_to_string(&mut contents)
.map_err(|e| format!("Failed to read invalid-block-roots file {}", e))?;
let invalid_block_roots: HashSet<Hash256> = contents
.split(',')
.filter_map(
|s| match Hash256::from_str(s.strip_prefix("0x").unwrap_or(s).trim()) {
Ok(block_root) => Some(block_root),
Err(e) => {
warn!(
log,
"Unable to parse invalid block root";
"block_root" => s,
"error" => ?e,
);
None
}
},
)
.collect();
client_config.chain.invalid_block_roots = invalid_block_roots;
}

Ok(client_config)
}

Expand Down
16 changes: 16 additions & 0 deletions lighthouse/tests/beacon_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2749,3 +2749,19 @@ fn beacon_node_backend_override() {
assert_eq!(config.store.backend, BeaconNodeBackend::LevelDb);
});
}

#[test]
fn invalid_block_root_flag() {
let dir = TempDir::new().expect("Unable to create temporary directory");
let mut file =
File::create(dir.path().join("invalid-block-roots")).expect("Unable to create file");
file.write_all(b"2db899881ed8546476d0b92c6aa9110bea9a4cd0dbeb5519eb0ea69575f1f359, 2db899881ed8546476d0b92c6aa9110bea9a4cd0dbeb5519eb0ea69575f1f358, 0x3db899881ed8546476d0b92c6aa9110bea9a4cd0dbeb5519eb0ea69575f1f358")
.expect("Unable to write to file");
CommandLineTest::new()
.flag(
"invalid-block-roots",
dir.path().join("invalid-block-roots").as_os_str().to_str(),
)
.run_with_zero_port()
.with_config(|config| assert_eq!(config.chain.invalid_block_roots.len(), 3))
}