From 201e0357e3dad65954c48cc384e45b579cf5a165 Mon Sep 17 00:00:00 2001 From: SW van Heerden Date: Wed, 24 Apr 2024 15:48:31 +0200 Subject: [PATCH 1/3] fix potential overflow of coinbase calc --- .../src/grpc/base_node_grpc_server.rs | 23 +++++++++++-------- base_layer/common_types/src/emoji.rs | 11 +++++---- .../transaction_output.rs | 2 +- integration_tests/tests/steps/node_steps.rs | 17 +++++++------- 4 files changed, 30 insertions(+), 23 deletions(-) diff --git a/applications/minotari_node/src/grpc/base_node_grpc_server.rs b/applications/minotari_node/src/grpc/base_node_grpc_server.rs index 4b2e044644..ed8c187aa9 100644 --- a/applications/minotari_node/src/grpc/base_node_grpc_server.rs +++ b/applications/minotari_node/src/grpc/base_node_grpc_server.rs @@ -797,18 +797,23 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer { Status::internal("Could not calculate the amount of fees in the block".to_string()), ) })? - .as_u64(); - let mut total_shares = 0u64; + .as_u64() as u128; + let mut total_shares = 0u128; for coinbase in &coinbases { - total_shares += coinbase.value; + total_shares += coinbase.value as u128; } - let mut remainder = reward - ((reward / total_shares) * total_shares); + let mut cur_share_sum = 0u128; + let mut prev_coinbase_value = 0u128; for coinbase in &mut coinbases { - coinbase.value *= reward / total_shares; - if remainder > 0 { - coinbase.value += 1; - remainder -= 1; - } + cur_share_sum += coinbase.value as u128; + coinbase.value = + u64::try_from((cur_share_sum * reward) / total_shares - prev_coinbase_value).map_err(|_| { + obscure_error_if_true( + report_error_flag, + Status::internal("Single coinbase fees exceeded u64".to_string()), + ) + })?; + prev_coinbase_value = coinbase.value as u128; } let key_manager = create_memory_db_key_manager(); diff --git a/base_layer/common_types/src/emoji.rs b/base_layer/common_types/src/emoji.rs index 8c84882ea7..32b9e3ff5a 100644 --- a/base_layer/common_types/src/emoji.rs +++ b/base_layer/common_types/src/emoji.rs @@ -50,25 +50,26 @@ use crate::{ /// # Example /// /// ``` +/// use std::str::FromStr; /// use tari_common_types::emoji::EmojiId; /// /// // Construct an emoji ID from an emoji string (this can fail) /// let emoji_string = "πŸŒ΄πŸ¦€πŸ”ŒπŸ“ŒπŸš‘πŸŒ°πŸŽ“πŸŒ΄πŸŠπŸŒπŸ”’πŸ’‘πŸœπŸ“œπŸ‘›πŸ΅πŸ‘›πŸ½πŸŽ‚πŸ»πŸ¦‹πŸ“πŸ‘ΆπŸ­πŸΌπŸ€πŸŽͺπŸ’”πŸ’΅πŸ₯‘πŸ”‹πŸŽ’πŸ₯Š"; -/// let emoji_id_from_emoji_string = EmojiId::from_emoji_string(emoji_string); +/// let emoji_id_from_emoji_string = EmojiId::from_str(emoji_string); /// assert!(emoji_id_from_emoji_string.is_ok()); /// /// // Get the public key -/// let public_key = emoji_id_from_emoji_string.unwrap().to_public_key(); +/// let public_key = emoji_id_from_emoji_string.unwrap().as_public_key().clone(); /// /// // Reconstruct the emoji ID from the public key (this cannot fail) -/// let emoji_id_from_public_key = EmojiId::from_public_key(&public_key); +/// let emoji_id_from_public_key = EmojiId::from(&public_key); /// /// // An emoji ID is deterministic -/// assert_eq!(emoji_id_from_public_key.to_emoji_string(), emoji_string); +/// assert_eq!(emoji_id_from_public_key.to_string(), emoji_string); /// /// // Oh no! We swapped the first two emoji characters by mistake, so this should fail /// let invalid_emoji_string = "πŸ¦€πŸŒ΄πŸ”ŒπŸ“ŒπŸš‘πŸŒ°πŸŽ“πŸŒ΄πŸŠπŸŒπŸ”’πŸ’‘πŸœπŸ“œπŸ‘›πŸ΅πŸ‘›πŸ½πŸŽ‚πŸ»πŸ¦‹πŸ“πŸ‘ΆπŸ­πŸΌπŸ€πŸŽͺπŸ’”πŸ’΅πŸ₯‘πŸ”‹πŸŽ’πŸ₯Š"; -/// assert!(EmojiId::from_emoji_string(invalid_emoji_string).is_err()); +/// assert!(EmojiId::from_str(invalid_emoji_string).is_err()); /// ``` #[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd)] pub struct EmojiId(PublicKey); diff --git a/base_layer/core/src/transactions/transaction_components/transaction_output.rs b/base_layer/core/src/transactions/transaction_components/transaction_output.rs index 49fd8fa53d..51d856cd1e 100644 --- a/base_layer/core/src/transactions/transaction_components/transaction_output.rs +++ b/base_layer/core/src/transactions/transaction_components/transaction_output.rs @@ -196,7 +196,7 @@ impl TransactionOutput { } } } else { - "None".to_string() + format!("None({})", self.minimum_value_promise) } } diff --git a/integration_tests/tests/steps/node_steps.rs b/integration_tests/tests/steps/node_steps.rs index f6c1049ab6..fe9c5f62fc 100644 --- a/integration_tests/tests/steps/node_steps.rs +++ b/integration_tests/tests/steps/node_steps.rs @@ -851,18 +851,19 @@ async fn generate_block_with_2_as_single_request_coinbases(world: &mut TariWorld } assert_eq!(coinbase_kernel_count, 1); assert_eq!(coinbase_utxo_count, 2); - let mut num_6154266700 = 0; - let mut num_12308533398 = 0; + let mut num_6154272109 = 0; + let mut num_12308544218 = 0; for output in body.outputs() { - if output.minimum_value_promise.as_u64() == 6154266700 { - num_6154266700 += 1; + if output.minimum_value_promise.as_u64() == 6154272109 { + num_6154272109 += 1; } - if output.minimum_value_promise.as_u64() == 12308533398 { - num_12308533398 += 1; + if output.minimum_value_promise.as_u64() == 12308544218 { + num_12308544218 += 1; } } - assert_eq!(num_6154266700, 1); - assert_eq!(num_12308533398, 1); + + assert_eq!(num_6154272109, 1); + assert_eq!(num_12308544218, 1); match client.submit_block(new_block).await { Ok(_) => (), From 4aaeafb0d3528630bc80361a3e39af9be1ba07a9 Mon Sep 17 00:00:00 2001 From: SW van Heerden Date: Wed, 24 Apr 2024 16:14:53 +0200 Subject: [PATCH 2/3] clippy --- .../src/grpc/base_node_grpc_server.rs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/applications/minotari_node/src/grpc/base_node_grpc_server.rs b/applications/minotari_node/src/grpc/base_node_grpc_server.rs index ed8c187aa9..dc5dbcb829 100644 --- a/applications/minotari_node/src/grpc/base_node_grpc_server.rs +++ b/applications/minotari_node/src/grpc/base_node_grpc_server.rs @@ -788,24 +788,25 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer { let mut coinbases: Vec = request.coinbases; // let validate the coinbase amounts; - let reward = self - .consensus_rules - .calculate_coinbase_and_fees(new_template.header.height, new_template.body.kernels()) - .map_err(|_| { - obscure_error_if_true( - report_error_flag, - Status::internal("Could not calculate the amount of fees in the block".to_string()), - ) - })? - .as_u64() as u128; + let reward = u128::from( + self.consensus_rules + .calculate_coinbase_and_fees(new_template.header.height, new_template.body.kernels()) + .map_err(|_| { + obscure_error_if_true( + report_error_flag, + Status::internal("Could not calculate the amount of fees in the block".to_string()), + ) + })? + .as_u64(), + ); let mut total_shares = 0u128; for coinbase in &coinbases { - total_shares += coinbase.value as u128; + total_shares += u128::from(coinbase.value); } let mut cur_share_sum = 0u128; let mut prev_coinbase_value = 0u128; for coinbase in &mut coinbases { - cur_share_sum += coinbase.value as u128; + cur_share_sum += u128::from(coinbase.value); coinbase.value = u64::try_from((cur_share_sum * reward) / total_shares - prev_coinbase_value).map_err(|_| { obscure_error_if_true( @@ -813,7 +814,7 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer { Status::internal("Single coinbase fees exceeded u64".to_string()), ) })?; - prev_coinbase_value = coinbase.value as u128; + prev_coinbase_value = u128::from(coinbase.value); } let key_manager = create_memory_db_key_manager(); From 05dc068ca081fa5ccea041fc95626884cfd649d8 Mon Sep 17 00:00:00 2001 From: SW van Heerden Date: Fri, 26 Apr 2024 12:21:36 +0200 Subject: [PATCH 3/3] fixed overflows --- .../src/grpc/base_node_grpc_server.rs | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/applications/minotari_node/src/grpc/base_node_grpc_server.rs b/applications/minotari_node/src/grpc/base_node_grpc_server.rs index dc5dbcb829..4ad1e1a1b3 100644 --- a/applications/minotari_node/src/grpc/base_node_grpc_server.rs +++ b/applications/minotari_node/src/grpc/base_node_grpc_server.rs @@ -807,13 +807,21 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer { let mut prev_coinbase_value = 0u128; for coinbase in &mut coinbases { cur_share_sum += u128::from(coinbase.value); - coinbase.value = - u64::try_from((cur_share_sum * reward) / total_shares - prev_coinbase_value).map_err(|_| { - obscure_error_if_true( + coinbase.value = u64::try_from( + (cur_share_sum.saturating_mul(reward)) + .checked_div(total_shares) + .ok_or(obscure_error_if_true( report_error_flag, - Status::internal("Single coinbase fees exceeded u64".to_string()), - ) - })?; + Status::internal("total shares are zero".to_string()), + ))? - + prev_coinbase_value, + ) + .map_err(|_| { + obscure_error_if_true( + report_error_flag, + Status::internal("Single coinbase fees exceeded u64".to_string()), + ) + })?; prev_coinbase_value = u128::from(coinbase.value); }