Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refund leases that have not started. #206

Merged
merged 11 commits into from
Mar 10, 2024
Merged

Refund leases that have not started. #206

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2cd4366
Also migrate leases that have not started.
Feb 29, 2024
69d1b23
Revert "Also migrate leases that have not started."
Feb 29, 2024
fe68b49
Clear all leases for unsupported.
Feb 29, 2024
099e179
Return value still needed.
Feb 29, 2024
f6cb253
Add changelog entry.
Mar 4, 2024
08d6d99
Merge branch 'main' into rk-migrate-lazy-leases
eskimor Mar 5, 2024
1b491de
Update CHANGELOG.md
eskimor Mar 5, 2024
b951ddf
Merge branch 'main' into rk-migrate-lazy-leases
eskimor Mar 5, 2024
619009a
Merge branch 'main' into rk-migrate-lazy-leases
eskimor Mar 6, 2024
dcee4ba
Add logging target.
Mar 8, 2024
4e1ebb1
Merge branch 'main' into rk-migrate-lazy-leases
bkchr Mar 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions relay/kusama/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1725,9 +1725,18 @@ pub mod migrations {
if lease.is_empty() {
return None
}
// Lease not yet started, ignore:
// Lease not yet started/or having holes, refund (coretime can't handle this):
if lease.iter().any(Option::is_none) {
return None
if let Err(err) = slots::Pallet::<Runtime>::clear_all_leases(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they currently have an active lease which ends in say three months, then also won an auction which gives them a lease starting in three months, then this will kick them off their current lease as well as the one that has not yet started. Surely the intended functionality is that they are refunded for only the lease that has not yet started.
Or can they only swap at the point their current lease ends, i.e. the second lease that has not started is always under a separate para_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your particular scenario there would be no Nones .. .you have a lease for the next three months and one an auction for the ones afterwards ... all good.

In general, what happens here is you either get your lease to coretime or not. If not, you get all your leases refunded - even an ongoing one as in fact you just lost it. So this looks correct to me.

Copy link
Contributor

@seadanda seadanda Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, bad example. Slightly different to get a None in there but keeping my question the same:
If they currently have an active lease which ends three months, then also won an auction which gives them a lease starting in four and half months, they have a None in there, they'll be refunded their future lease but also booted off their current lease three months earlier than they expected.

Maybe that's a bit contrived, but I don't think a current lease should not be migrated, even if it is refunded, just because they have a gap before the lease they have won starts.

Maybe this edge case doesn't exist in reality, in which case all good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in that case they already have a hole and thus downtime. I would assume if that edge case even exists that some more downtime won't kill them.

Not saying that this is ideal, but:

(1) I would really assume that this is an edge case that will very likely not even exist. And if so, a solution might be similar to whatever we are going to do about swaps.
(2) We could trim the leases to the first None, but then we would have another problem: The leases after the Nones would keep their token locked, despite the fact that we would have ended their lease on coretime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this the other day, that sounds sensible to me

frame_system::RawOrigin::Root.into(),
para,
) {
log::error!(
"Clearing lease for para: {:?} failed, with error: {:?}",
para,
err
);
};
}
let (index, _) =
<slots::Pallet<Runtime> as Leaser<BlockNumber>>::lease_period_index(now)?;
Expand Down
Loading