Skip to content

Commit 112b3c7

Browse files
jpraynaudjpraynaud
committed
refactor: make BlockRange computable from block number
Also rollback from re-implementation with 'next_multiple_of' as it does not handle edge cases properly.
1 parent 7cf0b3e commit 112b3c7

File tree

5 files changed

+122
-49
lines changed

5 files changed

+122
-49
lines changed

‎mithril-aggregator/src/services/prover.rs‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use async_trait::async_trait;
66
use mithril_common::{
77
crypto_helper::{MKMap, MKMapNode, MKTree, MKTreeNode},
88
entities::{Beacon, BlockRange, CardanoTransactionsSetProof, TransactionHash},
9-
signable_builder::{TransactionRetriever, BLOCK_RANGE_LENGTH},
9+
signable_builder::TransactionRetriever,
1010
StdResult,
1111
};
1212

@@ -51,11 +51,7 @@ impl ProverService for MithrilProverService {
5151
let mut transactions_by_block_ranges: HashMap<BlockRange, Vec<TransactionHash>> =
5252
HashMap::new();
5353
for transaction in &transactions {
54-
let block_range_end = transaction
55-
.block_number
56-
.next_multiple_of(BLOCK_RANGE_LENGTH);
57-
let block_range_start = block_range_end - BLOCK_RANGE_LENGTH;
58-
let block_range = BlockRange::new(block_range_start, block_range_end);
54+
let block_range = BlockRange::from_block_number(transaction.block_number);
5955
if transaction_hashes.contains(&transaction.transaction_hash) {
6056
transactions_to_certify.push((block_range.clone(), transaction));
6157
}

‎mithril-common/src/crypto_helper/merkle_map.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -596,7 +596,7 @@ mod tests {
596596
(
597597
entries
598598
.iter()
599-
.fold(BlockRange::new(0, 0), |mut acc, (range, _)| {
599+
.fold(BlockRange::new(0, 0), |acc, (range, _)| {
600600
acc.try_add(range).unwrap()
601601
}),
602602
MKMapNode::Map(Rc::new(MKMap::new(entries).unwrap())),

‎mithril-common/src/entities/block_range.rs‎

Lines changed: 115 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -14,35 +14,69 @@ use crate::{
1414
/// BlockNumber is the block number of a Cardano transaction.
1515
pub type BlockNumber = u64;
1616

17+
/// BlockRangeLength is the length of a block range.
18+
pub type BlockRangeLength = u64;
19+
1720
/// BlockRange for the Cardano chain
1821
#[derive(Serialize, Deserialize, Clone, Eq, PartialEq, Debug, Hash)]
1922
pub struct BlockRange {
2023
inner_range: Range<u64>,
2124
}
2225

2326
impl BlockRange {
27+
/// The length of the block range
28+
/// Important: this value should be updated with extreme care (probably with an era change) in order to avoid signing disruptions.
29+
pub const LENGTH: BlockRangeLength = 15;
30+
2431
/// BlockRange factory
2532
pub fn new(start: BlockNumber, end: BlockNumber) -> Self {
2633
Self {
2734
inner_range: start..end,
2835
}
2936
}
3037

31-
/// Try to add two BlockRanges
32-
pub fn try_add(&mut self, other: &BlockRange) -> StdResult<BlockRange> {
33-
if self.inner_range.end.max(other.inner_range.end)
34-
< self.inner_range.start.min(other.inner_range.start)
35-
{
38+
cfg_test_tools! {
39+
/// Try to add two BlockRanges
40+
pub fn try_add(&self, other: &BlockRange) -> StdResult<BlockRange> {
41+
if self.inner_range.end.max(other.inner_range.end)
42+
< self.inner_range.start.min(other.inner_range.start)
43+
{
44+
return Err(anyhow!(
45+
"BlockRange cannot be added as they don't strictly overlap"
46+
));
47+
}
48+
49+
Ok(Self {
50+
inner_range: Range {
51+
start: self.inner_range.start.min(other.inner_range.start),
52+
end: self.inner_range.end.max(other.inner_range.end),
53+
},
54+
})
55+
}
56+
}
57+
58+
/// Create a BlockRange from a block number
59+
pub fn from_block_number(number: BlockNumber) -> Self {
60+
// Unwrap is safe as the length is always strictly greater than 0
61+
Self::from_block_number_and_length(number, Self::LENGTH).unwrap()
62+
}
63+
64+
/// Create a BlockRange from a block number and a range length
65+
fn from_block_number_and_length(
66+
number: BlockNumber,
67+
length: BlockRangeLength,
68+
) -> StdResult<Self> {
69+
if length == 0 {
3670
return Err(anyhow!(
37-
"BlockRange cannot be added as they don't strictly overlap"
71+
"BlockRange cannot be be computed with a length of 0"
3872
));
3973
}
40-
74+
// The formula used to compute the lower bound of the block range is `⌊number / length⌋ * length`
75+
// The computation of the floor is done with the integer division `/` of Rust
76+
let block_range_start = (number / length) * length;
77+
let block_range_end = block_range_start + length;
4178
Ok(Self {
42-
inner_range: Range {
43-
start: self.inner_range.start.min(other.inner_range.start),
44-
end: self.inner_range.end.max(other.inner_range.end),
45-
},
79+
inner_range: block_range_start..block_range_end,
4680
})
4781
}
4882
}
@@ -103,30 +137,79 @@ mod tests {
103137

104138
#[test]
105139
fn test_block_range_cmp() {
106-
let range_1 = BlockRange::new(1, 10);
107-
let range_2 = BlockRange::new(1, 10);
108-
let range_3 = BlockRange::new(1, 11);
109-
let range_4 = BlockRange::new(2, 10);
110-
111-
assert_eq!(range_1, range_2);
112-
assert_ne!(range_1, range_3);
113-
assert_ne!(range_1, range_4);
114-
assert_ne!(range_3, range_4);
115-
116-
assert!(range_1 < range_3);
117-
assert!(range_1 < range_4);
118-
assert!(range_3 < range_4);
140+
assert_eq!(BlockRange::new(1, 10), BlockRange::new(1, 10));
141+
assert_ne!(BlockRange::new(1, 10), BlockRange::new(1, 11));
142+
assert_ne!(BlockRange::new(1, 10), BlockRange::new(2, 10));
143+
assert_ne!(BlockRange::new(1, 11), BlockRange::new(2, 10));
144+
145+
assert!(BlockRange::new(1, 10) < BlockRange::new(1, 11));
146+
assert!(BlockRange::new(1, 10) < BlockRange::new(2, 10));
147+
assert!(BlockRange::new(1, 11) < BlockRange::new(2, 10));
119148
}
120149

121150
#[test]
122151
fn test_block_range_try_add() {
123-
let mut range_1 = BlockRange::new(1, 10);
124-
let range_2 = BlockRange::new(1, 10);
125-
let range_3 = BlockRange::new(1, 11);
126-
let range_4 = BlockRange::new(2, 10);
127-
128-
assert_eq!(range_1.try_add(&range_2).unwrap(), range_1);
129-
assert_eq!(range_1.try_add(&range_3).unwrap(), range_3);
130-
assert_eq!(range_1.try_add(&range_4).unwrap(), BlockRange::new(1, 10));
152+
assert_eq!(
153+
BlockRange::new(1, 10)
154+
.try_add(&BlockRange::new(1, 10))
155+
.unwrap(),
156+
BlockRange::new(1, 10)
157+
);
158+
assert_eq!(
159+
BlockRange::new(1, 10)
160+
.try_add(&BlockRange::new(1, 11))
161+
.unwrap(),
162+
BlockRange::new(1, 11)
163+
);
164+
assert_eq!(
165+
BlockRange::new(1, 10)
166+
.try_add(&BlockRange::new(2, 10))
167+
.unwrap(),
168+
BlockRange::new(1, 10)
169+
);
170+
}
171+
172+
#[test]
173+
fn test_block_range_from_number() {
174+
assert_eq!(BlockRange::from_block_number(0), BlockRange::new(0, 15));
175+
assert_eq!(BlockRange::from_block_number(1), BlockRange::new(0, 15));
176+
assert_eq!(BlockRange::from_block_number(14), BlockRange::new(0, 15));
177+
assert_eq!(BlockRange::from_block_number(15), BlockRange::new(15, 30));
178+
assert_eq!(BlockRange::from_block_number(16), BlockRange::new(15, 30));
179+
assert_eq!(BlockRange::from_block_number(29), BlockRange::new(15, 30));
180+
}
181+
182+
#[test]
183+
fn test_block_range_from_number_and_length_with_valid_input() {
184+
assert_eq!(
185+
BlockRange::from_block_number_and_length(0, 10).unwrap(),
186+
BlockRange::new(0, 10)
187+
);
188+
assert_eq!(
189+
BlockRange::from_block_number_and_length(1, 10).unwrap(),
190+
BlockRange::new(0, 10)
191+
);
192+
assert_eq!(
193+
BlockRange::from_block_number_and_length(9, 10).unwrap(),
194+
BlockRange::new(0, 10)
195+
);
196+
assert_eq!(
197+
BlockRange::from_block_number_and_length(10, 10).unwrap(),
198+
BlockRange::new(10, 20)
199+
);
200+
assert_eq!(
201+
BlockRange::from_block_number_and_length(11, 10).unwrap(),
202+
BlockRange::new(10, 20)
203+
);
204+
assert_eq!(
205+
BlockRange::from_block_number_and_length(19, 10).unwrap(),
206+
BlockRange::new(10, 20)
207+
);
208+
}
209+
210+
#[test]
211+
fn test_block_range_from_number_and_length_with_invalid_input() {
212+
BlockRange::from_block_number_and_length(10, 0)
213+
.expect_err("BlockRange should not be computed with a length of 0");
131214
}
132215
}

‎mithril-common/src/entities/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ mod snapshot;
2323
mod type_alias;
2424

2525
pub use beacon::{Beacon, BeaconComparison, BeaconComparisonError};
26-
pub use block_range::{BlockNumber, BlockRange};
26+
pub use block_range::{BlockNumber, BlockRange, BlockRangeLength};
2727
pub use cardano_network::CardanoNetwork;
2828
pub use cardano_transaction::{CardanoTransaction, TransactionHash};
2929
pub use cardano_transactions_set_proof::CardanoTransactionsSetProof;

‎mithril-common/src/signable_builder/cardano_transactions.rs‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,6 @@ use crate::{
2222
#[cfg(test)]
2323
use mockall::automock;
2424

25-
/// The length of the block range
26-
/// Important: this value should be updated with extreme care (probably with an era change) in order to avoid signing disruptions.
27-
pub const BLOCK_RANGE_LENGTH: u64 = 15;
28-
2925
/// Cardano transactions store
3026
#[cfg_attr(test, automock)]
3127
#[async_trait]
@@ -73,11 +69,7 @@ impl CardanoTransactionsSignableBuilder {
7369
let mut transactions_by_block_ranges: HashMap<BlockRange, Vec<TransactionHash>> =
7470
HashMap::new();
7571
for transaction in transactions {
76-
let block_range_end = transaction
77-
.block_number
78-
.next_multiple_of(BLOCK_RANGE_LENGTH);
79-
let block_range_start = block_range_end - BLOCK_RANGE_LENGTH;
80-
let block_range = BlockRange::new(block_range_start, block_range_end);
72+
let block_range = BlockRange::from_block_number(transaction.block_number);
8173
transactions_by_block_ranges
8274
.entry(block_range)
8375
.or_default()
@@ -133,6 +125,8 @@ impl SignableBuilder<Beacon> for CardanoTransactionsSignableBuilder {
133125
.await?;
134126
}
135127

128+
// We temporarily retrieve all the transactions from the database after we have recorded them.
129+
// This avoids order side effects.
136130
let transactions = self.transaction_retriever.get_up_to(&beacon).await?;
137131
let mk_root = self.compute_merkle_root(&transactions)?;
138132

0 commit comments

Comments
 (0)