Skip to content

Commit

Permalink
fix: addition overflow when coinbase + fees is too high (tari-project…
Browse files Browse the repository at this point in the history
…#5706)

Description
---
Use a `checked_addition` and catch errors when overflow occurs.

Motivation and Context
---
Block could be produced and shared that have coinbases and fees that are
too high resulting in an overflow and node crash.

How Has This Been Tested?
---
CI

What process can a PR reviewer use to test or verify this change?
---
Mostly see if you like what's happening. In `base_node_grpc_server.rs` a
fair bit of the new wrapping results in either getting the expected
result or returning this specific problem and I'm not so sure it's the
most generic way to recover.

Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify
  • Loading branch information
brianp committed Aug 31, 2023
1 parent a58ec1f commit 13993f1
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 54 deletions.
89 changes: 51 additions & 38 deletions applications/minotari_node/src/grpc/base_node_grpc_server.rs
Expand Up @@ -401,56 +401,68 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer {
data.into_iter()
.map(|chain_block| {
let (block, acc_data, confirmations, _) = chain_block.dissolve();
let total_block_reward = consensus_rules
.calculate_coinbase_and_fees(block.header.height, block.body.kernels());

tari_rpc::BlockHeaderResponse {
difficulty: acc_data.achieved_difficulty.into(),
num_transactions: block.body.kernels().len() as u32,
confirmations,
header: Some(block.header.into()),
reward: total_block_reward.into(),
match consensus_rules
.calculate_coinbase_and_fees(block.header.height, block.body.kernels())
{
Ok(total_block_reward) => Ok(tari_rpc::BlockHeaderResponse {
difficulty: acc_data.achieved_difficulty.into(),
num_transactions: block.body.kernels().len() as u32,
confirmations,
header: Some(block.header.into()),
reward: total_block_reward.into(),
}),
Err(e) => Err(e),
}
})
.rev()
.collect::<Vec<_>>()
.collect::<Result<Vec<_>, String>>()
} else {
data.into_iter()
.map(|chain_block| {
let (block, acc_data, confirmations, _) = chain_block.dissolve();
let total_block_reward = consensus_rules
.calculate_coinbase_and_fees(block.header.height, block.body.kernels());

tari_rpc::BlockHeaderResponse {
difficulty: acc_data.achieved_difficulty.into(),
num_transactions: block.body.kernels().len() as u32,
confirmations,
header: Some(block.header.into()),
reward: total_block_reward.into(),
match consensus_rules
.calculate_coinbase_and_fees(block.header.height, block.body.kernels())
{
Ok(total_block_reward) => Ok(tari_rpc::BlockHeaderResponse {
difficulty: acc_data.achieved_difficulty.into(),
num_transactions: block.body.kernels().len() as u32,
confirmations,
header: Some(block.header.into()),
reward: total_block_reward.into(),
}),
Err(e) => Err(e),
}
})
.collect()
.collect::<Result<Vec<_>, String>>()
}
},
};
let result_size = result_data.len();
debug!(target: LOG_TARGET, "Result headers: {}", result_size);

for response in result_data {
// header wont be none here as we just filled it in above
debug!(
target: LOG_TARGET,
"Sending block header: {}",
response.header.as_ref().map(|h| h.height).unwrap_or(0)
);
if tx.send(Ok(response)).await.is_err() {
// Sender has closed i.e the connection has dropped/request was abandoned
warn!(
target: LOG_TARGET,
"[list_headers] GRPC request cancelled while sending response"
);
return;
}
match result_data {
Err(e) => {
error!(target: LOG_TARGET, "No result headers transmitted due to error: {}", e)
},
Ok(result_data) => {
let result_size = result_data.len();
debug!(target: LOG_TARGET, "Result headers: {}", result_size);

for response in result_data {
// header wont be none here as we just filled it in above
debug!(
target: LOG_TARGET,
"Sending block header: {}",
response.header.as_ref().map( | h| h.height).unwrap_or(0)
);
if tx.send(Ok(response)).await.is_err() {
// Sender has closed i.e the connection has dropped/request was abandoned
warn!(
target: LOG_TARGET,
"[list_headers] GRPC request cancelled while sending response"
);
return;
}
}
},
}
}
});
Expand Down Expand Up @@ -1349,7 +1361,8 @@ impl tari_rpc::base_node_server::BaseNode for BaseNodeGrpcServer {
let (block, acc_data, confirmations, _) = block.dissolve();
let total_block_reward = self
.consensus_rules
.calculate_coinbase_and_fees(block.header.height, block.body.kernels());
.calculate_coinbase_and_fees(block.header.height, block.body.kernels())
.map_err(Status::out_of_range)?;

let resp = tari_rpc::BlockHeaderResponse {
difficulty: acc_data.achieved_difficulty.into(),
Expand Down
23 changes: 20 additions & 3 deletions base_layer/core/src/consensus/consensus_manager.rs
Expand Up @@ -131,9 +131,26 @@ impl ConsensusManager {
}

/// Creates a total_coinbase offset containing all fees for the validation from the height and kernel set
pub fn calculate_coinbase_and_fees(&self, height: u64, kernels: &[TransactionKernel]) -> MicroMinotari {
let coinbase = self.emission_schedule().block_reward(height);
kernels.iter().fold(coinbase, |total, k| total + k.fee)
pub fn calculate_coinbase_and_fees(
&self,
height: u64,
kernels: &[TransactionKernel],
) -> Result<MicroMinotari, String> {
let mut total = self.emission_schedule().block_reward(height);

for kernel in kernels {
match total.checked_add(kernel.fee) {
Some(t) => total = t,
None => {
return Err(format!(
"Coinbase total ({}) + fee ({}) exceeds max transactions allowance",
total, kernel.fee
))
},
}
}

Ok(total)
}

/// Returns a ref to the chain strength comparer
Expand Down
18 changes: 10 additions & 8 deletions base_layer/core/src/test_helpers/mod.rs
Expand Up @@ -79,14 +79,16 @@ pub async fn create_block(
header.height = block_height;
// header.prev_hash = prev_block.hash();
let reward = spec.reward_override.unwrap_or_else(|| {
rules.calculate_coinbase_and_fees(
header.height,
&spec
.transactions
.iter()
.flat_map(|tx| tx.body.kernels().clone())
.collect::<Vec<_>>(),
)
rules
.calculate_coinbase_and_fees(
header.height,
&spec
.transactions
.iter()
.flat_map(|tx| tx.body.kernels().clone())
.collect::<Vec<_>>(),
)
.unwrap()
});

let spend_key_id = KeyId::Managed {
Expand Down
Expand Up @@ -108,7 +108,17 @@ fn validate_block_aggregate_body(
) -> Result<(), ValidationError> {
let offset = &block.header.total_kernel_offset;
let script_offset = &block.header.total_script_offset;
let total_coinbase = consensus_manager.calculate_coinbase_and_fees(block.header.height, block.body.kernels());
let total_coinbase = consensus_manager
.calculate_coinbase_and_fees(block.header.height, block.body.kernels())
.map_err(|err| {
warn!(
target: LOG_TARGET,
"Validation failed on block:{}:{:?}",
block.hash().to_hex(),
err
);
ValidationError::CoinbaseExceedsMaxLimit
})?;
validator
.validate(
&block.body,
Expand Down Expand Up @@ -136,7 +146,18 @@ fn check_coinbase_output(
rules: &ConsensusManager,
factories: &CryptoFactories,
) -> Result<(), ValidationError> {
let total_coinbase = rules.calculate_coinbase_and_fees(block.header.height, block.body.kernels());
let total_coinbase = rules
.calculate_coinbase_and_fees(block.header.height, block.body.kernels())
.map_err(|err| {
warn!(
target: LOG_TARGET,
"Validation failed on block:{}:{:?}",
block.hash().to_hex(),
err
);
ValidationError::CoinbaseExceedsMaxLimit
})?;

block
.check_coinbase_output(
total_coinbase,
Expand Down
2 changes: 2 additions & 0 deletions base_layer/core/src/validation/error.rs
Expand Up @@ -67,6 +67,8 @@ pub enum ValidationError {
ContainsDuplicateUtxoCommitment,
#[error("Final state validation failed: The UTXO set did not balance with the expected emission at height {0}")]
ChainBalanceValidationFailed(u64),
#[error("The total value + fees of the block exceeds the maximum allowance on chain")]
CoinbaseExceedsMaxLimit,
#[error("Proof of work error: {0}")]
ProofOfWorkError(#[from] PowError),
#[error("Attempted to validate genesis block")]
Expand Down
6 changes: 3 additions & 3 deletions base_layer/core/src/validation/helpers.rs
Expand Up @@ -534,7 +534,7 @@ mod test {

let body = AggregateBody::new(vec![], vec![coinbase_output], vec![coinbase_kernel]);

let reward = rules.calculate_coinbase_and_fees(height, body.kernels());
let reward = rules.calculate_coinbase_and_fees(height, body.kernels()).unwrap();
let coinbase_lock_height = rules.consensus_constants(height).coinbase_min_maturity();
body.check_coinbase_output(reward, coinbase_lock_height, &CryptoFactories::default(), height)
.unwrap();
Expand All @@ -553,7 +553,7 @@ mod test {

let body = AggregateBody::new(vec![], vec![coinbase_output], vec![coinbase_kernel]);

let reward = rules.calculate_coinbase_and_fees(height, body.kernels());
let reward = rules.calculate_coinbase_and_fees(height, body.kernels()).unwrap();
let coinbase_lock_height = rules.consensus_constants(height).coinbase_min_maturity();

let err = body
Expand All @@ -574,7 +574,7 @@ mod test {
let coinbase_kernel = test_helpers::create_coinbase_kernel(&coinbase.spending_key_id, &key_manager).await;

let body = AggregateBody::new(vec![], vec![coinbase_output], vec![coinbase_kernel]);
let reward = rules.calculate_coinbase_and_fees(height, body.kernels());
let reward = rules.calculate_coinbase_and_fees(height, body.kernels()).unwrap();
let coinbase_lock_height = rules.consensus_constants(height).coinbase_min_maturity();

let err = body
Expand Down

0 comments on commit 13993f1

Please sign in to comment.