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

feat(consensus): optionally configure max block time #1097

Conversation

therealdannzor
Copy link
Contributor

@therealdannzor therealdannzor commented Jul 29, 2024

Description

Update consensus constants to include the (maximum) base block time, a component of the GST before a leader fails.

Resolves #1088

Motivation and Context

Currently it is hard coded to 10 seconds. This will make it configurable.

How Has This Been Tested?

Unit and integration tests.

What process can a PR reviewer use to test or verify this change?

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Other - Please specify

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Looking good so far, some very minor comments

dan_layer/consensus/src/hotstuff/pacemaker.rs Outdated Show resolved Hide resolved
@therealdannzor therealdannzor requested a review from sdbondi July 29, 2024 10:05
@therealdannzor therealdannzor force-pushed the feat-consensus-block-time-config branch from 11b750d to f30c3a9 Compare July 29, 2024 10:34
Copy link

github-actions bot commented Jul 29, 2024

Test Results (CI)

542 tests  ±0   542 ✅ ±0   1h 41m 17s ⏱️ - 21m 46s
 64 suites ±0     0 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit e07df42. ± Comparison against base commit 359dd6e.

♻️ This comment has been updated with latest results.

@therealdannzor therealdannzor marked this pull request as ready for review July 29, 2024 11:34
@@ -26,6 +26,7 @@ pub struct ConsensusConstants {
pub committee_size: u32,
pub max_base_layer_blocks_ahead: u64,
pub max_base_layer_blocks_behind: u64,
pub pacemaker_max_base_time: std::time::Duration,
Copy link
Member

Choose a reason for hiding this comment

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

nit: use std::time::Duration

@sdbondi sdbondi added this pull request to the merge queue Jul 29, 2024
Merged via the queue into tari-project:development with commit f0beea4 Jul 29, 2024
13 checks passed
@therealdannzor therealdannzor deleted the feat-consensus-block-time-config branch July 29, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[consensus] Optionally specify maximum block time in pacemaker
3 participants