fix flaky test

tari-project · Jan 17, 2025 · 238c0e1 · 238c0e1
1 parent 5005be5
commit 238c0e1
Show file tree

Hide file tree

Showing 18 changed files with 110 additions and 79 deletions.
diff --git a/applications/tari_dan_app_utilities/src/base_layer_scanner.rs b/applications/tari_dan_app_utilities/src/base_layer_scanner.rs
@@ -48,13 +48,7 @@ use tari_crypto::{
     ristretto::RistrettoPublicKey,
     tari_utilities::{ByteArray, ByteArrayError},
 };
-use tari_dan_common_types::{
-    option::DisplayContainer,
-    optional::Optional,
-    Epoch,
-    NodeAddressable,
-    VersionedSubstateId,
-};
+use tari_dan_common_types::{option::Displayable, optional::Optional, Epoch, NodeAddressable, VersionedSubstateId};
 use tari_dan_storage::{
     consensus_models::{BurntUtxo, SubstateRecord},
     global::{GlobalDb, MetadataKey},

diff --git a/applications/tari_validator_node_cli/src/command/transaction.rs b/applications/tari_validator_node_cli/src/command/transaction.rs
@@ -30,7 +30,7 @@ use std::{
 use anyhow::anyhow;
 use clap::{Args, Subcommand};
 use tari_dan_common_types::{
-    option::{DisplayCont, DisplayContainer},
+    option::{DisplayContainer, Displayable},
     optional::Optional,
     SubstateAddress,
     SubstateRequirement,
@@ -498,7 +498,7 @@ fn summarize_finalize_result(finalize: &FinalizeResult) {
 }
 
 fn display_vec<W: fmt::Write>(writer: &mut W, ty: &Type, result: &InstructionResult) -> fmt::Result {
-    fn display_slice<T: fmt::Display>(slice: &[T]) -> DisplayCont<&[T]> {
+    fn display_slice<T: fmt::Display>(slice: &[T]) -> DisplayContainer<&[T]> {
         slice.display()
     }
 

diff --git a/dan_layer/common_types/src/option.rs b/dan_layer/common_types/src/option.rs
@@ -2,7 +2,7 @@
 //   SPDX-License-Identifier: BSD-3-Clause
 
 use std::{
-    collections::{BTreeSet, HashSet},
+    collections::{BTreeSet, HashMap, HashSet},
     fmt,
     fmt::{Debug, Display},
 };
@@ -12,7 +12,7 @@ use std::{
 ///
 /// # Example
 /// ```rust
-/// use tari_dan_common_types::option::DisplayContainer;
+/// use tari_dan_common_types::option::Displayable;
 ///
 /// let some_value = Some(42);
 /// let none_value: Option<i32> = None;
@@ -34,17 +34,17 @@ use std::{
 ///     "list: 1.01, 2.00, 3.00"
 /// );
 /// ```
-pub trait DisplayContainer {
+pub trait Displayable {
     type Item: ?Sized;
-    fn display(&self) -> DisplayCont<&'_ Self::Item>;
+    fn display(&self) -> DisplayContainer<&'_ Self::Item>;
 }
 
 #[derive(Debug, Clone, Copy)]
-pub struct DisplayCont<T> {
+pub struct DisplayContainer<T> {
     value: T,
 }
 
-impl<T: Display> Display for DisplayCont<&'_ Option<T>> {
+impl<T: Display> Display for DisplayContainer<&'_ Option<T>> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self.value {
             Some(value) => Display::fmt(value, f),
@@ -53,81 +53,110 @@ impl<T: Display> Display for DisplayCont<&'_ Option<T>> {
     }
 }
 
-impl<T: Display> Display for DisplayCont<&'_ [T]> {
+impl<T: Display> Display for DisplayContainer<&'_ [T]> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let len = self.value.len();
+        write!(f, "[")?;
         for (i, item) in self.value.iter().enumerate() {
             Display::fmt(item, f)?;
             if i < len - 1 {
                 write!(f, ", ")?;
             }
         }
+        write!(f, "]")?;
         Ok(())
     }
 }
 
-impl<T: Display> Display for DisplayCont<&'_ HashSet<T>> {
+impl<T: Display> Display for DisplayContainer<&'_ HashSet<T>> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let len = self.value.len();
+        write!(f, "{{")?;
         for (i, item) in self.value.iter().enumerate() {
             Display::fmt(item, f)?;
             if i < len - 1 {
                 write!(f, ", ")?;
             }
         }
+        write!(f, "}}")?;
         Ok(())
     }
 }
 
-impl<T: Display> Display for DisplayCont<&'_ BTreeSet<T>> {
+impl<T: Display> Display for DisplayContainer<&'_ BTreeSet<T>> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let len = self.value.len();
+        write!(f, "{{")?;
         for (i, item) in self.value.iter().enumerate() {
             Display::fmt(item, f)?;
             if i < len - 1 {
                 write!(f, ", ")?;
             }
         }
+        write!(f, "}}")?;
         Ok(())
     }
 }
 
-impl<T: Display> DisplayContainer for Option<T> {
+impl<K: Display, V: Display> Display for DisplayContainer<&'_ HashMap<K, V>> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let len = self.value.len();
+        write!(f, "{{")?;
+        for (i, (k, v)) in self.value.iter().enumerate() {
+            write!(f, "{k}: {v}")?;
+            if i < len - 1 {
+                write!(f, ", ")?;
+            }
+        }
+        write!(f, "}}")?;
+        Ok(())
+    }
+}
+
+impl<T: Display> Displayable for Option<T> {
     type Item = Self;
 
-    fn display(&self) -> DisplayCont<&'_ Self> {
-        DisplayCont { value: self }
+    fn display(&self) -> DisplayContainer<&'_ Self> {
+        DisplayContainer { value: self }
     }
 }
 
