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.

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

[pallet-broker] Force-unpool provisionally pooled regions before redispatching them #4081

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

seadanda
Copy link
Contributor

@seadanda seadanda commented Apr 11, 2024

This PR force removes regions from the pool before allowing them to be redispatched (through partition/interlace/assign) in the case that a region was pooled with Provisional finality.

This PR does not account for the case where a pooled region already entitles the benefactor to a contribution reward before the point of it being redispatched. However, claim_revenue should be called before trying to redispatch a region anyway.
Otherwise there would be ambiguity as to what should be done with the first part of a partitioned region. The first of the two new regions actually has the exact same region_id as the original region -- leave it pooled/withdraw from pool/claim contribution then withdraw?

@seadanda seadanda added the T2-pallets This PR/Issue is related to a particular pallet. label Apr 11, 2024
@seadanda seadanda self-assigned this Apr 11, 2024
@seadanda seadanda changed the title [pallet-broker] Force-unpool provisionally pooled regions before redeployment [pallet-broker] Force-unpool provisionally pooled regions before redispatching them Apr 11, 2024
@seadanda seadanda marked this pull request as ready for review April 16, 2024 16:31
@seadanda seadanda requested a review from a team as a code owner April 16, 2024 16:31
substrate/frame/broker/src/utility_impls.rs Outdated Show resolved Hide resolved
substrate/frame/broker/src/utility_impls.rs Outdated Show resolved Hide resolved
substrate/frame/broker/src/utility_impls.rs Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 17, 2024 09:33
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7198870

@@ -390,6 +390,14 @@ pub mod pallet {
/// The Region whose contribution is no longer exists.
region_id: RegionId,
},
/// A region has been force-removed from the pool. This is usually due to a provisionally
/// pooled region being redeployed.
RegionUnpooled {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the reason for the major bump, but since there is an event for a region being pooled we should also have one for it being unpooled.

Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Tests seem to fail?

// the current timeslice if we are already part-way through the region.
let size = region_id.mask.count_ones() as i32;
let unpooled_at = end_timeslice.max(region_id.begin);
InstaPoolIo::<T>::mutate(unpooled_at, |a| a.private.saturating_reduce(size));
Copy link
Member

Choose a reason for hiding this comment

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

Do we leave the WorkPlan as is on purpose?

substrate/frame/broker/src/utility_impls.rs Show resolved Hide resolved
@seadanda
Copy link
Contributor Author

Hmm yea I just updated from master today after a while of waiting for audit, and it seems that #3940 changed the mock for these tests

@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12029487702
Failed job name: test-linux-stable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: Audited
Development

Successfully merging this pull request may close these issues.

4 participants