Skip to content

Commit

Permalink
[fix] Cancel rights distribution (#817)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mateuszaaa
mateuszaaa authored Sep 25, 2024
2 parents 1cbf601 + d055888 commit 5b4d271
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 57 deletions.
37 changes: 21 additions & 16 deletions pallets/rolldown/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,8 @@ pub mod pallet {
pub type AwaitingCancelResolution<T: Config> = StorageMap<
_,
Blake2_128Concat,
(T::ChainId, T::AccountId),
BTreeSet<(u128, DisputeRole)>,
T::ChainId,
BTreeSet<(T::AccountId, u128, DisputeRole)>,
ValueQuery,
>;

Expand Down Expand Up @@ -554,11 +554,11 @@ pub mod pallet {
hash: hash_of_pending_request,
};

AwaitingCancelResolution::<T>::mutate((chain, submitter), |v| {
v.insert((l2_request_id, DisputeRole::Submitter))
AwaitingCancelResolution::<T>::mutate(chain, |v| {
v.insert((submitter.clone(), l2_request_id, DisputeRole::Submitter))
});
AwaitingCancelResolution::<T>::mutate((chain, canceler), |v| {
v.insert((l2_request_id, DisputeRole::Canceler))
AwaitingCancelResolution::<T>::mutate(chain, |v| {
v.insert((canceler, l2_request_id, DisputeRole::Canceler))
});

L2Requests::<T>::insert(
Expand Down Expand Up @@ -1188,11 +1188,11 @@ impl<T: Config> Pallet<T> {
});
}

AwaitingCancelResolution::<T>::mutate((l1, &updater), |v| {
v.remove(&(cancel_request_id, DisputeRole::Submitter))
AwaitingCancelResolution::<T>::mutate(l1, |v| {
v.remove(&(updater, cancel_request_id, DisputeRole::Submitter))
});
AwaitingCancelResolution::<T>::mutate((l1, &canceler), |v| {
v.remove(&(cancel_request_id, DisputeRole::Canceler))
AwaitingCancelResolution::<T>::mutate(l1, |v| {
v.remove(&(canceler, cancel_request_id, DisputeRole::Canceler))
});

// slash is after adding rights, since slash can reduce stake below required level and remove all rights
Expand Down Expand Up @@ -1411,9 +1411,9 @@ impl<T: Config> Pallet<T> {
}

read_rights.saturating_accrue(
AwaitingCancelResolution::<T>::get((chain, sequencer))
AwaitingCancelResolution::<T>::get(chain)
.iter()
.filter(|(_, role)| *role == DisputeRole::Submitter)
.filter(|(acc, _, role)| acc == sequencer && *role == DisputeRole::Submitter)
.count() as u128,
);

Expand All @@ -1424,9 +1424,9 @@ impl<T: Config> Pallet<T> {
chain: ChainIdOf<T>,
sequencer: &AccountIdOf<T>,
) -> usize {
AwaitingCancelResolution::<T>::get((chain, sequencer))
AwaitingCancelResolution::<T>::get(chain)
.iter()
.filter(|(_, role)| *role == DisputeRole::Canceler)
.filter(|(acc, _, role)| acc == sequencer && *role == DisputeRole::Canceler)
.count()
}

Expand Down Expand Up @@ -1674,8 +1674,13 @@ impl<T: Config> RolldownProviderTrait<ChainIdOf<T>, AccountIdOf<T>> for Pallet<T
},
);

for (_, rights) in sequencer_set.iter_mut().filter(|(s, _)| *s != sequencer) {
rights.cancel_rights.saturating_accrue(T::RightsMultiplier::get())
let sequencer_count = (sequencer_set.len() as u128).saturating_sub(1u128);

for (s, rights) in sequencer_set.iter_mut().filter(|(s, _)| *s != sequencer) {
rights.cancel_rights =
T::RightsMultiplier::get().saturating_mul(sequencer_count).saturating_sub(
Pallet::<T>::count_of_cancel_rights_under_dispute(chain, s) as u128,
)
}
});
}
Expand Down
134 changes: 93 additions & 41 deletions pallets/rolldown/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1306,18 +1306,19 @@ fn test_sequencer_unstaking() {
assert_ok!(Rolldown::sequencer_unstaking(consts::CHAIN, &ALICE));
assert_eq!(LastUpdateBySequencer::<Test>::get((consts::CHAIN, ALICE)), 0);

AwaitingCancelResolution::<Test>::insert(
(consts::CHAIN, ALICE),
BTreeSet::from([(0, DisputeRole::Canceler)]),
);
AwaitingCancelResolution::<Test>::mutate(consts::CHAIN, |v| {
v.insert((ALICE, 0u128, DisputeRole::Canceler))
});
assert_err!(
Rolldown::sequencer_unstaking(consts::CHAIN, &ALICE),
Error::<Test>::SequencerAwaitingCancelResolution
);

AwaitingCancelResolution::<Test>::remove((consts::CHAIN, ALICE));
AwaitingCancelResolution::<Test>::mutate(consts::CHAIN, |v| {
v.remove(&(ALICE, 0u128, DisputeRole::Canceler))
});
assert_ok!(Rolldown::sequencer_unstaking(consts::CHAIN, &ALICE));
assert_eq!(AwaitingCancelResolution::<Test>::get((consts::CHAIN, ALICE)), BTreeSet::new());
assert_eq!(AwaitingCancelResolution::<Test>::get(consts::CHAIN), BTreeSet::new());
});
}

Expand Down Expand Up @@ -1380,12 +1381,11 @@ fn test_cancel_updates_awaiting_cancel_resolution() {

// Assert
assert_eq!(
AwaitingCancelResolution::<Test>::get((consts::CHAIN, ALICE)),
BTreeSet::from([(1, DisputeRole::Submitter)])
);
assert_eq!(
AwaitingCancelResolution::<Test>::get((consts::CHAIN, BOB)),
BTreeSet::from([(1, DisputeRole::Canceler)])
AwaitingCancelResolution::<Test>::get(consts::CHAIN),
BTreeSet::from([
(ALICE, 1, DisputeRole::Submitter),
(BOB, 1, DisputeRole::Canceler),
])
);
});
}
Expand Down Expand Up @@ -1427,14 +1427,16 @@ fn test_cancel_resolution_updates_awaiting_cancel_resolution() {
forward_to_block::<Test>(12);
Rolldown::update_l2_from_l1_unsafe(RuntimeOrigin::signed(BOB), cancel_resolution)
.unwrap();
assert_eq!(
AwaitingCancelResolution::<Test>::get((consts::CHAIN, ALICE)),
BTreeSet::from([(1, DisputeRole::Submitter)])
);
assert_eq!(
AwaitingCancelResolution::<Test>::get((consts::CHAIN, BOB)),
BTreeSet::from([(1, DisputeRole::Canceler)])
);
assert!(AwaitingCancelResolution::<Test>::get(consts::CHAIN).contains(&(
ALICE,
1,
DisputeRole::Submitter
)));
assert!(AwaitingCancelResolution::<Test>::get(consts::CHAIN).contains(&(
BOB,
1,
DisputeRole::Canceler
)));
forward_to_block::<Test>(16);

let slash_sequencer_mock = MockSequencerStakingProviderApi::slash_sequencer_context();
Expand All @@ -1448,14 +1450,8 @@ fn test_cancel_resolution_updates_awaiting_cancel_resolution() {
forward_to_block::<Test>(17);
forward_to_next_block::<Test>();

assert_eq!(
AwaitingCancelResolution::<Test>::get((consts::CHAIN, ALICE)),
BTreeSet::new()
);
assert_eq!(
AwaitingCancelResolution::<Test>::get((consts::CHAIN, BOB)),
BTreeSet::new()
);
assert_eq!(AwaitingCancelResolution::<Test>::get(consts::CHAIN), BTreeSet::new());
assert_eq!(AwaitingCancelResolution::<Test>::get(consts::CHAIN), BTreeSet::new());
})
}

Expand Down Expand Up @@ -1638,7 +1634,7 @@ fn consider_awaiting_cancel_resolutions_and_cancel_disputes_when_assigning_initi
let slash_sequencer_mock = MockSequencerStakingProviderApi::slash_sequencer_context();
slash_sequencer_mock
.expect()
.withf(|chain, a, b| *chain == consts::CHAIN && *a == ALICE && b.cloned() == None)
.withf(|chain, a, b| *chain == consts::CHAIN && *a == ALICE && b.is_none())
.times(2)
.return_const(Ok(().into()));

Expand All @@ -1650,25 +1646,17 @@ fn consider_awaiting_cancel_resolutions_and_cancel_disputes_when_assigning_initi
forward_to_block::<Test>(10);
Rolldown::update_l2_from_l1_unsafe(RuntimeOrigin::signed(BOB), honest_update.clone())
.unwrap();
Rolldown::cancel_requests_from_l1(
RuntimeOrigin::signed(ALICE),
consts::CHAIN,
15u128.into(),
)
.unwrap();
Rolldown::cancel_requests_from_l1(RuntimeOrigin::signed(ALICE), consts::CHAIN, 15u128)
.unwrap();

forward_to_block::<Test>(11);
Rolldown::update_l2_from_l1_unsafe(
RuntimeOrigin::signed(CHARLIE),
honest_update.clone(),
)
.unwrap();
Rolldown::cancel_requests_from_l1(
RuntimeOrigin::signed(ALICE),
consts::CHAIN,
16u128.into(),
)
.unwrap();
Rolldown::cancel_requests_from_l1(RuntimeOrigin::signed(ALICE), consts::CHAIN, 16u128)
.unwrap();