-impl<T: Display> DisplayContainer for [T] {
+impl<T: Display> Displayable for [T] {
     type Item = Self;
 
-    fn display(&self) -> DisplayCont<&'_ Self> {
-        DisplayCont { value: self }
+    fn display(&self) -> DisplayContainer<&'_ Self> {
+        DisplayContainer { value: self }
     }
 }
 
-impl<T: Display> DisplayContainer for Vec<T> {
+impl<T: Display> Displayable for Vec<T> {
     type Item = [T];
 
-    fn display(&self) -> DisplayCont<&'_ [T]> {
+    fn display(&self) -> DisplayContainer<&'_ [T]> {
         (*self.as_slice()).display()
     }
 }
 
-impl<T: Display> DisplayContainer for HashSet<T> {
+impl<T: Display> Displayable for HashSet<T> {
+    type Item = Self;
+
+    fn display(&self) -> DisplayContainer<&'_ Self> {
+        DisplayContainer { value: self }
+    }
+}
+
+impl<T: Display> Displayable for BTreeSet<T> {
     type Item = Self;
 
-    fn display(&self) -> DisplayCont<&'_ Self> {
-        DisplayCont { value: self }
+    fn display(&self) -> DisplayContainer<&'_ Self> {
+        DisplayContainer { value: self }
     }
 }
 
-impl<T: Display> DisplayContainer for BTreeSet<T> {
+impl<K: Display, V: Display> Displayable for HashMap<K, V> {
     type Item = Self;
 
-    fn display(&self) -> DisplayCont<&'_ Self> {
-        DisplayCont { value: self }
+    fn display(&self) -> DisplayContainer<&'_ Self> {
+        DisplayContainer { value: self }
     }
 }
