Skip to content

Xpan auto retry limit #10178

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

Jump to bottom
Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Xpan auto retry limit #10178

wants to merge 11 commits into from
+131 −14

Conversation

@xpanvictor
Copy link

@xpanvictor xpanvictor commented Oct 31, 2025

BREAKING CHANGE: runtime must define MaxAutoRenewalRetries

Fixes #9548
Problem: After auto-renewal fails due to some error, the system disables autorenewal entirely contrary to the documentation.
Solution: A more robust retry which is configurable in the pallet config. This is compile time setting currently.
Introduce T::MaxAutoRenewalRetries constant in Config trait track retry count in AutoRenewalRetries storage map, increment on each failed renewal attempt. When retries >= max, emit AutoRenewalFailed, remove auto-renew record, and reset counter to 0. Add integration tests:

  • persistent retry when count < max
  • record removal when count >= max

Integration

Downstream projects using pallet-broker must define MaxAutoRenewalRetries config. They just need to pass a retries limit. Can be set to 0 for no retry. This is similar to current logic.

Review Notes

Changes:

  1. Added MaxAutoRenewalRetries to config trait. Default type is u8.
  2. Added a storage for renewal retries record. This increments for failed retries. It resets on success or when renewal is cancelled.
  3. Existing code needs to add the new configuration parameter.

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T required)
    • External contributors: Use /cmd label <label-name> to add labels
    • Maintainers can also add labels manually
  • [ ] I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@xpanvictor
feat(pallet-broker): add auto-renewal retry limit with counter reset
6f28bdc 
Introduce `T::MaxAutoRenewalRetries` constant in `Config` trait
track retry count in `AutoRenewalRetries` storage map, increment on each
failed renewal attempt. When retries >= max, emit `AutoRenewalFailed`, remove
auto-renew record, and reset counter to 0. Add integration tests:
- persistent retry when count < max
- record removal when count >= max

BREAKING CHANGE: runtime must define `MaxAutoRenewalRetries`
@xpanvictor
fix(test): auto_renewal_works expectation for failed renewal reassign…
948b1b9 
…ment
@xpanvictor
fix(pallet-broker): renew_cores reset on success as well
4f848ce
@xpanvictor xpanvictor requested a review from a team as a code owner October 31, 2025 16:36
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Oct 31, 2025

User @xpanvictor, please sign the CLA here.

@xpanvictor
Copy link
Author

xpanvictor commented Oct 31, 2025

/cmd prdoc

@xpanvictor xpanvictor mentioned this pull request Oct 31, 2025
Open
2 tasks
@franciscoaguirre franciscoaguirre added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 31, 2025
franciscoaguirre
franciscoaguirre reviewed Oct 31, 2025
View reviewed changes
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

The logic looks good. @seadanda ?

prdoc/pr_10178.prdoc Outdated
@@ -0,0 +1,35 @@
title: Xpan auto retry limit
doc:
- audience: Todo
Copy link
Contributor

@franciscoaguirre franciscoaguirre Oct 31, 2025

Choose a reason for hiding this comment

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

Suggested change
- audience: Todo
- audience: Runtime Dev

prdoc/pr_10178.prdoc
title: Xpan auto retry limit
doc:
- audience: Todo
description: |-
Copy link
Contributor

@franciscoaguirre franciscoaguirre Oct 31, 2025

Choose a reason for hiding this comment

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

You can probably just leave the description

@xpanvictor @franciscoaguirre
Update pr_10178.prdoc
112c8d8 
Co-authored-by: Francisco Aguirre <[email protected]>
franciscoaguirre
franciscoaguirre reviewed Oct 31, 2025
View reviewed changes
Copy link
Contributor

@franciscoaguirre franciscoaguirre left a comment

Choose a reason for hiding this comment

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

You should add this new config item to both coretime-westend and coretime-rococo

@xpanvictor
Copy link
Author

xpanvictor commented Oct 31, 2025

You should add this new config item to both coretime-westend and coretime-rococo

Ok. I'll do that.

@xpanvictor
Copy link
Author

xpanvictor commented Oct 31, 2025

You should add this new config item to both coretime-westend and coretime-rococo

Can you share a pointer to the crates? A bit large navigating the sdk.

@franciscoaguirre
Copy link
Contributor

franciscoaguirre commented Nov 1, 2025

They're both under https://github.com/paritytech/polkadot-sdk/tree/master/cumulus/parachains/runtimes/coretime

@xpanvictor
Copy link
Author

xpanvictor commented Nov 1, 2025

They're both under https://github.com/paritytech/polkadot-sdk/tree/master/cumulus/parachains/runtimes/coretime

I've added them @franciscoaguirre, is that fine? Also used default constu8<2> as I used in test.

@xpanvictor
Copy link
Author

xpanvictor commented Nov 2, 2025

https://docs.polkadot.com/develop/parachains/deployment/coretime-renewal/#:~:text=Even%20if%20an,the%20following%20sections.

I will make changes to the docs to reflect the new structure.

Also what should we make the MaxAutoRenewalRetries value be for coretime-rococo and coretime-westend?

@franciscoaguirre
Copy link
Contributor

franciscoaguirre commented Nov 4, 2025

2 looks like a good value

@acatangiu acatangiu requested a review from seadanda November 5, 2025 09:45
seadanda
seadanda reviewed Nov 5, 2025
View reviewed changes
Copy link
Contributor

@seadanda seadanda left a comment

Choose a reason for hiding this comment

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

Had a quick look, will take more time over the core approach and logic

@xpanvictor
Copy link
Author

xpanvictor commented Nov 6, 2025

@seadanda can you review, I've applied the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@franciscoaguirre franciscoaguirre franciscoaguirre left review comments

@seadanda seadanda seadanda left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

T2-pallets This PR/Issue is related to a particular pallet.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Auto-renewal gets disabled after failed payment attempts

3 participants

@xpanvictor @franciscoaguirre @seadanda