// lets pretned that alice misbehaved and got slashed, as a result her stake dropped below
// active sequencer threshold and she got immadietely removed from sequencers set
Expand Down Expand Up @@ -3194,3 +3182,67 @@ fn test_ferry_deposit_that_fails() {
});
})
}

#[test]
#[serial]
fn test_reproduce_bug_with_sequencer_being_able_to_get_more_cancel_rights_than_he_should() {
ExtBuilder::new()
.issue(ETH_RECIPIENT_ACCOUNT_MGX, ETH_TOKEN_ADDRESS_MGX, MILLION)
.execute_with_default_mocks(|| {
// Arrange
let slash_sequencer_mock = MockSequencerStakingProviderApi::slash_sequencer_context();
slash_sequencer_mock.expect().times(1).return_const(Ok(().into()));

let honest_update = L1UpdateBuilder::default()
.with_requests(vec![L1UpdateRequest::Deposit(Default::default())])
.build();
forward_to_block::<Test>(10);
Rolldown::update_l2_from_l1_unsafe(RuntimeOrigin::signed(ALICE), honest_update.clone())
.unwrap();

// accidently canceling honest update
forward_to_block::<Test>(11);
Rolldown::cancel_requests_from_l1(RuntimeOrigin::signed(BOB), consts::CHAIN, 15u128)
.unwrap();

forward_to_block::<Test>(15);
let cancel_resolution = L1UpdateBuilder::default()
.with_requests(vec![
// L1UpdateRequest::Deposit(Default::default()),
L1UpdateRequest::CancelResolution(messages::CancelResolution {
requestId: Default::default(),
l2RequestId: 1u128,
cancelJustified: false,
timeStamp: sp_core::U256::from(1),
}),
L1UpdateRequest::Deposit(Default::default()),
])
.build();

Rolldown::handle_sequencer_deactivations(consts::CHAIN, vec![ALICE, BOB, CHARLIE]);
Rolldown::new_sequencer_active(consts::CHAIN, &BOB);
Rolldown::new_sequencer_active(consts::CHAIN, &CHARLIE);

assert_eq!(
*SequencersRights::<Test>::get(consts::CHAIN).get(&BOB).unwrap(),
SequencerRights { read_rights: 1u128, cancel_rights: 0u128 }
);
assert_eq!(
*SequencersRights::<Test>::get(consts::CHAIN).get(&CHARLIE).unwrap(),
SequencerRights { read_rights: 1u128, cancel_rights: 1u128 }
);

Rolldown::update_l2_from_l1_unsafe(RuntimeOrigin::signed(CHARLIE), cancel_resolution)
.unwrap();

forward_to_block::<Test>(21);
assert_eq!(
*SequencersRights::<Test>::get(consts::CHAIN).get(&BOB).unwrap(),
SequencerRights { read_rights: 1u128, cancel_rights: 1u128 }
);
assert_eq!(
*SequencersRights::<Test>::get(consts::CHAIN).get(&CHARLIE).unwrap(),
SequencerRights { read_rights: 1u128, cancel_rights: 1u128 }
);
});
}

0 comments on commit 5b4d271

Please sign in to comment.