Skip to content

Commit

Permalink
Broker pallet: fix interlacing (paritytech#2811)
Browse files Browse the repository at this point in the history
With the current code, when a user interlaces their region, the end
result will be three regions in the state:
- the non-interlaced region
- first part of the interlaced region
- second part of the interlaced region

The existing implementation retains the non-interlaced region in the
state, leading to a problematic scenario:

1. User 1 acquires a region from the market.
2. User 1 then interlaces this region.
3. Subsequently, User 1 transfers one part of the interlaced regions to
User 2.
Despite this transfer, User 1 retains the ability to assign the entire
original non-interlaced region, which is inconsistent with the fact that
they no longer own one of the interlaced parts.

This PR resolves the issue by removing the original region, ensuring
that only the two new interlaced regions remain in the state.
  • Loading branch information
Szegoo authored Dec 29, 2023
1 parent 494a716 commit 249f993
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 0 deletions.
3 changes: 3 additions & 0 deletions substrate/frame/broker/src/dispatchable_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,9 @@ impl<T: Config> Pallet<T> {
ensure!(!pivot.is_void(), Error::<T>::VoidPivot);
ensure!(pivot != region_id.mask, Error::<T>::CompletePivot);

// The old region should be removed.
Regions::<T>::remove(&region_id);

let one = RegionId { mask: pivot, ..region_id };
Regions::<T>::insert(&one, &region);
let other = RegionId { mask: region_id.mask ^ pivot, ..region_id };
Expand Down
37 changes: 37 additions & 0 deletions substrate/frame/broker/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,43 @@ fn interlace_works() {
});
}

#[test]
fn cant_assign_unowned_region() {
TestExt::new().endow(1, 1000).execute_with(|| {
assert_ok!(Broker::do_start_sales(100, 1));
advance_to(2);
let region = Broker::do_purchase(1, u64::max_value()).unwrap();
let (region1, region2) =
Broker::do_interlace(region, Some(1), CoreMask::from_chunk(0, 30)).unwrap();

// Transfer the interlaced region to account 2.
assert_ok!(Broker::do_transfer(region2, Some(1), 2));

// The initial owner should not be able to assign the non-interlaced region, since they have
// just transferred an interlaced part of it to account 2.
assert_noop!(Broker::do_assign(region, Some(1), 1001, Final), Error::<Test>::UnknownRegion);

// Account 1 can assign only the interlaced region that they did not transfer.
assert_ok!(Broker::do_assign(region1, Some(1), 1001, Final));
// Account 2 can assign the region they received.
assert_ok!(Broker::do_assign(region2, Some(2), 1002, Final));

advance_to(10);
assert_eq!(
CoretimeTrace::get(),
vec![(
6,
AssignCore {
core: 0,
begin: 8,
assignment: vec![(Task(1001), 21600), (Task(1002), 36000)],
end_hint: None
}
),]
);
});
}

#[test]
fn interlace_then_partition_works() {
TestExt::new().endow(1, 1000).execute_with(|| {
Expand Down

0 comments on commit 249f993

Please sign in to comment.