From 8c423baf32ab0fe7a055a9fa66779082aa0ac98a Mon Sep 17 00:00:00 2001
From: Davide Galassi <davxy@datawok.net>
Date: Fri, 11 Nov 2022 16:22:26 +0100
Subject: [PATCH] Safe TreeRoute constructor (#12691)
* Safe TreeRoute constructor
* Remove test duplicate
* Better tree route error info
---
substrate/client/db/src/lib.rs | 8 ++++
.../transaction-pool/src/enactment_state.rs | 42 +++++++++++--------
.../blockchain/src/header_metadata.rs | 14 +++++--
3 files changed, 43 insertions(+), 21 deletions(-)
diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs
index 3bbff1625f2..305db2284b2 100644
--- a/substrate/client/db/src/lib.rs
+++ b/substrate/client/db/src/lib.rs
@@ -2796,6 +2796,14 @@ pub(crate) mod tests {
let b1 = insert_header(&backend, 1, block0, None, H256::from([1; 32]));
let b2 = insert_header(&backend, 2, b1, None, Default::default());
+ {
+ let tree_route = tree_route(blockchain, a1, a1).unwrap();
+
+ assert_eq!(tree_route.common_block().hash, a1);
+ assert!(tree_route.retracted().is_empty());
+ assert!(tree_route.enacted().is_empty());
+ }
+
{
let tree_route = tree_route(blockchain, a3, b2).unwrap();
diff --git a/substrate/client/transaction-pool/src/enactment_state.rs b/substrate/client/transaction-pool/src/enactment_state.rs
index b347de824fa..6aac98641cf 100644
--- a/substrate/client/transaction-pool/src/enactment_state.rs
+++ b/substrate/client/transaction-pool/src/enactment_state.rs
@@ -231,14 +231,19 @@ mod enactment_state_tests {
}
};
- Ok(TreeRoute::new(vec, pivot))
+ TreeRoute::new(vec, pivot)
}
mod mock_tree_route_tests {
use super::*;
/// asserts that tree routes are equal
- fn assert_treeroute_eq(expected: TreeRoute<Block>, result: TreeRoute<Block>) {
+ fn assert_treeroute_eq(
+ expected: Result<TreeRoute<Block>, String>,
+ result: Result<TreeRoute<Block>, String>,
+ ) {
+ let expected = expected.unwrap();
+ let result = result.unwrap();
assert_eq!(result.common_block().hash, expected.common_block().hash);
assert_eq!(result.enacted().len(), expected.enacted().len());
assert_eq!(result.retracted().len(), expected.retracted().len());
@@ -255,65 +260,66 @@ mod enactment_state_tests {
}
// some tests for mock tree_route function
+
#[test]
fn tree_route_mock_test_01() {
- let result = tree_route(b1().hash, a().hash).expect("tree route exists");
+ let result = tree_route(b1().hash, a().hash);
let expected = TreeRoute::new(vec![b1(), a()], 1);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_02() {
- let result = tree_route(a().hash, b1().hash).expect("tree route exists");
+ let result = tree_route(a().hash, b1().hash);
let expected = TreeRoute::new(vec![a(), b1()], 0);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_03() {
- let result = tree_route(a().hash, c2().hash).expect("tree route exists");
+ let result = tree_route(a().hash, c2().hash);
let expected = TreeRoute::new(vec![a(), b2(), c2()], 0);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_04() {
- let result = tree_route(e2().hash, a().hash).expect("tree route exists");
+ let result = tree_route(e2().hash, a().hash);
let expected = TreeRoute::new(vec![e2(), d2(), c2(), b2(), a()], 4);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_05() {
- let result = tree_route(d1().hash, b1().hash).expect("tree route exists");
+ let result = tree_route(d1().hash, b1().hash);
let expected = TreeRoute::new(vec![d1(), c1(), b1()], 2);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_06() {
- let result = tree_route(d2().hash, b2().hash).expect("tree route exists");
+ let result = tree_route(d2().hash, b2().hash);
let expected = TreeRoute::new(vec![d2(), c2(), b2()], 2);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_07() {
- let result = tree_route(b1().hash, d1().hash).expect("tree route exists");
+ let result = tree_route(b1().hash, d1().hash);
let expected = TreeRoute::new(vec![b1(), c1(), d1()], 0);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_08() {
- let result = tree_route(b2().hash, d2().hash).expect("tree route exists");
+ let result = tree_route(b2().hash, d2().hash);
let expected = TreeRoute::new(vec![b2(), c2(), d2()], 0);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_09() {
- let result = tree_route(e2().hash, e1().hash).expect("tree route exists");
+ let result = tree_route(e2().hash, e1().hash);
let expected =
TreeRoute::new(vec![e2(), d2(), c2(), b2(), a(), b1(), c1(), d1(), e1()], 4);
assert_treeroute_eq(result, expected);
@@ -321,49 +327,49 @@ mod enactment_state_tests {
#[test]
fn tree_route_mock_test_10() {
- let result = tree_route(e1().hash, e2().hash).expect("tree route exists");
+ let result = tree_route(e1().hash, e2().hash);
let expected =
TreeRoute::new(vec![e1(), d1(), c1(), b1(), a(), b2(), c2(), d2(), e2()], 4);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_11() {
- let result = tree_route(b1().hash, c2().hash).expect("tree route exists");
+ let result = tree_route(b1().hash, c2().hash);
let expected = TreeRoute::new(vec![b1(), a(), b2(), c2()], 1);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_12() {
- let result = tree_route(d2().hash, b1().hash).expect("tree route exists");
+ let result = tree_route(d2().hash, b1().hash);
let expected = TreeRoute::new(vec![d2(), c2(), b2(), a(), b1()], 3);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_13() {
- let result = tree_route(c2().hash, e1().hash).expect("tree route exists");
+ let result = tree_route(c2().hash, e1().hash);
let expected = TreeRoute::new(vec![c2(), b2(), a(), b1(), c1(), d1(), e1()], 2);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_14() {
- let result = tree_route(b1().hash, b1().hash).expect("tree route exists");
+ let result = tree_route(b1().hash, b1().hash);
let expected = TreeRoute::new(vec![b1()], 0);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_15() {
- let result = tree_route(b2().hash, b2().hash).expect("tree route exists");
+ let result = tree_route(b2().hash, b2().hash);
let expected = TreeRoute::new(vec![b2()], 0);
assert_treeroute_eq(result, expected);
}
#[test]
fn tree_route_mock_test_16() {
- let result = tree_route(a().hash, a().hash).expect("tree route exists");
+ let result = tree_route(a().hash, a().hash);
let expected = TreeRoute::new(vec![a()], 0);
assert_treeroute_eq(result, expected);
}
diff --git a/substrate/primitives/blockchain/src/header_metadata.rs b/substrate/primitives/blockchain/src/header_metadata.rs
index 1db37b47e4d..87ac4445998 100644
--- a/substrate/primitives/blockchain/src/header_metadata.rs
+++ b/substrate/primitives/blockchain/src/header_metadata.rs
@@ -179,9 +179,17 @@ pub struct TreeRoute<Block: BlockT> {
impl<Block: BlockT> TreeRoute<Block> {
/// Creates a new `TreeRoute`.
///
- /// It is required that `pivot >= route.len()`, otherwise it may panics.
- pub fn new(route: Vec<HashAndNumber<Block>>, pivot: usize) -> Self {
- TreeRoute { route, pivot }
+ /// To preserve the structure safety invariats it is required that `pivot < route.len()`.
+ pub fn new(route: Vec<HashAndNumber<Block>>, pivot: usize) -> Result<Self, String> {
+ if pivot < route.len() {
+ Ok(TreeRoute { route, pivot })
+ } else {
+ Err(format!(
+ "TreeRoute pivot ({}) should be less than route length ({})",
+ pivot,
+ route.len()
+ ))
+ }
}
/// Get a slice of all retracted blocks in reverse order (towards common ancestor).
--
GitLab