Skip to content

Commit

Permalink
fix: potential overflow of coinbase calc (#6306)
Browse files Browse the repository at this point in the history
Description
---
fix potential overflow of multiple coinbase grpc call

Motivation and Context
---

How Has This Been Tested?
---
tests
  • Loading branch information
SWvheerden authored Apr 26, 2024
1 parent 4cc082d commit 030d389
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 31 deletions.
48 changes: 31 additions & 17 deletions applications/minotari_node/src/grpc/base_node_grpc_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -788,27 +788,41 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer {
let mut coinbases: Vec<tari_rpc::NewBlockCoinbase> = request.coinbases;

// let validate the coinbase amounts;
let reward = self
.consensus_rules
.calculate_coinbase_and_fees(new_template.header.height, new_template.body.kernels())
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 += u128::from(coinbase.value);
}
let mut cur_share_sum = 0u128;
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.saturating_mul(reward))
.checked_div(total_shares)
.ok_or(obscure_error_if_true(
report_error_flag,
Status::internal("total shares are zero".to_string()),
))? -
prev_coinbase_value,
)
.map_err(|_| {
obscure_error_if_true(
report_error_flag,
Status::internal("Could not calculate the amount of fees in the block".to_string()),
Status::internal("Single coinbase fees exceeded u64".to_string()),
)
})?
.as_u64();
let mut total_shares = 0u64;
for coinbase in &coinbases {
total_shares += coinbase.value;
}
let mut remainder = reward - ((reward / total_shares) * total_shares);
for coinbase in &mut coinbases {
coinbase.value *= reward / total_shares;
if remainder > 0 {
coinbase.value += 1;
remainder -= 1;
}
})?;
prev_coinbase_value = u128::from(coinbase.value);
}

let key_manager = create_memory_db_key_manager();
Expand Down
11 changes: 6 additions & 5 deletions base_layer/common_types/src/emoji.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ impl TransactionOutput {
}
}
} else {
"None".to_string()
format!("None({})", self.minimum_value_promise)
}
}

Expand Down
17 changes: 9 additions & 8 deletions integration_tests/tests/steps/node_steps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(_) => (),
Expand Down

0 comments on commit 030d389

Please sign in to comment.