From fa1a33d3fa9fdc3a288413ea388d1239ad8f0b7d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 22 Sep 2021 20:32:32 -0700 Subject: [PATCH] Cater for edge case of heavier being head --- frame/bags-list/src/list/mod.rs | 85 +++++++----- frame/bags-list/src/list/tests.rs | 5 +- frame/bags-list/src/tests.rs | 209 +++++++++++++++++------------- 3 files changed, 169 insertions(+), 130 deletions(-) diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 3d96089535531..0fb5474e1bea7 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -395,58 +395,75 @@ impl List { use frame_support::ensure; let lighter_node = Node::::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?; - let mut heavier_node = Node::::get(&heavier_id).ok_or(pallet::Error::IdNotFound)?; + let heavier_node = Node::::get(&heavier_id).ok_or(pallet::Error::IdNotFound)?; ensure!(lighter_node.bag_upper == heavier_node.bag_upper, pallet::Error::NotInSameBag); // this is the most expensive check, so we do it last. ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), pallet::Error::NotHeavier); - // we need to write the bag to storage if its head and/or tail is updated. - if lighter_node.is_terminal() || heavier_node.is_terminal() { - let mut bag = Bag::::get(lighter_node.bag_upper).ok_or_else(|| { - debug_assert!(false, "bag that should exist cannot be found"); - crate::log!(warn, "bag that should exist cannot be found"); - pallet::Error::BagNotFound - })?; - debug_assert!(bag.iter().count() > 1); + // remove the heavier node from this list. Note that this removes the node from storage and + // decrements the node counter. + Self::remove(&heavier_id); - if bag.head.as_ref() == Some(lighter_id) { - debug_assert!(lighter_node.next.is_some(), "lighter node must have next if head"); - bag.head = Some(heavier_id.clone()); - } + // re-fetch `lighter_node` from storage since it may have been updated when `heavier_node` + // was removed. + let lighter_node = Node::::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?; - // re-assign bag tail if lighter is the tail - if bag.tail.as_ref() == Some(heavier_id) { - debug_assert!(heavier_node.prev.is_some(), "heavier node must have prev if tail"); - bag.tail = heavier_node.prev.clone(); - } + Self::insert_at(lighter_node, heavier_node)?; - // within this block we know the bag must have been updated, so we update storage. - bag.put() - } + // since `insert_at` was successful, increment the counter, which got decremented by + // `Self::remove`. + crate::CounterForListNodes::::mutate(|prev_count| { + *prev_count = prev_count.saturating_add(1) + }); - // cut heavier out of the list, updating its neighbors. - heavier_node.excise(); + Ok(()) + } - // re-fetch `lighter_node` from storage since it may have been updated when `heavier_node` was - // excised. - let mut lighter_node = Node::::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?; + /// Insert `node` directly in front of `at`. + fn insert_at(mut at: Node, mut node: Node) -> Result<(), crate::pallet::Error> { + use crate::pallet; // connect `heavier_node` to its new `prev`. - if let Some(mut prev) = lighter_node.prev() { - prev.next = Some(heavier_id.clone()); + node.prev = at.prev.clone(); + if let Some(mut prev) = at.prev() { + prev.next = Some(node.id().clone()); prev.put() } - heavier_node.prev = lighter_node.prev; - // connect `heavier_node` and `lighter`. - heavier_node.next = Some(lighter_id.clone()); - lighter_node.prev = Some(heavier_id.clone()); + // connect `node` and `at`. + node.next = Some(at.id().clone()); + at.prev = Some(node.id().clone()); + + if at.is_terminal() || node.is_terminal() { + // `at` is the tail and/or `node` is the head, so we make sure the bag is updated. Note, + // since `node` is always in front of `at` we know that 1) there is always at least 2 + // nodes in the bag, and 2) only `node` could be the head and only `at` could be the + // tail. + let mut bag = Bag::::get(at.bag_upper).ok_or_else(|| { + debug_assert!(false, "bag that should exist cannot be found"); + crate::log!(warn, "bag that should exist cannot be found"); + pallet::Error::BagNotFound + })?; + + if node.prev == None { + bag.head = Some(node.id().clone()) + } + + if at.next == None { + bag.tail = Some(at.id().clone()) + } + + debug_assert!(at.prev.is_some(), "`at` cannot be the head"); + debug_assert!(node.next.is_some(), "`node` cannot be the tail"); + + bag.put() + }; // write the updated nodes to storage. - lighter_node.put(); - heavier_node.put(); + at.put(); + node.put(); Ok(()) } diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index afa66b62c1b47..b91bc07668699 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -376,10 +376,7 @@ mod list { } #[test] - #[cfg_attr( - debug_assertions, - should_panic = "bag that should exist cannot be found" - )] + #[cfg_attr(debug_assertions, should_panic = "bag that should exist cannot be found")] fn put_in_front_of_exits_early_if_bag_not_found() { ExtBuilder::default().build_and_execute_no_post_check(|| { let node_710_no_bag = diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs index a3ba9596cc0e3..39f16220b1ffc 100644 --- a/frame/bags-list/src/tests.rs +++ b/frame/bags-list/src/tests.rs @@ -198,7 +198,7 @@ mod pallet { } #[test] - fn put_in_front_of_with_two_node_bag() { + fn put_in_front_of_two_node_bag() { ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| { // given assert_eq!( @@ -221,122 +221,147 @@ mod pallet { } #[test] - fn put_in_front_of_with_non_terminal_nodes() { - ExtBuilder::default() - .add_ids(vec![(11, 20), (710, 15), (711, 16), (12, 20)]) - .build_and_execute(|| { - // given - assert_eq!( - List::::get_bags(), - vec![(10, vec![1]), (20, vec![11, 710, 711, 12]), (1_000, vec![2, 3, 4])] - ); - - assert_eq!(::VoteWeightProvider::vote_weight(&710), 15); - assert_eq!(::VoteWeightProvider::vote_weight(&711), 16); - - // when - assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711)); - - // then - assert_eq!( - List::::get_bags(), - vec![(10, vec![1]), (20, vec![11, 711, 710, 12]), (1_000, vec![2, 3, 4])] - ); - }); + fn put_in_front_of_two_node_bag_heavier_is_head() { + ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| { + // given + assert_eq!( + List::::get_bags(), + vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])] + ); + + assert_eq!(::VoteWeightProvider::vote_weight(&710), 15); + assert_eq!(::VoteWeightProvider::vote_weight(&711), 16); + + // when + assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711)); + + // then + assert_eq!( + List::::get_bags(), + vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])] + ); + }); } #[test] - fn put_in_front_of_with_lighter_as_head() { - ExtBuilder::default() - .add_ids(vec![(710, 15), (711, 16), (12, 20)]) - .build_and_execute(|| { - // given - assert_eq!( - List::::get_bags(), - vec![(10, vec![1]), (20, vec![710, 711, 12]), (1_000, vec![2, 3, 4])] - ); - - assert_eq!(::VoteWeightProvider::vote_weight(&710), 15); - assert_eq!(::VoteWeightProvider::vote_weight(&711), 16); - - // when - assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711)); - - // then - assert_eq!( - List::::get_bags(), - vec![(10, vec![1]), (20, vec![711, 710, 12]), (1_000, vec![2, 3, 4])] - ); - }); + fn put_in_front_of_head_and_tail_non_terminal_nodes() { + ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| { + // given + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]); + + StakingMock::set_vote_weight_of(&4, 1_000); + StakingMock::set_vote_weight_of(&3, 999); + + // when + assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 3, 4)); + + // then + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3, 5])]); + }); } #[test] - fn put_in_front_of_with_heavier_as_tail() { - ExtBuilder::default() - .add_ids(vec![(11, 20), (710, 15), (711, 16)]) - .build_and_execute(|| { - // given - assert_eq!( - List::::get_bags(), - vec![(10, vec![1]), (20, vec![11, 710, 711]), (1_000, vec![2, 3, 4])] - ); - - assert_eq!(::VoteWeightProvider::vote_weight(&710), 15); - assert_eq!(::VoteWeightProvider::vote_weight(&711), 16); - - // when - assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711)); - - // then - assert_eq!( - List::::get_bags(), - vec![(10, vec![1]), (20, vec![11, 711, 710]), (1_000, vec![2, 3, 4])] - ); - }); + fn put_in_front_of_lighter_is_head_heavier_is_non_terminal() { + ExtBuilder::default().build_and_execute(|| { + // given + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); + + StakingMock::set_vote_weight_of(&3, 1_000); + StakingMock::set_vote_weight_of(&2, 999); + + // when + assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 2, 3)); + + // then + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4])]); + }); } #[test] - fn put_in_front_of_with_heavier_and_lighter_terminal_multiple_middle_nodes() { - ExtBuilder::default() - .add_ids(vec![(710, 15), (11, 20), (12, 20), (711, 15)]) - .build_and_execute(|| { - // given - assert_eq!( - List::::get_bags(), - vec![(10, vec![1]), (20, vec![710, 11, 12, 711]), (1_000, vec![2, 3, 4])] - ); - - assert_eq!(::VoteWeightProvider::vote_weight(&710), 15); - assert_eq!(::VoteWeightProvider::vote_weight(&711), 16); - - // when - assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711)); - - // then - assert_eq!( - List::::get_bags(), - vec![(10, vec![1]), (20, vec![711, 710, 11, 12]), (1_000, vec![2, 3, 4])] - ); - }); + fn put_in_front_of_heavier_is_tail_lighter_is_non_terminal() { + ExtBuilder::default().build_and_execute(|| { + // given + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); + + StakingMock::set_vote_weight_of(&4, 1_000); + StakingMock::set_vote_weight_of(&3, 999); + + // when + assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 3, 4)); + + // then + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3])]); + }); } #[test] - fn heavier_is_head_lighter_is_not_terminal() { + fn put_in_front_of_heavier_is_tail_lighter_is_head() { ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| { // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]); - StakingMock::set_vote_weight_of(&4, 1_000); + StakingMock::set_vote_weight_of(&5, 1_000); StakingMock::set_vote_weight_of(&2, 999); // when - BagsList::put_in_front_of(Origin::signed(0), 2, 4).unwrap(); + assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 2, 5)); + + // then + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![5, 2, 3, 4])]); + }); + } + + #[test] + fn put_in_front_of_heavier_is_head_lighter_is_not_terminal() { + ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| { + // given + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]); + + StakingMock::set_vote_weight_of(&2, 1_000); + StakingMock::set_vote_weight_of(&4, 999); + + // when + BagsList::put_in_front_of(Origin::signed(0), 4, 2).unwrap(); // then assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4, 5])]); }); } + #[test] + fn put_in_front_of_lighter_is_tail_heavier_is_not_terminal() { + ExtBuilder::default().add_ids(vec![(5, 900)]).build_and_execute(|| { + // given + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]); + + StakingMock::set_vote_weight_of(&5, 900); + StakingMock::set_vote_weight_of(&3, 1_000); + + // when + BagsList::put_in_front_of(Origin::signed(0), 5, 3).unwrap(); + + // then + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3, 5])]); + }); + } + + #[test] + fn put_in_front_of_lighter_is_tail_heavier_is_head() { + ExtBuilder::default().build_and_execute(|| { + // given + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); + + StakingMock::set_vote_weight_of(&2, 1_000); + StakingMock::set_vote_weight_of(&4, 999); + + // when + BagsList::put_in_front_of(Origin::signed(0), 4, 2).unwrap(); + + // then + assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4])]); + }); + } + #[test] fn put_in_front_of_errors_if_heavier_is_less_than_lighter() { ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {