Skip to content

Commit 3436867

Browse files
fix(ethexe-consensus): address review issues in base-announce-priority
- Remove accidental O(N²) debug chain-traversal block from initial.rs - Use saturating_sub(1) instead of - 1 in best_parent_announce/block_best_announce - Include announce hashes in double-announcement warning - Add docstring to best_announce noting it may return hash outside input set - Add warn! logging when announces_have_equal_outcomes has missing outcome - Add Ord/PartialOrd/Hash impls to WithHashOf<T> based on hash field Co-authored-by: Gregory Sobol <grishasobol@users.noreply.github.com>
1 parent 1554071 commit 3436867

3 files changed

Lines changed: 41 additions & 29 deletions

File tree

ethexe/common/src/hash.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,3 +222,21 @@ pub struct WithHashOf<T: 'static> {
222222
pub hash: HashOf<T>,
223223
pub value: T,
224224
}
225+
226+
impl<T> PartialOrd for WithHashOf<T> {
227+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
228+
Some(self.cmp(other))
229+
}
230+
}
231+
232+
impl<T> Ord for WithHashOf<T> {
233+
fn cmp(&self, other: &Self) -> Ordering {
234+
self.hash.cmp(&other.hash)
235+
}
236+
}
237+
238+
impl<T> Hash for WithHashOf<T> {
239+
fn hash<H: Hasher>(&self, state: &mut H) {
240+
self.hash.hash(state)
241+
}
242+
}

ethexe/consensus/src/announces.rs

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -645,7 +645,7 @@ pub fn best_parent_announce(
645645
// to be sure that we take only not expired parent announces.
646646
let candidates = db.announces_parents(announces)?;
647647

648-
best_announce(db, candidates, commitment_delay_limit - 1)
648+
best_announce(db, candidates, commitment_delay_limit.saturating_sub(1))
649649
}
650650

651651
pub fn block_best_announce(
@@ -664,7 +664,7 @@ pub fn block_best_announce(
664664
// to be sure that we take only not expired parent announces.
665665
let parent_announces = db.announces_parents(candidates.iter().cloned())?;
666666

667-
let best_parent = best_announce(db, parent_announces, commitment_delay_limit - 1)?;
667+
let best_parent = best_announce(db, parent_announces, commitment_delay_limit.saturating_sub(1))?;
668668

669669
// Find child announces
670670
let mut not_base_announce_hash = None;
@@ -675,8 +675,12 @@ pub fn block_best_announce(
675675
.ok_or_else(|| anyhow!("announce({candidate}) not found"))?;
676676

677677
if announce.parent == best_parent && !announce.is_base() {
678-
if not_base_announce_hash.is_some() {
679-
tracing::warn!("Found multiple not-base announces: maybe double announcement");
678+
if let Some(existing) = not_base_announce_hash {
679+
tracing::warn!(
680+
existing = %existing,
681+
new = %candidate,
682+
"Found multiple not-base announces: maybe double announcement"
683+
);
680684
} else {
681685
not_base_announce_hash = Some(candidate);
682686
}
@@ -705,6 +709,9 @@ pub fn block_best_announce(
705709
}
706710

707711
/// Returns announce hash, which is supposed to be best among provided announces.
712+
///
713+
/// Note: if the best candidate is a not-base announce that has an equal-outcome base sibling,
714+
/// the function may return that base sibling's hash, which is **not** in the input set.
708715
fn best_announce(
709716
db: &impl DBAnnouncesExt,
710717
announces: impl IntoIterator<Item = HashOf<Announce>>,
@@ -777,9 +784,18 @@ pub fn announces_have_equal_outcomes(
777784
announce1_hash: HashOf<Announce>,
778785
announce2_hash: HashOf<Announce>,
779786
) -> bool {
780-
db.announce_outcome(announce1_hash)
781-
.map(|base_outcome| Some(base_outcome) == db.announce_outcome(announce2_hash))
782-
.unwrap_or(false)
787+
let outcome1 = db.announce_outcome(announce1_hash);
788+
let outcome2 = db.announce_outcome(announce2_hash);
789+
if outcome1.is_none() || outcome2.is_none() {
790+
tracing::warn!(
791+
announce1 = %announce1_hash,
792+
announce2 = %announce2_hash,
793+
"announces_have_equal_outcomes: one or both outcomes not yet computed, \
794+
defaulting to not equal"
795+
);
796+
return false;
797+
}
798+
outcome1 == outcome2
783799
}
784800

785801
#[derive(Debug, Clone, PartialEq, Eq, derive_more::Display)]

ethexe/consensus/src/validator/initial.rs

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -223,28 +223,6 @@ impl Initial {
223223

224224
impl ValidatorContext {
225225
fn switch_to_producer_or_subordinate(self, block: SimpleBlockData) -> Result<ValidatorState> {
226-
// print chain
227-
{
228-
use ethexe_common::{
229-
HashOf,
230-
db::{AnnounceStorageRO, BlockMetaStorageRO},
231-
};
232-
233-
let db = &self.core.db;
234-
for mut head in db.block_meta(block.hash).announces.into_iter().flatten() {
235-
let mut chain_str = String::new();
236-
while head != HashOf::zero() {
237-
let announce = db.announce(head).unwrap();
238-
chain_str = format!(
239-
"{head:6}({}) <- {chain_str}",
240-
if announce.is_base() { "B" } else { "P" }
241-
);
242-
head = announce.parent;
243-
}
244-
tracing::info!(block = %block.hash, "Announces chain: {chain_str}");
245-
}
246-
}
247-
248226
let era_index = self.core.timelines.era_from_ts(block.header.timestamp);
249227
let validators = self
250228
.core

0 commit comments

Comments
 (0)