Skip to content

Connectivity Report spam mitigations#314

Merged
SurfingNerd merged 3 commits intoDMDcoin:4.0from
SurfingNerd:i243-no-reports-on-missing-block-production
Nov 23, 2025
Merged

Connectivity Report spam mitigations#314
SurfingNerd merged 3 commits intoDMDcoin:4.0from
SurfingNerd:i243-no-reports-on-missing-block-production

Conversation

@SurfingNerd
Copy link
Collaborator

connectivity reports should not trigger if there is no block production
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements spam mitigations for connectivity reports in the HBBFT (Honey Badger BFT) consensus engine. The changes address issues #313 and #243 by introducing timing controls and cache optimizations to prevent unnecessary connectivity report spam.

  • Adds block-based cache tracking to avoid redundant engine cache updates
  • Separates early epoch end actions onto a dedicated timer (300s interval) to control connectivity report frequency
  • Implements timestamp-based validation to prevent connectivity reports when block production is stalled

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 13 comments.

File Description
crates/ethcore/src/engines/hbbft/hbbft_engine_cache.rs Adds block_last_update tracking to prevent duplicate cache refreshes and changes method signatures to static for better concurrency
crates/ethcore/src/engines/hbbft/hbbft_engine.rs Introduces HBBFT_CONNECTIVITY_TOKEN timer and refactors validator engine actions into separate methods for regular actions vs. early epoch end connectivity checks
crates/ethcore/src/engines/hbbft/hbbft_early_epoch_end_manager.rs Adds time-based validation to skip connectivity reports when block production has stalled (comparing block timestamp against current time)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// todo:
// this is max blocktime (hartbeat) x 2.
// on phoenix protocol triggers, this would also skip the block production.
if elapsed_since_last_block > 10 {
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The threshold value of 10 seconds seems quite small and may be too restrictive. The comment mentions "max blocktime (hartbeat) x 2" which should be 300 seconds (based on HBBFT_CONNECTIVITY_TOKEN timer) * 2 = 600 seconds. Using 10 seconds could cause legitimate early epoch end reports to be skipped when there's a slight delay in block production. Consider using a more appropriate threshold value that aligns with the comment or clarifying the intention.

Copilot uses AI. Check for mistakes.
if now >= block_time {
let elapsed_since_last_block = now - block_time;
// todo:
// this is max blocktime (hartbeat) x 2.
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Typo in comment: "hartbeat" should be "heartbeat"

Suggested change
// this is max blocktime (hartbeat) x 2.
// this is max blocktime (heartbeat) x 2.

Copilot uses AI. Check for mistakes.
// this is max blocktime (hartbeat) x 2.
// on phoenix protocol triggers, this would also skip the block production.
if elapsed_since_last_block > 10 {
info!(target:"engine", "skipping early-epoch-end: now {now} ; block_time {block_time}: Block WAS created in the future ?!?! :-x. not sending early epoch end reports.");
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Grammar issue: "Block WAS created" should use "was" in lowercase for consistent sentence capitalization.

Copilot uses AI. Check for mistakes.
// or the blockchain really produces blocks from the future.
// we are just not sending reports in this case.

error!(target:"engine", "early-epoch-end: now {now} ; block_time {block_time}: Block WAS created in the future ?!?! :-x. not sending early epoch end reports.");
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Grammar issue: "Block WAS created" should use "was" in lowercase for consistent sentence capitalization.

Copilot uses AI. Check for mistakes.
/// decides on the memorium data if we should update to contract data.
/// end executes them.
/// decides on the memorium data if we should update to contract data,
/// end sends out transactions to do so.
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Typo in comment: "end sends" should be "and sends"

Suggested change
/// end sends out transactions to do so.
/// and sends out transactions to do so.

Copilot uses AI. Check for mistakes.
/// hbbft early epoch end actions are executed on a different timing than the regular validator engine steps
fn do_validator_engine_early_epoch_end_actions(&self) -> Result<(), Error> {
// here we need to differentiate the different engine functions,
// that requre different levels of access to the client.
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Typo in comment: "requre" should be "require"

Suggested change
// that requre different levels of access to the client.
// that require different levels of access to the client.

Copilot uses AI. Check for mistakes.
// if we do not have to do anything, we can return early.
if !(should_connect_to_validator_set || should_handle_early_epoch_end) {
return Ok(());
if validator_set.len() > 0 {
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The comparison if validator_set.len() > 0 is not idiomatic Rust. Consider using !validator_set.is_empty() instead, which is the preferred way to check if a collection is non-empty.

Suggested change
if validator_set.len() > 0 {
if !validator_set.is_empty() {

Copilot uses AI. Check for mistakes.
self.hbbft_engine_cache.lock().is_staked()
}

/** returns if the signer of hbbft is a current validator. */
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The documentation comment uses /** */ syntax instead of the more idiomatic Rust /// syntax for doc comments. Consider using /// returns if the signer of hbbft is a current validator. instead.

Suggested change
/** returns if the signer of hbbft is a current validator. */
/// returns if the signer of hbbft is a current validator.

Copilot uses AI. Check for mistakes.
|e| warn!(target: "consensus", "ENGINE_PHOENIX_CHECK Timer failed: {}.", e),
);

// ealry epoch end connecitity token should be the same lenght then the max blocktime.
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Typo in comment: "lenght" should be "length"

Suggested change
// ealry epoch end connecitity token should be the same lenght then the max blocktime.
// ealry epoch end connecitity token should be the same length then the max blocktime.

Copilot uses AI. Check for mistakes.
// end of implementation for:
// https://github.com/DMDcoin/diamond-node/issues/243

let treshold: u64 = 2;
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Typo in comment: "treshold" should be "threshold"

Copilot uses AI. Check for mistakes.
@SurfingNerd SurfingNerd merged commit ca0bed3 into DMDcoin:4.0 Nov 23, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants