From 9b6af2c63a612b6d40fd815041443c3b6e688f81 Mon Sep 17 00:00:00 2001 From: mrnaveira <47919901+mrnaveira@users.noreply.github.com> Date: Thu, 29 Feb 2024 22:32:59 +0000 Subject: [PATCH 1/5] min_block_time config parameter --- applications/tari_validator_node/src/config.rs | 3 +++ integration_tests/src/validator_node.rs | 6 +++++- integration_tests/tests/cucumber.rs | 9 ++++++++- integration_tests/tests/features/concurrency.feature | 2 +- integration_tests/tests/steps/validator_node.rs | 5 ++--- 5 files changed, 19 insertions(+), 6 deletions(-) diff --git a/applications/tari_validator_node/src/config.rs b/applications/tari_validator_node/src/config.rs index 6ab4e09b1..c711db3d4 100644 --- a/applications/tari_validator_node/src/config.rs +++ b/applications/tari_validator_node/src/config.rs @@ -106,6 +106,8 @@ pub struct ValidatorNodeConfig { pub fee_claim_public_key: RistrettoPublicKey, /// Create identity file if not exists pub dont_create_id: bool, + /// Minimum time between blocks + pub min_block_time: Duration, } impl ValidatorNodeConfig { @@ -149,6 +151,7 @@ impl Default for ValidatorNodeConfig { // Burn your fees fee_claim_public_key: RistrettoPublicKey::default(), dont_create_id: false, + min_block_time: Duration::ZERO, } } } diff --git a/integration_tests/src/validator_node.rs b/integration_tests/src/validator_node.rs index 672457bba..b3a8c6d5f 100644 --- a/integration_tests/src/validator_node.rs +++ b/integration_tests/src/validator_node.rs @@ -24,7 +24,7 @@ use std::{ fs, path::{Path, PathBuf}, }; - +use core::time::Duration; use reqwest::Url; use tari_common::{ configuration::{CommonConfig, StringList}, @@ -74,6 +74,7 @@ pub async fn spawn_validator_node( validator_node_name: String, base_node_name: String, wallet_name: String, + min_block_time: Duration, ) -> ValidatorNodeProcess { // each spawned VN will use different ports let (port, json_rpc_port) = get_os_assigned_ports(); @@ -126,6 +127,9 @@ pub async fn spawn_validator_node( // The VNS will try to auto register upon startup config.validator_node.auto_register = false; + // Minimu block time (to allow testing concurrent transactions in the same block) + config.validator_node.min_block_time = min_block_time; + // Add all other VNs as peer seeds config.peer_seeds.peer_seeds = StringList::from(peer_seeds); run_validator_node(&config, shutdown_signal).await diff --git a/integration_tests/tests/cucumber.rs b/integration_tests/tests/cucumber.rs index 4b1531664..cc093e1d6 100644 --- a/integration_tests/tests/cucumber.rs +++ b/integration_tests/tests/cucumber.rs @@ -98,7 +98,14 @@ async fn fees_are_enabled(world: &mut TariWorld) { #[given(expr = "a validator node {word} connected to base node {word} and wallet {word}")] async fn start_validator_node(world: &mut TariWorld, vn_name: String, bn_name: String, wallet_name: String) { - let vn = spawn_validator_node(world, vn_name.clone(), bn_name, wallet_name).await; + let vn = spawn_validator_node(world, vn_name.clone(), bn_name, wallet_name, Duration::ZERO).await; + world.validator_nodes.insert(vn_name, vn); +} + +#[given(expr = "a validator node {word} with minimum block time {int} seconds connected to base node {word} and wallet {word}")] +async fn start_validator_node_block_time(world: &mut TariWorld, vn_name: String, min_block_time: usize, bn_name: String, wallet_name: String) { + let min_block_time = Duration::from_secs(min_block_time.try_into().unwrap()); + let vn = spawn_validator_node(world, vn_name.clone(), bn_name, wallet_name, min_block_time).await; world.validator_nodes.insert(vn_name, vn); } diff --git a/integration_tests/tests/features/concurrency.feature b/integration_tests/tests/features/concurrency.feature index a702b5814..ca5dc5d0d 100644 --- a/integration_tests/tests/features/concurrency.feature +++ b/integration_tests/tests/features/concurrency.feature @@ -12,7 +12,7 @@ Feature: Concurrency Given a miner MINER connected to base node BASE and wallet WALLET # Initialize a VN - Given a validator node VAL_1 connected to base node BASE and wallet WALLET + Given a validator node VAL_1 with minimum block time 5 seconds connected to base node BASE and wallet WALLET # The wallet must have some funds before the VN sends transactions When miner MINER mines 6 new blocks diff --git a/integration_tests/tests/steps/validator_node.rs b/integration_tests/tests/steps/validator_node.rs index da2c04e59..bf7169699 100644 --- a/integration_tests/tests/steps/validator_node.rs +++ b/integration_tests/tests/steps/validator_node.rs @@ -5,7 +5,6 @@ use std::{ str::FromStr, time::{Duration, Instant}, }; - use cucumber::{given, then, when}; use integration_tests::{ base_node::get_base_node_client, @@ -24,7 +23,7 @@ use tari_validator_node_client::types::{AddPeerRequest, GetStateRequest, GetTemp #[given(expr = "a seed validator node {word} connected to base node {word} and wallet {word}")] async fn start_seed_validator_node(world: &mut TariWorld, seed_vn_name: String, bn_name: String, wallet_name: String) { - let validator = spawn_validator_node(world, seed_vn_name.clone(), bn_name, wallet_name).await; + let validator = spawn_validator_node(world, seed_vn_name.clone(), bn_name, wallet_name, Duration::ZERO).await; // Ensure any existing nodes know about the new seed node let mut client = validator.get_client(); let ident = client.get_identity().await.unwrap(); @@ -58,7 +57,7 @@ async fn start_seed_validator_node(world: &mut TariWorld, seed_vn_name: String, async fn start_multiple_validator_nodes(world: &mut TariWorld, num_nodes: u64, bn_name: String, wallet_name: String) { for i in 1..=num_nodes { let vn_name = format!("VAL_{i}"); - let vn = spawn_validator_node(world, vn_name.clone(), bn_name.clone(), wallet_name.clone()).await; + let vn = spawn_validator_node(world, vn_name.clone(), bn_name.clone(), wallet_name.clone(), Duration::ZERO).await; world.validator_nodes.insert(vn_name, vn); } } From 72f9b1b740f54514d644ee414ad2910a7b80e6c8 Mon Sep 17 00:00:00 2001 From: mrnaveira <47919901+mrnaveira@users.noreply.github.com> Date: Thu, 7 Mar 2024 21:11:42 +0000 Subject: [PATCH 2/5] avoid db filtering txs with no versions --- .../consensus_models/executed_transaction.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/dan_layer/storage/src/consensus_models/executed_transaction.rs b/dan_layer/storage/src/consensus_models/executed_transaction.rs index 4bb79bb6d..f3e9c934b 100644 --- a/dan_layer/storage/src/consensus_models/executed_transaction.rs +++ b/dan_layer/storage/src/consensus_models/executed_transaction.rs @@ -146,28 +146,37 @@ impl ExecutedTransaction { pub fn to_initial_evidence(&self) -> Evidence { let mut deduped_evidence = HashMap::new(); - deduped_evidence.extend(self.transaction.inputs().iter().map(|input| { + deduped_evidence.extend(self.transaction + .inputs() + .iter() + .filter(|i| i.version().is_some()) + .map(|input| { (input.to_substate_address(), ShardEvidence { qc_ids: IndexSet::new(), lock: LockFlag::Write, }) })); - deduped_evidence.extend(self.transaction.input_refs().iter().map(|input_ref| { + deduped_evidence.extend(self.transaction.input_refs().iter() + .filter(|i| i.version().is_some()) + .map(|input_ref| { (input_ref.to_substate_address(), ShardEvidence { qc_ids: IndexSet::new(), lock: LockFlag::Read, }) })); - deduped_evidence.extend(self.transaction.filled_inputs().iter().map(|input_ref| { + deduped_evidence.extend(self.transaction.filled_inputs().iter() + .filter(|i| i.version().is_some()) + .map(|input_ref| { (input_ref.to_substate_address(), ShardEvidence { qc_ids: IndexSet::new(), lock: LockFlag::Write, }) })); - deduped_evidence.extend(self.resulting_outputs.iter().map(|output| { + deduped_evidence.extend(self.resulting_outputs.iter() + .map(|output| { (*output, ShardEvidence { qc_ids: IndexSet::new(), lock: LockFlag::Write, From dea0db8171917f0ddf44385ba12cbb451186911e Mon Sep 17 00:00:00 2001 From: mrnaveira <47919901+mrnaveira@users.noreply.github.com> Date: Tue, 12 Mar 2024 11:44:47 +0000 Subject: [PATCH 3/5] remove min_block_time --- applications/tari_validator_node/src/config.rs | 3 --- .../storage/src/consensus_models/executed_transaction.rs | 2 ++ integration_tests/src/validator_node.rs | 5 ----- integration_tests/tests/cucumber.rs | 9 +-------- integration_tests/tests/features/concurrency.feature | 2 +- integration_tests/tests/steps/validator_node.rs | 4 ++-- 6 files changed, 6 insertions(+), 19 deletions(-) diff --git a/applications/tari_validator_node/src/config.rs b/applications/tari_validator_node/src/config.rs index c711db3d4..6ab4e09b1 100644 --- a/applications/tari_validator_node/src/config.rs +++ b/applications/tari_validator_node/src/config.rs @@ -106,8 +106,6 @@ pub struct ValidatorNodeConfig { pub fee_claim_public_key: RistrettoPublicKey, /// Create identity file if not exists pub dont_create_id: bool, - /// Minimum time between blocks - pub min_block_time: Duration, } impl ValidatorNodeConfig { @@ -151,7 +149,6 @@ impl Default for ValidatorNodeConfig { // Burn your fees fee_claim_public_key: RistrettoPublicKey::default(), dont_create_id: false, - min_block_time: Duration::ZERO, } } } diff --git a/dan_layer/storage/src/consensus_models/executed_transaction.rs b/dan_layer/storage/src/consensus_models/executed_transaction.rs index f3e9c934b..28320f1a7 100644 --- a/dan_layer/storage/src/consensus_models/executed_transaction.rs +++ b/dan_layer/storage/src/consensus_models/executed_transaction.rs @@ -145,7 +145,9 @@ impl ExecutedTransaction { } pub fn to_initial_evidence(&self) -> Evidence { + // Note that we only add evidence for inputs that have specific version numbers let mut deduped_evidence = HashMap::new(); + deduped_evidence.extend(self.transaction .inputs() .iter() diff --git a/integration_tests/src/validator_node.rs b/integration_tests/src/validator_node.rs index b3a8c6d5f..b83eb4afd 100644 --- a/integration_tests/src/validator_node.rs +++ b/integration_tests/src/validator_node.rs @@ -24,7 +24,6 @@ use std::{ fs, path::{Path, PathBuf}, }; -use core::time::Duration; use reqwest::Url; use tari_common::{ configuration::{CommonConfig, StringList}, @@ -74,7 +73,6 @@ pub async fn spawn_validator_node( validator_node_name: String, base_node_name: String, wallet_name: String, - min_block_time: Duration, ) -> ValidatorNodeProcess { // each spawned VN will use different ports let (port, json_rpc_port) = get_os_assigned_ports(); @@ -127,9 +125,6 @@ pub async fn spawn_validator_node( // The VNS will try to auto register upon startup config.validator_node.auto_register = false; - // Minimu block time (to allow testing concurrent transactions in the same block) - config.validator_node.min_block_time = min_block_time; - // Add all other VNs as peer seeds config.peer_seeds.peer_seeds = StringList::from(peer_seeds); run_validator_node(&config, shutdown_signal).await diff --git a/integration_tests/tests/cucumber.rs b/integration_tests/tests/cucumber.rs index cc093e1d6..4b1531664 100644 --- a/integration_tests/tests/cucumber.rs +++ b/integration_tests/tests/cucumber.rs @@ -98,14 +98,7 @@ async fn fees_are_enabled(world: &mut TariWorld) { #[given(expr = "a validator node {word} connected to base node {word} and wallet {word}")] async fn start_validator_node(world: &mut TariWorld, vn_name: String, bn_name: String, wallet_name: String) { - let vn = spawn_validator_node(world, vn_name.clone(), bn_name, wallet_name, Duration::ZERO).await; - world.validator_nodes.insert(vn_name, vn); -} - -#[given(expr = "a validator node {word} with minimum block time {int} seconds connected to base node {word} and wallet {word}")] -async fn start_validator_node_block_time(world: &mut TariWorld, vn_name: String, min_block_time: usize, bn_name: String, wallet_name: String) { - let min_block_time = Duration::from_secs(min_block_time.try_into().unwrap()); - let vn = spawn_validator_node(world, vn_name.clone(), bn_name, wallet_name, min_block_time).await; + let vn = spawn_validator_node(world, vn_name.clone(), bn_name, wallet_name).await; world.validator_nodes.insert(vn_name, vn); } diff --git a/integration_tests/tests/features/concurrency.feature b/integration_tests/tests/features/concurrency.feature index ca5dc5d0d..a702b5814 100644 --- a/integration_tests/tests/features/concurrency.feature +++ b/integration_tests/tests/features/concurrency.feature @@ -12,7 +12,7 @@ Feature: Concurrency Given a miner MINER connected to base node BASE and wallet WALLET # Initialize a VN - Given a validator node VAL_1 with minimum block time 5 seconds connected to base node BASE and wallet WALLET + Given a validator node VAL_1 connected to base node BASE and wallet WALLET # The wallet must have some funds before the VN sends transactions When miner MINER mines 6 new blocks diff --git a/integration_tests/tests/steps/validator_node.rs b/integration_tests/tests/steps/validator_node.rs index bf7169699..e0a671cff 100644 --- a/integration_tests/tests/steps/validator_node.rs +++ b/integration_tests/tests/steps/validator_node.rs @@ -23,7 +23,7 @@ use tari_validator_node_client::types::{AddPeerRequest, GetStateRequest, GetTemp #[given(expr = "a seed validator node {word} connected to base node {word} and wallet {word}")] async fn start_seed_validator_node(world: &mut TariWorld, seed_vn_name: String, bn_name: String, wallet_name: String) { - let validator = spawn_validator_node(world, seed_vn_name.clone(), bn_name, wallet_name, Duration::ZERO).await; + let validator = spawn_validator_node(world, seed_vn_name.clone(), bn_name, wallet_name).await; // Ensure any existing nodes know about the new seed node let mut client = validator.get_client(); let ident = client.get_identity().await.unwrap(); @@ -57,7 +57,7 @@ async fn start_seed_validator_node(world: &mut TariWorld, seed_vn_name: String, async fn start_multiple_validator_nodes(world: &mut TariWorld, num_nodes: u64, bn_name: String, wallet_name: String) { for i in 1..=num_nodes { let vn_name = format!("VAL_{i}"); - let vn = spawn_validator_node(world, vn_name.clone(), bn_name.clone(), wallet_name.clone(), Duration::ZERO).await; + let vn = spawn_validator_node(world, vn_name.clone(), bn_name.clone(), wallet_name.clone()).await; world.validator_nodes.insert(vn_name, vn); } } From 791df9a72d60f9789334003e7bdb59813e6a2e1a Mon Sep 17 00:00:00 2001 From: mrnaveira <47919901+mrnaveira@users.noreply.github.com> Date: Tue, 12 Mar 2024 14:04:41 +0000 Subject: [PATCH 4/5] count distinct shards for tx with no versions --- dan_layer/consensus/src/hotstuff/on_propose.rs | 13 ++++++++++++- .../src/hotstuff/on_ready_to_vote_on_local_block.rs | 11 +++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/dan_layer/consensus/src/hotstuff/on_propose.rs b/dan_layer/consensus/src/hotstuff/on_propose.rs index bf48aaa69..61c3927c7 100644 --- a/dan_layer/consensus/src/hotstuff/on_propose.rs +++ b/dan_layer/consensus/src/hotstuff/on_propose.rs @@ -230,7 +230,18 @@ where TConsensusSpec: ConsensusSpec // prepared. We can now propose to Accept it. We also propose the decision change which everyone // should agree with if they received the same foreign LocalPrepare. TransactionPoolStage::LocalPrepared => { - let involved = local_committee_shard.count_distinct_shards(t.transaction().evidence.shards_iter()); + // For now we need to treat transactions without versions in a special case + // TODO: update the evidence after execution so all transactions are treated equally here + let db_transaction = t.get_transaction(tx)?; + let involved = if db_transaction.transaction().has_inputs_without_version() { + db_transaction + .transaction() + .all_inputs_iter() + .count() + } else { + local_committee_shard.count_distinct_shards(t.transaction().evidence.shards_iter()) + }; + let involved = NonZeroU64::new(involved as u64).ok_or_else(|| { HotStuffError::InvariantError(format!( "Number of involved shards is zero for transaction {}", 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 index 0ba03ca53..f2bcddcf7 100644 --- 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 @@ -630,8 +630,15 @@ where TConsensusSpec: ConsensusSpec return Ok(None); } - let distinct_shards = - local_committee_shard.count_distinct_shards(tx_rec.transaction().evidence.shards_iter()); + // For now we need to treat transactions without versions in a special case + // TODO: update the evidence after execution so all transactions are treated equally here + let executed = self.get_executed_transaction(tx, &t.id, &mut executor)?; + let transaction = executed.transaction(); + let distinct_shards = if transaction.has_inputs_without_version() { + transaction.all_inputs_iter().count() + } else { + local_committee_shard.count_distinct_shards(tx_rec.transaction().evidence.shards_iter()) + }; let distinct_shards = NonZeroU64::new(distinct_shards as u64).ok_or_else(|| { HotStuffError::InvariantError(format!( "Distinct shards is zero for transaction {} in block {}", From 793229af0b3a959656dfb1224941da5ff846b224 Mon Sep 17 00:00:00 2001 From: mrnaveira <47919901+mrnaveira@users.noreply.github.com> Date: Tue, 12 Mar 2024 14:47:10 +0000 Subject: [PATCH 5/5] fix fmt --- .../src/services/account_monitor.rs | 16 ++--- .../consensus/src/hotstuff/on_propose.rs | 5 +- .../consensus_models/executed_transaction.rs | 71 +++++++++++-------- integration_tests/src/validator_node.rs | 1 + .../tests/steps/validator_node.rs | 1 + 5 files changed, 51 insertions(+), 43 deletions(-) diff --git a/applications/tari_dan_wallet_daemon/src/services/account_monitor.rs b/applications/tari_dan_wallet_daemon/src/services/account_monitor.rs index c3a663bdd..31309fe45 100644 --- a/applications/tari_dan_wallet_daemon/src/services/account_monitor.rs +++ b/applications/tari_dan_wallet_daemon/src/services/account_monitor.rs @@ -185,14 +185,14 @@ where .await .optional()?; let Some(ValidatorScanResult { - address: versioned_addr, - substate, - created_by_tx, - }) = scan_result - else { - warn!(target: LOG_TARGET, "Vault {} for account {} does not exist according to validator node", vault_addr, versioned_account_address); - continue; - }; + address: versioned_addr, + substate, + created_by_tx, + }) = scan_result + else { + warn!(target: LOG_TARGET, "Vault {} for account {} does not exist according to validator node", vault_addr, versioned_account_address); + continue; + }; if let Some(vault_version) = maybe_vault_version { // The first time a vault is found, know about the vault substate from the tx result but never added diff --git a/dan_layer/consensus/src/hotstuff/on_propose.rs b/dan_layer/consensus/src/hotstuff/on_propose.rs index 61c3927c7..121538669 100644 --- a/dan_layer/consensus/src/hotstuff/on_propose.rs +++ b/dan_layer/consensus/src/hotstuff/on_propose.rs @@ -234,10 +234,7 @@ where TConsensusSpec: ConsensusSpec // TODO: update the evidence after execution so all transactions are treated equally here let db_transaction = t.get_transaction(tx)?; let involved = if db_transaction.transaction().has_inputs_without_version() { - db_transaction - .transaction() - .all_inputs_iter() - .count() + db_transaction.transaction().all_inputs_iter().count() } else { local_committee_shard.count_distinct_shards(t.transaction().evidence.shards_iter()) }; diff --git a/dan_layer/storage/src/consensus_models/executed_transaction.rs b/dan_layer/storage/src/consensus_models/executed_transaction.rs index 28320f1a7..86e27a98a 100644 --- a/dan_layer/storage/src/consensus_models/executed_transaction.rs +++ b/dan_layer/storage/src/consensus_models/executed_transaction.rs @@ -148,37 +148,46 @@ impl ExecutedTransaction { // Note that we only add evidence for inputs that have specific version numbers let mut deduped_evidence = HashMap::new(); - deduped_evidence.extend(self.transaction - .inputs() - .iter() - .filter(|i| i.version().is_some()) - .map(|input| { - (input.to_substate_address(), ShardEvidence { - qc_ids: IndexSet::new(), - lock: LockFlag::Write, - }) - })); - - deduped_evidence.extend(self.transaction.input_refs().iter() - .filter(|i| i.version().is_some()) - .map(|input_ref| { - (input_ref.to_substate_address(), ShardEvidence { - qc_ids: IndexSet::new(), - lock: LockFlag::Read, - }) - })); - - deduped_evidence.extend(self.transaction.filled_inputs().iter() - .filter(|i| i.version().is_some()) - .map(|input_ref| { - (input_ref.to_substate_address(), ShardEvidence { - qc_ids: IndexSet::new(), - lock: LockFlag::Write, - }) - })); - - deduped_evidence.extend(self.resulting_outputs.iter() - .map(|output| { + deduped_evidence.extend( + self.transaction + .inputs() + .iter() + .filter(|i| i.version().is_some()) + .map(|input| { + (input.to_substate_address(), ShardEvidence { + qc_ids: IndexSet::new(), + lock: LockFlag::Write, + }) + }), + ); + + deduped_evidence.extend( + self.transaction + .input_refs() + .iter() + .filter(|i| i.version().is_some()) + .map(|input_ref| { + (input_ref.to_substate_address(), ShardEvidence { + qc_ids: IndexSet::new(), + lock: LockFlag::Read, + }) + }), + ); + + deduped_evidence.extend( + self.transaction + .filled_inputs() + .iter() + .filter(|i| i.version().is_some()) + .map(|input_ref| { + (input_ref.to_substate_address(), ShardEvidence { + qc_ids: IndexSet::new(), + lock: LockFlag::Write, + }) + }), + ); + + deduped_evidence.extend(self.resulting_outputs.iter().map(|output| { (*output, ShardEvidence { qc_ids: IndexSet::new(), lock: LockFlag::Write, diff --git a/integration_tests/src/validator_node.rs b/integration_tests/src/validator_node.rs index b83eb4afd..672457bba 100644 --- a/integration_tests/src/validator_node.rs +++ b/integration_tests/src/validator_node.rs @@ -24,6 +24,7 @@ use std::{ fs, path::{Path, PathBuf}, }; + use reqwest::Url; use tari_common::{ configuration::{CommonConfig, StringList}, diff --git a/integration_tests/tests/steps/validator_node.rs b/integration_tests/tests/steps/validator_node.rs index e0a671cff..da2c04e59 100644 --- a/integration_tests/tests/steps/validator_node.rs +++ b/integration_tests/tests/steps/validator_node.rs @@ -5,6 +5,7 @@ use std::{ str::FromStr, time::{Duration, Instant}, }; + use cucumber::{given, then, when}; use integration_tests::{ base_node::get_base_node_client,