Skip to content

Commit

Permalink
[Consensus] Small enhancement to the invalid blocks metric (#19515)
Browse files Browse the repository at this point in the history
## Description 

Use the hostname whenever possible. Add the error type as well.

## Test plan 

CI

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
  • Loading branch information
akichidis authored Sep 26, 2024
1 parent 5473d94 commit 15de3f2
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 6 deletions.
4 changes: 2 additions & 2 deletions consensus/core/src/authority_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl<C: CoreThreadDispatcher> NetworkService for AuthorityService<C> {
.metrics
.node_metrics
.invalid_blocks
.with_label_values(&[peer_hostname, "handle_send_block"])
.with_label_values(&[peer_hostname, "handle_send_block", "UnexpectedAuthority"])
.inc();
let e = ConsensusError::UnexpectedAuthority(signed_block.author(), peer);
info!("Block with wrong authority from {}: {}", peer, e);
Expand All @@ -108,7 +108,7 @@ impl<C: CoreThreadDispatcher> NetworkService for AuthorityService<C> {
.metrics
.node_metrics
.invalid_blocks
.with_label_values(&[peer_hostname, "handle_send_block"])
.with_label_values(&[peer_hostname, "handle_send_block", e.clone().name()])
.inc();
info!("Invalid block from {}: {}", peer, e);
return Err(e);
Expand Down
9 changes: 8 additions & 1 deletion consensus/core/src/block_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,18 @@ impl BlockManager {
}
}
for (block_ref, block) in blocks_to_reject {
let hostname = self
.context
.committee
.authority(block_ref.author)
.hostname
.clone();

self.context
.metrics
.node_metrics
.invalid_blocks
.with_label_values(&[&block_ref.author.to_string(), "accept_block"])
.with_label_values(&[&hostname, "accept_block", "InvalidAncestors"])
.inc();
warn!("Invalid block {:?} is rejected", block);
}
Expand Down
43 changes: 43 additions & 0 deletions consensus/core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,13 @@ pub(crate) enum ConsensusError {
Shutdown,
}

impl ConsensusError {
/// Returns the error name - only the enun name without any parameters - as a static string.
pub fn name(&self) -> &'static str {
self.into()
}
}

pub type ConsensusResult<T> = Result<T, ConsensusError>;

#[macro_export]
Expand All @@ -209,3 +216,39 @@ macro_rules! ensure {
}
};
}

#[cfg(test)]
mod test {
use super::*;

/// This test ensures that consensus errors when converted to a static string are the same as the enum name without
/// any parameterers included to the result string.
#[test]
fn test_error_name() {
{
let error = ConsensusError::InvalidAncestorRound {
ancestor: 10,
block: 11,
};
let error: &'static str = error.into();

assert_eq!(error, "InvalidAncestorRound");
}

{
let error = ConsensusError::InvalidAuthorityIndex {
index: AuthorityIndex::new_for_test(3),
max: 10,
};
assert_eq!(error.name(), "InvalidAuthorityIndex");
}

{
let error = ConsensusError::InsufficientParentStakes {
parent_stakes: 5,
quorum: 20,
};
assert_eq!(error.name(), "InsufficientParentStakes");
}
}
}
2 changes: 1 addition & 1 deletion consensus/core/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ impl NodeMetrics {
invalid_blocks: register_int_counter_vec_with_registry!(
"invalid_blocks",
"Number of invalid blocks per peer authority",
&["authority", "source"],
&["authority", "source", "error"],
registry,
).unwrap(),
rejected_blocks: register_int_counter_vec_with_registry!(
Expand Down
11 changes: 9 additions & 2 deletions consensus/core/src/synchronizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -611,11 +611,13 @@ impl<C: NetworkClient, V: BlockVerifier, D: CoreThreadDispatcher> Synchronizer<C
if let Err(e) = block_verifier.verify(&signed_block) {
// TODO: we might want to use a different metric to track the invalid "served" blocks
// from the invalid "proposed" ones.
let hostname = context.committee.authority(peer_index).hostname.clone();

context
.metrics
.node_metrics
.invalid_blocks
.with_label_values(&[&signed_block.author().to_string(), "synchronizer"])
.with_label_values(&[&hostname, "synchronizer", e.clone().name()])
.inc();
warn!("Invalid block received from {}: {}", peer_index, e);
return Err(e);
Expand Down Expand Up @@ -720,11 +722,16 @@ impl<C: NetworkClient, V: BlockVerifier, D: CoreThreadDispatcher> Synchronizer<C
for serialized_block in blocks {
let signed_block = bcs::from_bytes(&serialized_block).map_err(ConsensusError::MalformedBlock)?;
block_verifier.verify(&signed_block).tap_err(|err|{
let hostname = if context.committee.is_valid_index(signed_block.author()) {
context.committee.authority(signed_block.author()).hostname.clone()
} else {
signed_block.author().to_string()
};
context
.metrics
.node_metrics
.invalid_blocks
.with_label_values(&[&signed_block.author().to_string(), "synchronizer_own_block"])
.with_label_values(&[&hostname, "synchronizer_own_block", err.clone().name()])
.inc();
warn!("Invalid block received from {}: {}", authority_index, err);
})?;
Expand Down

0 comments on commit 15de3f2

Please sign in to comment.