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

fix(drive-abci): require 75 percent of active, not total hpmns #2127

Merged
merged 2 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -331,9 +331,6 @@ where
block_execution_context::v0::BlockExecutionContextV0 {
block_state_info: block_state_info.into(),
epoch_info: epoch_info.clone(),
// TODO: It doesn't seem correct to use previous block count of hpmns.
// We currently not using this field in the codebase. We probably should just remove it.
hpmn_count: last_committed_platform_state.hpmn_list_len() as u32,
unsigned_withdrawal_transactions: unsigned_withdrawal_transaction_bytes,
block_platform_state,
proposer_results: None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,6 @@ mod tests {
let block_execution_context = BlockExecutionContextV0 {
block_state_info: block_info.clone().into(),
epoch_info,
hpmn_count: 0,
unsigned_withdrawal_transactions: Default::default(),
block_platform_state,
proposer_results: None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,6 @@ mod tests {
let block_execution_context = BlockExecutionContextV0 {
block_state_info: block_info.clone().into(),
epoch_info: epoch_info.clone(),
hpmn_count: 0,
unsigned_withdrawal_transactions: Default::default(),
block_platform_state,
proposer_results: None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,13 @@ impl<C> Platform<C> {
));
}

tracing::trace!(
tracing::debug!(
total_hpmns,
required_upgraded_hpmns,
all_votes = ?protocol_versions_counter.global_cache,
?versions_passing_threshold,
"Protocol version voting is finished. {} versions passing the threshold: {:?}",
"Protocol version voting is finished. we require {} upgraded, {} versions passing the threshold: {:?}",
required_upgraded_hpmns,
versions_passing_threshold.len(),
versions_passing_threshold
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::platform_types::platform_state::v0::PlatformStateV0Methods;
use crate::platform_types::platform_state::PlatformState;
use chrono::{TimeZone, Utc};
use dpp::block::block_info::BlockInfo;
use dpp::dashcore::Network::Testnet;
use dpp::version::PlatformVersion;
use drive::grovedb::Transaction;

Expand Down Expand Up @@ -57,10 +58,16 @@ impl<C> Platform<C> {
// Determine a new protocol version for the next epoch if enough proposers voted
// otherwise keep the current one

let hpmn_list_len = last_committed_platform_state.hpmn_list_len() as u32;
let hpmn_active_list_len =
if self.config.network == Testnet && epoch_info.current_epoch_index() <= 1430 {
// We had a bug on testnet that would use the entire hpmn list len, including banned nodes
last_committed_platform_state.hpmn_list_len() as u32
} else {
last_committed_platform_state.hpmn_active_list_len() as u32
};

let next_epoch_protocol_version =
self.check_for_desired_protocol_upgrade(hpmn_list_len, platform_version)?;
self.check_for_desired_protocol_upgrade(hpmn_active_list_len, platform_version)?;

if let Some(protocol_version) = next_epoch_protocol_version {
tracing::trace!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,6 @@ impl BlockExecutionContextV0Getters for BlockExecutionContext {
}
}

fn hpmn_count(&self) -> u32 {
match self {
BlockExecutionContext::V0(v0) => v0.hpmn_count,
}
}

fn unsigned_withdrawal_transactions(&self) -> &UnsignedWithdrawalTxs {
match self {
BlockExecutionContext::V0(v0) => &v0.unsigned_withdrawal_transactions,
Expand Down Expand Up @@ -80,12 +74,6 @@ impl BlockExecutionContextV0Setters for BlockExecutionContext {
}
}

fn set_hpmn_count(&mut self, count: u32) {
match self {
BlockExecutionContext::V0(v0) => v0.hpmn_count = count,
}
}

fn set_unsigned_withdrawal_transactions(&mut self, transactions: UnsignedWithdrawalTxs) {
match self {
BlockExecutionContext::V0(v0) => v0.unsigned_withdrawal_transactions = transactions,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ pub struct BlockExecutionContextV0 {
pub block_state_info: BlockStateInfo,
/// Epoch info
pub epoch_info: EpochInfo,
/// Total hpmn count
pub hpmn_count: u32,
/// Unsigned withdrawal transactions to be available for extend and verify votes handlers
pub unsigned_withdrawal_transactions: UnsignedWithdrawalTxs,
/// Block state
Expand Down Expand Up @@ -137,11 +135,6 @@ impl BlockExecutionContextV0Getters for BlockExecutionContextV0 {
&self.epoch_info
}

/// Returns the hpmn_count field.
fn hpmn_count(&self) -> u32 {
self.hpmn_count
}

/// Returns a reference to the unsigned withdrawal transactions
fn unsigned_withdrawal_transactions(&self) -> &UnsignedWithdrawalTxs {
&self.unsigned_withdrawal_transactions
Expand All @@ -167,10 +160,6 @@ impl BlockExecutionContextV0Setters for BlockExecutionContextV0 {
fn set_epoch_info(&mut self, info: EpochInfo) {
self.epoch_info = info;
}
/// Sets the hpmn_count field.
fn set_hpmn_count(&mut self, count: u32) {
self.hpmn_count = count;
}
/// Sets the withdrawal_transactions field.
fn set_unsigned_withdrawal_transactions(&mut self, transactions: UnsignedWithdrawalTxs) {
self.unsigned_withdrawal_transactions = transactions;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,6 @@ pub(in crate::execution) mod tests {
app_hash: None,
}),
epoch_info: EpochInfo::V0(EpochInfoV0::default()),
hpmn_count: 0,
unsigned_withdrawal_transactions: Default::default(),
block_platform_state: platform_state.clone(),
proposer_results: None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,12 @@ impl PlatformStateV0Methods for PlatformState {
}
}

fn hpmn_active_list_len(&self) -> usize {
match self {
PlatformState::V0(v0) => v0.hpmn_active_list_len(),
}
}

fn current_validator_set(&self) -> Result<&ValidatorSet, Error> {
match self {
PlatformState::V0(v0) => v0.current_validator_set(),
Expand Down
12 changes: 12 additions & 0 deletions packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use dpp::block::extended_block_info::v0::ExtendedBlockInfoV0Getters;
use dpp::version::{PlatformVersion, TryIntoPlatformVersioned};

use crate::config::PlatformConfig;
use crate::platform_types::platform_state::PlatformState;
use crate::platform_types::signature_verification_quorum_set::{
SignatureVerificationQuorumSet, SignatureVerificationQuorumSetForSaving,
};
Expand Down Expand Up @@ -453,6 +454,9 @@ pub trait PlatformStateV0Methods {
fn hpmn_masternode_list_changes(&self, previous: &Self) -> MasternodeListChanges
where
Self: Sized;

/// The size of the hpmn list that are currently not banned
fn hpmn_active_list_len(&self) -> usize;
}

impl PlatformStateV0PrivateMethods for PlatformStateV0 {
Expand Down Expand Up @@ -565,6 +569,14 @@ impl PlatformStateV0Methods for PlatformStateV0 {
self.hpmn_masternode_list.len()
}

/// HPMN active list len
fn hpmn_active_list_len(&self) -> usize {
self.hpmn_masternode_list
.values()
.filter(|masternode| masternode.state.pose_ban_height.is_none())
.count()
}

/// Get the current quorum
fn current_validator_set(&self) -> Result<&ValidatorSet, Error> {
self.validator_sets
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,6 @@ pub(crate) fn process_epoch_change(
previous_epoch_index: epoch_index.checked_sub(1),
is_epoch_change: true,
}),
// TODO: It doesn't seem correct to use previous block count of hpmns.
// We currently not using this field in the codebase. We probably should just remove it.
hpmn_count: 100,
unsigned_withdrawal_transactions: UnsignedWithdrawalTxs::default(),
block_platform_state: platform_state,
proposer_results: None,
Expand Down
Loading