diff --git a/dan_layer/consensus/src/hotstuff/block_change_set.rs b/dan_layer/consensus/src/hotstuff/block_change_set.rs
@@ -10,7 +10,7 @@ use std::{
 use indexmap::IndexMap;
 use log::*;
 use tari_common_types::types::PublicKey;
-use tari_dan_common_types::{option::DisplayContainer, optional::Optional, shard::Shard, Epoch, ShardGroup};
+use tari_dan_common_types::{option::Displayable, optional::Optional, shard::Shard, Epoch, ShardGroup};
 use tari_dan_storage::{
     consensus_models::{
         Block,

diff --git a/dan_layer/consensus/src/hotstuff/foreign_proposal_processor.rs b/dan_layer/consensus/src/hotstuff/foreign_proposal_processor.rs
@@ -2,7 +2,7 @@
 //   SPDX-License-Identifier: BSD-3-Clause
 
 use log::*;
-use tari_dan_common_types::{committee::CommitteeInfo, option::DisplayContainer, ShardGroup, SubstateAddress};
+use tari_dan_common_types::{committee::CommitteeInfo, option::Displayable, ShardGroup, SubstateAddress};
 use tari_dan_storage::{
     consensus_models::{
         BlockPledge,

diff --git a/dan_layer/consensus/src/hotstuff/on_next_sync_view.rs b/dan_layer/consensus/src/hotstuff/on_next_sync_view.rs
@@ -2,7 +2,7 @@
 //  SPDX-License-Identifier: BSD-3-Clause
 
 use log::*;
-use tari_dan_common_types::{committee::Committee, option::DisplayContainer, optional::Optional, Epoch, NodeHeight};
+use tari_dan_common_types::{committee::Committee, option::Displayable, optional::Optional, Epoch, NodeHeight};
 use tari_dan_storage::{
     consensus_models::{HighQc, LastSentVote, LeafBlock},
     StateStore,

diff --git a/dan_layer/consensus/src/hotstuff/on_propose.rs b/dan_layer/consensus/src/hotstuff/on_propose.rs
@@ -12,7 +12,7 @@ use tari_common_types::types::{FixedHash, PublicKey};
 use tari_crypto::tari_utilities::epoch_time::EpochTime;
 use tari_dan_common_types::{
     committee::{Committee, CommitteeInfo},
-    option::DisplayContainer,
+    option::Displayable,
     optional::Optional,
     shard::Shard,
     Epoch,

diff --git a/dan_layer/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs b/dan_layer/consensus/src/hotstuff/on_ready_to_vote_on_local_block.rs
@@ -7,7 +7,7 @@ use log::*;
 use tari_crypto::ristretto::RistrettoPublicKey;
 use tari_dan_common_types::{
     committee::CommitteeInfo,
-    option::DisplayContainer,
+    option::Displayable,
     optional::Optional,
     Epoch,
     ShardGroup,

diff --git a/dan_layer/consensus/src/hotstuff/on_receive_foreign_proposal.rs b/dan_layer/consensus/src/hotstuff/on_receive_foreign_proposal.rs
@@ -4,13 +4,7 @@
 use std::collections::HashSet;
 
 use log::*;
-use tari_dan_common_types::{
-    committee::CommitteeInfo,
-    option::DisplayContainer,
-    optional::Optional,
-    Epoch,
-    ShardGroup,
-};
+use tari_dan_common_types::{committee::CommitteeInfo, option::Displayable, optional::Optional, Epoch, ShardGroup};
 use tari_dan_storage::{
     consensus_models::{
         Block,

diff --git a/dan_layer/consensus/src/hotstuff/on_receive_request_missing_transactions.rs b/dan_layer/consensus/src/hotstuff/on_receive_request_missing_transactions.rs
@@ -2,7 +2,7 @@
 //   SPDX-License-Identifier: BSD-3-Clause
 
 use log::*;
-use tari_dan_common_types::option::DisplayContainer;
+use tari_dan_common_types::option::Displayable;
 use tari_dan_storage::{consensus_models::TransactionRecord, StateStore};
 
 use crate::{

diff --git a/dan_layer/consensus/src/hotstuff/substate_store/pending_store.rs b/dan_layer/consensus/src/hotstuff/substate_store/pending_store.rs
@@ -6,6 +6,7 @@ use std::{borrow::Cow, collections::HashMap, fmt::Display};
 use indexmap::IndexMap;
 use log::*;
 use tari_dan_common_types::{
+    option::Displayable,
     optional::Optional,
     LockIntent,
     NumPreshards,
@@ -531,7 +532,7 @@ impl<'a, 'tx, TStore: StateStore + 'a + 'tx> PendingSubstateStore<'a, 'tx, TStor
     fn get_pending(&self, addr: &SubstateAddress) -> Option<&SubstateChange> {
         self.pending
             .get(addr)
-            .map(|&pos| self.diff.get(pos).expect("Index map and diff are out of sync"))
+            .map(|&pos| self.diff.get(pos).expect("pending map and diff are out of sync"))
     }
 
     fn insert(&mut self, change: SubstateChange) {
@@ -565,22 +566,41 @@ impl<'a, 'tx, TStore: StateStore + 'a + 'tx> PendingSubstateStore<'a, 'tx, TStor
     }
 
     fn assert_is_up(&self, id: &VersionedSubstateId) -> Result<(), SubstateStoreError> {
+        debug!(
+            target: LOG_TARGET,
+            "assert_is_up: id: {}, pending: {}, head: {}",
+            id,
+            self.pending.display(),
+            self.head.display()
+        );
         if let Some(change) = self.get_pending(&id.to_substate_address()) {
             if change.is_down() {
                 return Err(SubstateStoreError::SubstateIsDown { id: id.clone() });
             }
             return Ok(());
         }
 
+        debug!(
+            target: LOG_TARGET,
+            "assert_is_up id: {} not found in pending",
+            id,
+        );
+
         if let Some(change) =
-            BlockDiff::get_for_substate(self.read_transaction(), &self.parent_block, &id.substate_id).optional()?
+            BlockDiff::get_for_substate(self.read_transaction(), &self.parent_block, id.substate_id()).optional()?
         {
             if change.is_up() {
                 return Ok(());
             }
             return Err(SubstateStoreError::SubstateIsDown { id: id.clone() });
         }
 
+        debug!(
+            target: LOG_TARGET,
+            "assert_is_up: id: {} not found in block diff",
+            id,
+        );
+
         match SubstateRecord::substate_is_up(self.read_transaction(), &id.to_substate_address()).optional()? {
             Some(true) => Ok(()),
             Some(false) => Err(SubstateStoreError::SubstateIsDown { id: id.clone() }),

diff --git a/dan_layer/consensus_tests/src/consensus.rs b/dan_layer/consensus_tests/src/consensus.rs
@@ -748,11 +748,11 @@ async fn multishard_output_conflict_abort() {
     test.assert_all_validators_at_same_height().await;
     // Currently not deterministic (test harness) which transaction will arrive first so we check that one transaction
     // is committed and the other is aborted. TODO: It is also possible that both are aborted.
-    let tx1_vn1 = test.get_transaction_record(&TestAddress::new("1"), tx_ids[0]);
-    let tx2_vn1 = test.get_transaction_record(&TestAddress::new("1"), tx_ids[1]);
+    let tx1_vn1 = test.get_validator(&TestAddress::new("1")).get_transaction(tx_ids[0]);
+    let tx2_vn1 = test.get_validator(&TestAddress::new("1")).get_transaction(tx_ids[1]);
 
-    let tx1_vn3 = test.get_transaction_record(&TestAddress::new("3"), tx_ids[0]);
-    let tx2_vn3 = test.get_transaction_record(&TestAddress::new("3"), tx_ids[1]);
+    let tx1_vn3 = test.get_validator(&TestAddress::new("3")).get_transaction(tx_ids[0]);
+    let tx2_vn3 = test.get_validator(&TestAddress::new("3")).get_transaction(tx_ids[1]);
 
     assert_eq!(tx1_vn1.final_decision().unwrap(), tx1_vn3.final_decision().unwrap());
     assert_eq!(tx2_vn1.final_decision().unwrap(), tx2_vn3.final_decision().unwrap());
@@ -813,22 +813,19 @@ async fn single_shard_inputs_from_previous_outputs() {
     }
 
     test.assert_all_validators_at_same_height().await;
-    // We do not work out input dependencies when we sequence transactions in blocks. Currently ordering within a block
-    // is lexicographical by transaction id, therefore both will only be committed if tx1 happens to be sequenced
-    // first.
-    // if tx1.id() < tx2.id() {
+    // Assert that the decision matches for all validators. If tx2 is sequenced first, then it will be aborted due to
+    // the input not existing
     test.assert_all_validators_have_decision(tx1.id(), Decision::Commit)
         .await;
-    test.assert_all_validators_have_decision(tx2.id(), Decision::Commit)
-        .await;
-    // TODO: this is no longer true - it always commits both. Need to confirm correctness because it may be that the
-    //       implementation is more intelligent (correct sequencing or downgrading to a read lock) but not certain.
-    // } else {
-    //     test.assert_all_validators_have_decision(tx1.id(), Decision::Commit)
-    //         .await;
-    //     test.assert_all_validators_have_decision(tx2.id(), Decision::Abort(AbortReason::OneOrMoreInputsNotFound))
-    //         .await;
-    // }
+    let decision_tx2 = test
+        .get_validator(&TestAddress::new("1"))
+        .get_transaction(tx2.id())
+        .final_decision()
+        .expect("tx2 final decision not reached");
+    test.assert_all_validators_have_decision(tx2.id(), decision_tx2).await;
+    if let Some(reason) = decision_tx2.abort_reason() {
+        assert_eq!(reason, AbortReason::OneOrMoreInputsNotFound);
+    }
 
     test.assert_clean_shutdown().await;
     log::info!("total messages sent: {}", test.network().total_messages_sent());

diff --git a/dan_layer/consensus_tests/src/support/harness.rs b/dan_layer/consensus_tests/src/support/harness.rs
@@ -214,15 +214,6 @@ impl Test {
         &self.validators
     }
 
-    pub fn get_transaction_record(&self, address: &TestAddress, transaction_id: &TransactionId) -> TransactionRecord {
-        self.validators
-            .get(address)
-            .unwrap()
-            .state_store
-            .with_read_tx(|tx| TransactionRecord::get(tx, transaction_id))
-            .unwrap()
-    }
-
     pub async fn on_hotstuff_event(&mut self) -> (TestAddress, HotstuffEvent) {
         if self.network.task_handle().is_finished() {
             panic!("Network task exited while waiting for Hotstuff event");