From 4ea0d2eec90c803e9a9c1c9bd8169ca5ac259ed5 Mon Sep 17 00:00:00 2001 From: Samer Afach Date: Fri, 29 Sep 2023 02:52:19 +0400 Subject: [PATCH 1/6] Only checked add/sub for Uint* types --- .../src/detail/block_invalidation/mod.rs | 7 +++- chainstate/src/detail/chainstateref/mod.rs | 3 +- common/src/chain/chaintrust/asymptote.rs | 4 +- common/src/uint/impls.rs | 42 +++++++++++-------- consensus/src/pos/target.rs | 6 ++- 5 files changed, 38 insertions(+), 24 deletions(-) diff --git a/chainstate/src/detail/block_invalidation/mod.rs b/chainstate/src/detail/block_invalidation/mod.rs index ddfc97fdd0..c1087a42ce 100644 --- a/chainstate/src/detail/block_invalidation/mod.rs +++ b/chainstate/src/detail/block_invalidation/mod.rs @@ -175,8 +175,11 @@ impl<'a, S: BlockchainStorage, V: TransactionVerificationStrategy> BlockInvalida let cur_best_block_index = get_best_block_index(&chainstate_ref)?; let cur_best_chain_trust = cur_best_block_index.chain_trust(); - let best_chain_candidates = - BestChainCandidates::new(&chainstate_ref, cur_best_chain_trust + Uint256::ONE)?; + let best_chain_candidates = BestChainCandidates::new( + &chainstate_ref, + (cur_best_chain_trust + Uint256::ONE) + .expect("Chain trust won't be saturated in a very long time"), + )?; (cur_best_chain_trust, best_chain_candidates) }; diff --git a/chainstate/src/detail/chainstateref/mod.rs b/chainstate/src/detail/chainstateref/mod.rs index 9f77737fc3..6df9d1f30e 100644 --- a/chainstate/src/detail/chainstateref/mod.rs +++ b/chainstate/src/detail/chainstateref/mod.rs @@ -974,7 +974,8 @@ impl<'a, S: BlockchainStorageRead, V: TransactionVerificationStrategy> Chainstat // Set Chain Trust let prev_block_chaintrust: Uint256 = prev_block_index.chain_trust(); - let chain_trust = prev_block_chaintrust + current_block_proof; + let chain_trust = (prev_block_chaintrust + current_block_proof) + .expect("Chain trust growth is locally controlled. This can't happen."); let block_index = BlockIndex::new( block, chain_trust, diff --git a/common/src/chain/chaintrust/asymptote.rs b/common/src/chain/chaintrust/asymptote.rs index 9db281b2b0..572c6aee26 100644 --- a/common/src/chain/chaintrust/asymptote.rs +++ b/common/src/chain/chaintrust/asymptote.rs @@ -96,7 +96,9 @@ pub fn calculate_block_proof(timestamp_diff: u64) -> Uint256 { let block_weight = Uint256::from(get_weight_for_block()); let empty_time_slots_weight = Uint256::from(empty_time_slots_weight); - block_weight - empty_time_slots_weight + block_weight + .checked_sub(&empty_time_slots_weight) + .expect("Checked above; cannot fail") } #[cfg(test)] diff --git a/common/src/uint/impls.rs b/common/src/uint/impls.rs index 1824a9d0d3..9c0afc4eac 100644 --- a/common/src/uint/impls.rs +++ b/common/src/uint/impls.rs @@ -211,7 +211,7 @@ macro_rules! construct_uint { loop { if sub_copy >= shift_copy { ret[shift / 64] |= 1 << (shift % 64); - sub_copy = sub_copy - shift_copy; + sub_copy = sub_copy.unchecked_sub(&shift_copy); } shift_copy = shift_copy >> 1; if shift == 0 { @@ -256,6 +256,12 @@ macro_rules! construct_uint { self.overflowing_add_with_carry(other, false) } + fn unchecked_sub(&self, other: &Self) -> Self { + let a = self.overflowing_add(&!*other).0; + let one: $name = $crate::uint::BitArray::one(); + a.overflowing_add(&one).0 + } + // TODO: use checked functions for all operations // mintlayer/mintlayer-core/-/issues/1132 pub fn checked_add(&self, other: &Self) -> Option { @@ -264,7 +270,7 @@ macro_rules! construct_uint { } pub fn checked_sub(&self, other: &Self) -> Option { - (*self >= *other).then(|| *self - *other) + (*self >= *other).then(|| self.unchecked_sub(other)) } pub fn widening_mul(&self, other: &Self) -> (Self, Self) { @@ -381,19 +387,19 @@ macro_rules! construct_uint { } impl ::core::ops::Add<$name> for $name { - type Output = $name; + type Output = Option<$name>; - fn add(self, other: $name) -> $name { - self.overflowing_add(&other).0 + fn add(self, other: $name) -> Option<$name> { + self.checked_add(&other) } } impl ::core::ops::Sub<$name> for $name { - type Output = $name; + type Output = Option<$name>; #[inline] - fn sub(self, other: $name) -> $name { - self + !other + $crate::uint::BitArray::one() + fn sub(self, other: $name) -> Option<$name> { + self.checked_sub(&other) } } @@ -852,7 +858,7 @@ mod tests { let init = Uint256::from_u64(0xDEADBEEFDEADBEEF); let copy = init; - let add = init + copy; + let add = (init + copy).unwrap(); assert_eq!(add, Uint256([0xBD5B7DDFBD5B7DDEu64, 1, 0, 0])); // Bitshifts let shl = add << 88; @@ -870,7 +876,7 @@ mod tests { Uint256([0x7DDE000000000001u64, 0x0001BD5B7DDFBD5B, 0, 0]) ); // Subtraction - let sub = incr - init; + let sub = (incr - init).unwrap(); assert_eq!( sub, Uint256([0x9F30411021524112u64, 0x0001BD5B7DDFBD5A, 0, 0]) @@ -900,7 +906,7 @@ mod tests { Uint256::from_u64(35498456) % Uint256::from_u64(3435), Uint256::from_u64(1166) ); - let rem_src = mult * Uint256::from_u64(39842) + Uint256::from_u64(9054); + let rem_src = (mult * Uint256::from_u64(39842) + Uint256::from_u64(9054)).unwrap(); assert_eq!(rem_src % Uint256::from_u64(39842), Uint256::from_u64(9054)); // TODO: bit inversion } @@ -1049,7 +1055,7 @@ mod tests { #[test] pub fn uint256_bitslice_test() { let init = Uint256::from_u64(0xDEADBEEFDEADBEEF); - let add = init + (init << 64); + let add = (init + (init << 64)).unwrap(); assert_eq!(add.bit_slice(64, 128), init); assert_eq!(add.mask(64), init); } @@ -1061,7 +1067,7 @@ mod tests { let init = Uint256::from_u64(0xDEADBEEFDEADBEEF); assert_eq!(init << 64, Uint256([0, 0xDEADBEEFDEADBEEF, 0, 0])); - let add = (init << 64) + init; + let add = ((init << 64) + init).unwrap(); assert_eq!(add, Uint256([0xDEADBEEFDEADBEEF, 0xDEADBEEFDEADBEEF, 0, 0])); assert_eq!(add >> 64, Uint256([0xDEADBEEFDEADBEEF, 0, 0, 0])); assert_eq!( @@ -1126,7 +1132,7 @@ mod tests { let b = Uint512([u64::MAX, u64::MAX, u64::MAX, u64::MAX, 0x00, 0x00, 0x00, 0x00]); assert_eq!(Uint256::try_from(b).unwrap(), a); - let a = Uint512::from(Uint256::MAX) + Uint512::ONE; + let a = (Uint512::from(Uint256::MAX) + Uint512::ONE).unwrap(); assert_eq!( Uint256::try_from(a).unwrap_err(), UintConversionError::ConversionOverflow @@ -1166,7 +1172,7 @@ mod tests { let b = Uint256([u64::MAX, u64::MAX, 0x00, 0x00]); assert_eq!(u128::try_from(b).unwrap(), a); - let a = Uint256::from(u128::MAX) + Uint256::ONE; + let a = (Uint256::from(u128::MAX) + Uint256::ONE).unwrap(); assert_eq!( u128::try_from(a).unwrap_err(), UintConversionError::ConversionOverflow @@ -1192,8 +1198,8 @@ mod tests { } { let a: Uint128 = rng.gen::().into(); - let b = Uint128::MAX - a; - assert_eq!(a.checked_add(&b), Some(a + b)); + let b = (Uint128::MAX - a).unwrap(); + assert_eq!(a.checked_add(&b), Some(a.overflowing_add(&b).0)); } } @@ -1211,7 +1217,7 @@ mod tests { let a = rng.gen::(); let b: Uint128 = rng.gen_range(0..a).into(); let a: Uint128 = a.into(); - assert_eq!(a.checked_sub(&b), Some(a - b)); + assert_eq!(a.checked_sub(&b), Some(a.unchecked_sub(&b))); assert_eq!(b.checked_sub(&a), None); } } diff --git a/consensus/src/pos/target.rs b/consensus/src/pos/target.rs index 6da5dfdef6..c55f0e90cb 100644 --- a/consensus/src/pos/target.rs +++ b/consensus/src/pos/target.rs @@ -87,8 +87,10 @@ fn calculate_new_target( let difficulty_change_limit = prev_target * Uint512::from_u64(pos_config.difficulty_change_limit().value().into()) / Uint512::from_u64(1000); - let lower_limit = prev_target - difficulty_change_limit; - let upper_limit = prev_target + difficulty_change_limit; + let lower_limit = (prev_target - difficulty_change_limit) + .expect("Cannot fail because difficulty_change_limit is in [0, 1]"); + let upper_limit = (prev_target + difficulty_change_limit) + .expect("Cannot fail because it was converted from Uint256 to Uint512"); let new_target = num::clamp(new_target, lower_limit, upper_limit); let new_target = Uint256::try_from(new_target).unwrap_or(pos_config.target_limit()); From 8b1b5b5d5c6f30210d8285c1b340c53d648f9961 Mon Sep 17 00:00:00 2001 From: Samer Afach Date: Fri, 29 Sep 2023 03:19:02 +0400 Subject: [PATCH 2/6] Enforce checked_mul by default for Uint* types --- .../reward_distribution.rs | 3 ++- common/src/chain/chaintrust/asymptote.rs | 2 +- common/src/uint/impls.rs | 21 ++++++++++++------- consensus/src/pos/hash_check.rs | 6 ++++-- consensus/src/pos/target.rs | 11 ++++++---- consensus/src/pow/helpers.rs | 2 +- 6 files changed, 28 insertions(+), 17 deletions(-) diff --git a/chainstate/tx-verifier/src/transaction_verifier/reward_distribution.rs b/chainstate/tx-verifier/src/transaction_verifier/reward_distribution.rs index f91d769da1..635c210e95 100644 --- a/chainstate/tx-verifier/src/transaction_verifier/reward_distribution.rs +++ b/chainstate/tx-verifier/src/transaction_verifier/reward_distribution.rs @@ -191,7 +191,8 @@ fn calculate_rewards_per_delegation( .map( |(delegation_id, balance_amount)| -> Result<_, ConnectTransactionError> { let balance = Uint256::from_amount(*balance_amount); - let reward = (total_delegations_reward * balance) / total_delegations_balance; + let numer = (total_delegations_reward * balance).expect("Source types are smaller"); + let reward = numer / total_delegations_balance; let reward: common::primitives::amount::UnsignedIntType = reward.try_into().map_err(|_| { ConnectTransactionError::DelegationRewardOverflow( diff --git a/common/src/chain/chaintrust/asymptote.rs b/common/src/chain/chaintrust/asymptote.rs index 572c6aee26..7ab39bcc74 100644 --- a/common/src/chain/chaintrust/asymptote.rs +++ b/common/src/chain/chaintrust/asymptote.rs @@ -173,7 +173,7 @@ mod tests { // Given that the maximum block weight is 1*SCALING_FACTOR, // and it only goes down when there are empty time-slots in between, // the maximum chain trust is the following: - let max_chain_trust = max_block_height * single_block_weight; + let max_chain_trust = (max_block_height * single_block_weight).unwrap(); // There should not be any overflow to ensure that the chain trust is always less than the maximum possible value. assert!(max_block_height < max_chain_trust); diff --git a/common/src/uint/impls.rs b/common/src/uint/impls.rs index 9c0afc4eac..94470f1b6f 100644 --- a/common/src/uint/impls.rs +++ b/common/src/uint/impls.rs @@ -313,6 +313,10 @@ macro_rules! construct_uint { pub fn checked_div(&self, other: &Self) -> Option { (*other != Self::ZERO).then(|| *self / *other) } + + pub fn overflowing_mul(&self, other: &Self) -> Self { + self.widening_mul(other).0 + } } impl From<[u8; $n_words * 8]> for $name { @@ -404,10 +408,10 @@ macro_rules! construct_uint { } impl ::core::ops::Mul<$name> for $name { - type Output = $name; + type Output = Option<$name>; - fn mul(self, other: $name) -> $name { - self.checked_mul(&other).expect("overflow") + fn mul(self, other: $name) -> Option<$name> { + self.checked_mul(&other) } } @@ -906,7 +910,8 @@ mod tests { Uint256::from_u64(35498456) % Uint256::from_u64(3435), Uint256::from_u64(1166) ); - let rem_src = (mult * Uint256::from_u64(39842) + Uint256::from_u64(9054)).unwrap(); + let m = (mult * Uint256::from_u64(39842)).unwrap(); + let rem_src = (m + Uint256::from_u64(9054)).unwrap(); assert_eq!(rem_src % Uint256::from_u64(39842), Uint256::from_u64(9054)); // TODO: bit inversion } @@ -954,14 +959,14 @@ mod tests { pub fn multiplication_test() { let u64_val = Uint256::from_u64(0xDEADBEEFDEADBEEF); - let u128_res = u64_val * u64_val; + let u128_res = (u64_val * u64_val).unwrap(); assert_eq!( u128_res, Uint256([0x048D1354216DA321u64, 0xC1B1CD13A4D13D46, 0, 0]) ); - let u256_res = u128_res * u128_res; + let u256_res = (u128_res * u128_res).unwrap(); assert_eq!( u256_res, @@ -980,7 +985,7 @@ mod tests { 0x7FFFFFFFFFFFFFFF, ]) .into(); - let u512_res = u256_val * u256_val; + let u512_res = (u256_val * u256_val).unwrap(); assert_eq!( u512_res, Uint512([ @@ -1243,7 +1248,7 @@ mod tests { { let a = Uint128::from_u64(rng.gen::()); let b = Uint128::from_u64(rng.gen::()); - assert_eq!(a.checked_mul(&b), Some(a * b)); + assert_eq!(a.checked_mul(&b), Some(a.overflowing_mul(&b))); } } diff --git a/consensus/src/pos/hash_check.rs b/consensus/src/pos/hash_check.rs index 9b425b47fa..0f91e1d0e1 100644 --- a/consensus/src/pos/hash_check.rs +++ b/consensus/src/pos/hash_check.rs @@ -57,7 +57,8 @@ fn check_pos_hash_v0( let pool_balance: Uint512 = pool_balance.into(); ensure!( - hash <= pool_balance * target.into(), + hash <= (pool_balance * target.into()) + .expect("Cannot fail because both were converted from smaller type"), ConsensusPoSError::StakeKernelHashTooHigh ); @@ -95,7 +96,8 @@ fn check_pos_hash_v1( let effective_balance: Uint512 = effective_balance.into(); ensure!( - hash <= effective_balance * target.into(), + hash <= (effective_balance * target.into()) + .expect("Cannot fail because both were converted from smaller type"), ConsensusPoSError::StakeKernelHashTooHigh ); diff --git a/consensus/src/pos/target.rs b/consensus/src/pos/target.rs index c55f0e90cb..d87746adc6 100644 --- a/consensus/src/pos/target.rs +++ b/consensus/src/pos/target.rs @@ -83,9 +83,12 @@ fn calculate_new_target( ); let prev_target: Uint512 = (*prev_target).into(); - let new_target = prev_target * actual_block_time / target_block_time; - let difficulty_change_limit = prev_target - * Uint512::from_u64(pos_config.difficulty_change_limit().value().into()) + let new_target = (prev_target * actual_block_time) + .expect("Source types are smaller than these types") + / target_block_time; + let difficulty_change_limit = (prev_target + * Uint512::from_u64(pos_config.difficulty_change_limit().value().into())) + .expect("Original types are smaller") / Uint512::from_u64(1000); let lower_limit = (prev_target - difficulty_change_limit) .expect("Cannot fail because difficulty_change_limit is in [0, 1]"); @@ -982,7 +985,7 @@ mod tests { let current_hash: Uint512 = current_hash.try_into().unwrap(); // add block if hash satisfies the target - if current_hash <= target_u512 * pool_balance { + if current_hash <= (target_u512 * pool_balance).unwrap() { let block = make_block(&mut rng, tip_id, block_timestamp, target); tip_height = tip_height.next_height(); let new_tip = BlockIndex::new( diff --git a/consensus/src/pow/helpers.rs b/consensus/src/pow/helpers.rs index 56d79964b0..5972efe2ba 100644 --- a/consensus/src/pow/helpers.rs +++ b/consensus/src/pow/helpers.rs @@ -75,7 +75,7 @@ pub fn calculate_new_target( // new target is computed by multiplying the old target by ratio of the actual timespan / target timespan. // see Bitcoin's Protocol rules of Difficulty change: https://en.bitcoin.it/wiki/Protocol_rules - let mut new_target = old_target * actual_timespan; + let mut new_target = (old_target * actual_timespan).unwrap_or(Uint256::MAX); new_target = new_target / target_timespan; new_target = if new_target > difficulty_limit { From 49da717c45702dea2990d7edf0f348aedab8e768 Mon Sep 17 00:00:00 2001 From: Samer Afach Date: Fri, 29 Sep 2023 03:43:42 +0400 Subject: [PATCH 3/6] Enforce used checked_div and checked_rem --- .../reward_distribution.rs | 3 +- common/src/chain/block/consensus_data.rs | 2 +- common/src/chain/pos.rs | 6 ++- common/src/chain/pow.rs | 5 ++- common/src/uint/impls.rs | 41 +++++++++++++------ consensus/src/pos/target.rs | 22 ++++++---- consensus/src/pow/helpers.rs | 9 ++-- consensus/src/pow/mod.rs | 12 +++--- 8 files changed, 65 insertions(+), 35 deletions(-) diff --git a/chainstate/tx-verifier/src/transaction_verifier/reward_distribution.rs b/chainstate/tx-verifier/src/transaction_verifier/reward_distribution.rs index 635c210e95..cbe08aa1ec 100644 --- a/chainstate/tx-verifier/src/transaction_verifier/reward_distribution.rs +++ b/chainstate/tx-verifier/src/transaction_verifier/reward_distribution.rs @@ -192,7 +192,8 @@ fn calculate_rewards_per_delegation( |(delegation_id, balance_amount)| -> Result<_, ConnectTransactionError> { let balance = Uint256::from_amount(*balance_amount); let numer = (total_delegations_reward * balance).expect("Source types are smaller"); - let reward = numer / total_delegations_balance; + let reward = (numer / total_delegations_balance) + .ok_or(ConnectTransactionError::TotalDelegationBalanceZero(pool_id))?; let reward: common::primitives::amount::UnsignedIntType = reward.try_into().map_err(|_| { ConnectTransactionError::DelegationRewardOverflow( diff --git a/common/src/chain/block/consensus_data.rs b/common/src/chain/block/consensus_data.rs index b12e47526f..695a11c82a 100644 --- a/common/src/chain/block/consensus_data.rs +++ b/common/src/chain/block/consensus_data.rs @@ -146,7 +146,7 @@ impl PoWData { let mut ret = !target; let mut ret1 = target; ret1.increment(); - ret = ret / ret1; + ret = (ret / ret1)?; ret.increment(); Some(ret) } diff --git a/common/src/chain/pos.rs b/common/src/chain/pos.rs index cdb28112ba..66785241d0 100644 --- a/common/src/chain/pos.rs +++ b/common/src/chain/pos.rs @@ -162,7 +162,8 @@ const DEFAULT_MATURITY_DISTANCE: BlockDistance = BlockDistance::new(2000); pub fn create_testnet_pos_config(consensus_version: PoSConsensusVersion) -> PoSChainConfig { let target_block_time = NonZeroU64::new(2 * 60).expect("cannot be 0"); - let target_limit = Uint256::MAX / Uint256::from_u64(target_block_time.get()); + let target_limit = (Uint256::MAX / Uint256::from_u64(target_block_time.get())) + .expect("Target block time cannot be zero as per NonZeroU64"); PoSChainConfig { target_limit, @@ -189,7 +190,8 @@ pub fn create_unittest_pos_config() -> PoSChainConfig { pub fn create_regtest_pos_config(consensus_version: PoSConsensusVersion) -> PoSChainConfig { let target_block_time = NonZeroU64::new(2 * 60).expect("cannot be 0"); - let target_limit = Uint256::MAX / Uint256::from_u64(target_block_time.get()); + let target_limit = (Uint256::MAX / Uint256::from_u64(target_block_time.get())) + .expect("Target block time cannot be zero as per NonZeroU64"); PoSChainConfig { target_limit, diff --git a/common/src/chain/pow.rs b/common/src/chain/pow.rs index 70a8938f28..755a9f8611 100644 --- a/common/src/chain/pow.rs +++ b/common/src/chain/pow.rs @@ -189,8 +189,9 @@ mod tests { 0xFFFFFFFFFFFFFFFF, ]); - let target_max = - target_max / Uint256::from_u64(mainnet_cfg.target_timespan().as_secs() * 4); + let target_max = (target_max + / Uint256::from_u64(mainnet_cfg.target_timespan().as_secs() * 4)) + .unwrap(); assert!(mainnet_cfg.limit() < target_max); } } diff --git a/common/src/uint/impls.rs b/common/src/uint/impls.rs index 94470f1b6f..0f9c8a493b 100644 --- a/common/src/uint/impls.rs +++ b/common/src/uint/impls.rs @@ -262,6 +262,14 @@ macro_rules! construct_uint { a.overflowing_add(&one).0 } + fn unchecked_div(&self, other: &Self) -> Self { + self.div_rem(*other).0 + } + + fn unchecked_rem(&self, other: &Self) -> Self { + self.div_rem(*other).1 + } + // TODO: use checked functions for all operations // mintlayer/mintlayer-core/-/issues/1132 pub fn checked_add(&self, other: &Self) -> Option { @@ -311,7 +319,11 @@ macro_rules! construct_uint { } pub fn checked_div(&self, other: &Self) -> Option { - (*other != Self::ZERO).then(|| *self / *other) + (*other != Self::ZERO).then(|| self.unchecked_div(other)) + } + + pub fn checked_rem(&self, other: &Self) -> Option { + (*other != Self::ZERO).then(|| self.unchecked_rem(other)) } pub fn overflowing_mul(&self, other: &Self) -> Self { @@ -416,18 +428,18 @@ macro_rules! construct_uint { } impl ::core::ops::Div<$name> for $name { - type Output = $name; + type Output = Option<$name>; - fn div(self, other: $name) -> $name { - self.div_rem(other).0 + fn div(self, other: $name) -> Option<$name> { + self.checked_div(&other) } } impl ::core::ops::Rem<$name> for $name { - type Output = $name; + type Output = Option<$name>; - fn rem(self, other: $name) -> $name { - self.div_rem(other).1 + fn rem(self, other: $name) -> Option<$name> { + self.checked_rem(&other) } } @@ -893,26 +905,29 @@ mod tests { ); // Division assert_eq!( - Uint256::from_u64(105) / Uint256::from_u64(5), + (Uint256::from_u64(105) / Uint256::from_u64(5)).unwrap(), Uint256::from_u64(21) ); - let div = mult / Uint256::from_u64(300); + let div = (mult / Uint256::from_u64(300)).unwrap(); assert_eq!( div, Uint256([0x9F30411021524112u64, 0x0001BD5B7DDFBD5A, 0, 0]) ); assert_eq!( - Uint256::from_u64(105) % Uint256::from_u64(5), + (Uint256::from_u64(105) % Uint256::from_u64(5)).unwrap(), Uint256::from_u64(0) ); assert_eq!( - Uint256::from_u64(35498456) % Uint256::from_u64(3435), + (Uint256::from_u64(35498456) % Uint256::from_u64(3435)).unwrap(), Uint256::from_u64(1166) ); let m = (mult * Uint256::from_u64(39842)).unwrap(); let rem_src = (m + Uint256::from_u64(9054)).unwrap(); - assert_eq!(rem_src % Uint256::from_u64(39842), Uint256::from_u64(9054)); + assert_eq!( + (rem_src % Uint256::from_u64(39842)).unwrap(), + Uint256::from_u64(9054) + ); // TODO: bit inversion } @@ -1270,7 +1285,7 @@ mod tests { { let a: Uint128 = rng.gen::().into(); let b: Uint128 = rng.gen::().into(); - assert_eq!(a.checked_div(&b), Some(a / b)); + assert_eq!(a.checked_div(&b), Some(a.unchecked_div(&b))); } } } diff --git a/consensus/src/pos/target.rs b/consensus/src/pos/target.rs index d87746adc6..be6285f3aa 100644 --- a/consensus/src/pos/target.rs +++ b/consensus/src/pos/target.rs @@ -83,13 +83,21 @@ fn calculate_new_target( ); let prev_target: Uint512 = (*prev_target).into(); - let new_target = (prev_target * actual_block_time) - .expect("Source types are smaller than these types") - / target_block_time; - let difficulty_change_limit = (prev_target - * Uint512::from_u64(pos_config.difficulty_change_limit().value().into())) - .expect("Original types are smaller") - / Uint512::from_u64(1000); + let new_target = { + let numerator = + (prev_target * actual_block_time).expect("Source types are smaller than these types"); + let denominator = target_block_time; + (numerator / denominator).expect("Target block time is not zero given its type") + }; + + let difficulty_change_limit = { + let numerator = (prev_target + * Uint512::from_u64(pos_config.difficulty_change_limit().value().into())) + .expect("Original types are smaller"); + let denominator = Uint512::from_u64(1000); + (numerator / denominator).expect("Denominator is not zero") + }; + let lower_limit = (prev_target - difficulty_change_limit) .expect("Cannot fail because difficulty_change_limit is in [0, 1]"); let upper_limit = (prev_target + difficulty_change_limit) diff --git a/consensus/src/pow/helpers.rs b/consensus/src/pow/helpers.rs index 5972efe2ba..6f984015be 100644 --- a/consensus/src/pow/helpers.rs +++ b/consensus/src/pow/helpers.rs @@ -13,6 +13,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::num::NonZeroU64; + use chainstate_types::{BlockIndex, GenBlockIndex, PropertyQueryError}; use common::{ chain::block::timestamp::BlockTimestamp, @@ -62,13 +64,13 @@ where /// `difficulty_limit` - found in the PoWChainConfig, as `limit` pub fn calculate_new_target( actual_timespan_of_last_interval: u64, - target_timespan: u64, + target_timespan: NonZeroU64, old_target: Compact, difficulty_limit: Uint256, ) -> Result { let actual_timespan = Uint256::from_u64(actual_timespan_of_last_interval); - let target_timespan = Uint256::from_u64(target_timespan); + let target_timespan = Uint256::from_u64(target_timespan.get()); let old_target = Uint256::try_from(old_target) .map_err(|_| ConsensusPoWError::PreviousBitsDecodingFailed(old_target))?; @@ -76,7 +78,8 @@ pub fn calculate_new_target( // new target is computed by multiplying the old target by ratio of the actual timespan / target timespan. // see Bitcoin's Protocol rules of Difficulty change: https://en.bitcoin.it/wiki/Protocol_rules let mut new_target = (old_target * actual_timespan).unwrap_or(Uint256::MAX); - new_target = new_target / target_timespan; + new_target = + (new_target / target_timespan).expect("target_timespan cannot be zero as per its type"); new_target = if new_target > difficulty_limit { difficulty_limit diff --git a/consensus/src/pow/mod.rs b/consensus/src/pow/mod.rs index 629451bafe..3a982a50da 100644 --- a/consensus/src/pow/mod.rs +++ b/consensus/src/pow/mod.rs @@ -23,7 +23,7 @@ mod helpers; pub mod input_data; mod work; -use std::time::Duration; +use std::{num::NonZeroU64, time::Duration}; use common::{ chain::{ChainConfig, PoWChainConfig}, @@ -57,24 +57,24 @@ impl PoW { self.0.max_retarget_factor() } - pub fn target_timespan_in_secs(&self) -> u64 { - self.0.target_timespan().as_secs() + pub fn target_timespan_in_secs(&self) -> NonZeroU64 { + NonZeroU64::new(self.0.target_timespan().as_secs()).expect("Invalid initialization of PoW") } /// Follows the upper bound of the target timespan (2 weeks * 4) of Bitcoin. /// See Bitcoin's Protocol rules on [Difficulty change](https://en.bitcoin.it/wiki/Protocol_rules) pub fn max_target_timespan_in_secs(&self) -> u64 { - self.target_timespan_in_secs() * self.max_retarget_factor() + self.target_timespan_in_secs().get() * self.max_retarget_factor() } /// Follows the lower bound of the target timespan (2 weeks / 4) of Bitcoin. /// See Bitcoin's Protocol rules on [Difficulty change](https://en.bitcoin.it/wiki/Protocol_rules) pub fn min_target_timespan_in_secs(&self) -> u64 { - self.target_timespan_in_secs() / self.max_retarget_factor() + self.target_timespan_in_secs().get() / self.max_retarget_factor() } pub fn difficulty_adjustment_interval(&self) -> u64 { // or a total of 2016 blocks - self.target_timespan_in_secs() / self.target_spacing().as_secs() + self.target_timespan_in_secs().get() / self.target_spacing().as_secs() } } From a7e6ec817b40671cbf1c9dc1514ddb41d3dd2392 Mon Sep 17 00:00:00 2001 From: Samer Afach Date: Fri, 29 Sep 2023 04:17:50 +0400 Subject: [PATCH 4/6] Make all unsafe methods private in Uint* types --- common/src/uint/impls.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/common/src/uint/impls.rs b/common/src/uint/impls.rs index 0f9c8a493b..68c18d8f35 100644 --- a/common/src/uint/impls.rs +++ b/common/src/uint/impls.rs @@ -65,7 +65,7 @@ macro_rules! construct_uint { self.widening_mul_u64(other as u64).0 } - pub fn widening_mul_u64(self, rhs: u64) -> (Self, u64) { + fn widening_mul_u64(self, rhs: u64) -> (Self, u64) { const U64_MAX: u128 = u64::MAX as u128; let lhs = &self.0; let mut res = [0u64; $n_words]; @@ -235,11 +235,7 @@ macro_rules! construct_uint { } } - pub fn overflowing_add_with_carry( - &self, - other: &Self, - mut carry: bool, - ) -> (Self, bool) { + fn overflowing_add_with_carry(&self, other: &Self, mut carry: bool) -> (Self, bool) { let lhs = &self.0; let rhs = &other.0; let mut ret = [0u64; $n_words]; @@ -252,7 +248,7 @@ macro_rules! construct_uint { (Self(ret), carry) } - pub fn overflowing_add(&self, other: &Self) -> (Self, bool) { + fn overflowing_add(&self, other: &Self) -> (Self, bool) { self.overflowing_add_with_carry(other, false) } @@ -270,8 +266,6 @@ macro_rules! construct_uint { self.div_rem(*other).1 } - // TODO: use checked functions for all operations - // mintlayer/mintlayer-core/-/issues/1132 pub fn checked_add(&self, other: &Self) -> Option { let (result, carry) = self.overflowing_add(other); (!carry).then_some(result) @@ -326,7 +320,8 @@ macro_rules! construct_uint { (*other != Self::ZERO).then(|| self.unchecked_rem(other)) } - pub fn overflowing_mul(&self, other: &Self) -> Self { + #[allow(dead_code)] + fn overflowing_mul(&self, other: &Self) -> Self { self.widening_mul(other).0 } } From 7f3442c62ab4e3468ecd8ec5b748a940346d7745 Mon Sep 17 00:00:00 2001 From: Samer Afach Date: Fri, 29 Sep 2023 04:21:50 +0400 Subject: [PATCH 5/6] Removed unused multiplication function --- common/src/uint/impls.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/common/src/uint/impls.rs b/common/src/uint/impls.rs index 68c18d8f35..1f1200ace5 100644 --- a/common/src/uint/impls.rs +++ b/common/src/uint/impls.rs @@ -319,11 +319,6 @@ macro_rules! construct_uint { pub fn checked_rem(&self, other: &Self) -> Option { (*other != Self::ZERO).then(|| self.unchecked_rem(other)) } - - #[allow(dead_code)] - fn overflowing_mul(&self, other: &Self) -> Self { - self.widening_mul(other).0 - } } impl From<[u8; $n_words * 8]> for $name { @@ -1258,7 +1253,7 @@ mod tests { { let a = Uint128::from_u64(rng.gen::()); let b = Uint128::from_u64(rng.gen::()); - assert_eq!(a.checked_mul(&b), Some(a.overflowing_mul(&b))); + assert_eq!(a.checked_mul(&b), Some(a.widening_mul(&b).0)); } } From 46ed67f5d43d2bc01b9c341a313135f648a2f54a Mon Sep 17 00:00:00 2001 From: Samer Afach Date: Fri, 29 Sep 2023 14:01:40 +0400 Subject: [PATCH 6/6] Get rid of Uint*::increment() function and fix a bug in get_block_proof of PoW --- common/src/chain/block/consensus_data.rs | 4 +-- common/src/uint/impls.rs | 38 +++++------------------- 2 files changed, 10 insertions(+), 32 deletions(-) diff --git a/common/src/chain/block/consensus_data.rs b/common/src/chain/block/consensus_data.rs index 695a11c82a..4a939866c9 100644 --- a/common/src/chain/block/consensus_data.rs +++ b/common/src/chain/block/consensus_data.rs @@ -145,9 +145,9 @@ impl PoWData { let target: Uint256 = self.bits.try_into().ok()?; let mut ret = !target; let mut ret1 = target; - ret1.increment(); + ret1 = (ret1 + Uint256::ONE)?; ret = (ret / ret1)?; - ret.increment(); + ret = (ret + Uint256::ONE).unwrap_or(Uint256::MAX); Some(ret) } } diff --git a/common/src/uint/impls.rs b/common/src/uint/impls.rs index 1f1200ace5..ac4cfce052 100644 --- a/common/src/uint/impls.rs +++ b/common/src/uint/impls.rs @@ -223,18 +223,6 @@ macro_rules! construct_uint { ($name(ret), sub_copy) } - /// Increment by 1 - #[inline] - pub fn increment(&mut self) { - let &mut $name(ref mut arr) = self; - for i in 0..$n_words { - arr[i] = arr[i].wrapping_add(1); - if arr[i] != 0 { - break; - } - } - } - fn overflowing_add_with_carry(&self, other: &Self, mut carry: bool) -> (Self, bool) { let lhs = &self.0; let rhs = &other.0; @@ -876,7 +864,7 @@ mod tests { ); // Increment let mut incr = shr; - incr.increment(); + incr = (incr + 1u64.into()).unwrap(); assert_eq!( incr, Uint256([0x7DDE000000000001u64, 0x0001BD5B7DDFBD5B, 0, 0]) @@ -1014,7 +1002,7 @@ mod tests { 0xFFFFFFFFFFFFFFFFu64, 0xEFFFFFFFFFFFFFFFu64, ]); - val.increment(); + val = (val + 1u64.into()).unwrap(); assert_eq!( val, Uint256([ @@ -1024,7 +1012,7 @@ mod tests { 0xEFFFFFFFFFFFFFFFu64, ]) ); - val.increment(); + val = (val + 1u64.into()).unwrap(); assert_eq!( val, Uint256([ @@ -1035,31 +1023,21 @@ mod tests { ]) ); - let mut val = Uint256([ + let val = Uint256([ 0xFFFFFFFFFFFFFFFFu64, 0xFFFFFFFFFFFFFFFFu64, 0xFFFFFFFFFFFFFFFFu64, 0xFFFFFFFFFFFFFFFFu64, ]); - val.increment(); - assert_eq!( - val, - Uint256([ - 0x0000000000000000u64, - 0x0000000000000000u64, - 0x0000000000000000u64, - 0x0000000000000000u64, - ]) - ); + assert!((val + 1u64.into()).is_none()); let max_val_u256 = Uint256::MAX; let mut a = Uint512::from(max_val_u256); - a.increment(); + a = (a + 1u64.into()).unwrap(); assert_eq!(a, Uint512([0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00])); - let mut max_val = Uint512::MAX; - max_val.increment(); - assert_eq!(max_val, Uint512::ZERO); + let max_val = Uint512::MAX; + assert!((max_val + 1u64.into()).is_none()); } #[test]