Skip to content

Conversation

@kunal-drall
Copy link

@kunal-drall kunal-drall commented Dec 24, 2025

Description

Fixes #10652

This PR fixes an off-by-one error in pallet-child-bounties where the add_child_bounty function allowed creating MaxActiveChildBountyCount + 1 child bounties instead of being capped at MaxActiveChildBountyCount.

The validation check used <= (less than or equal) instead of < (less than), allowing the count to exceed the limit by one when the current count equaled MaxActiveChildBountyCount. Since the count is incremented immediately after the validation check, this resulted in MaxActiveChildBountyCount + 1 active child bounties.

Integration

No integration changes required. This is a bug fix that ensures runtime configuration limits are properly enforced. Downstream projects will automatically benefit from the corrected limit enforcement without any code changes needed.

Impact:

  • Runtime configuration limits are now strictly enforced
  • No breaking changes - this is a bug fix that restores intended behavior
  • No migration required - existing child bounties are unaffected

Review Notes

Problem Analysis

The bug was located in substrate/frame/child-bounties/src/lib.rs at line 290. The validation check:

ensure!(
    ParentChildBounties::<T>::get(parent_bounty_id) <=
        T::MaxActiveChildBountyCount::get() as u32,
    Error::<T>::TooManyChildBounties,
);

When ParentChildBounties::<T>::get(parent_bounty_id) equals MaxActiveChildBountyCount, the check passes (because <=), and then the count is incremented on line 323, resulting in MaxActiveChildBountyCount + 1.

Solution

File: substrate/frame/child-bounties/src/lib.rs

Changed the comparison operator from <= to < and removed the unnecessary as u32 cast:

ensure!(
-    ParentChildBounties::<T>::get(parent_bounty_id) <=
-        T::MaxActiveChildBountyCount::get() as u32,
+    ParentChildBounties::<T>::get(parent_bounty_id) <
+        T::MaxActiveChildBountyCount::get(),
    Error::<T>::TooManyChildBounties,
);

Rationale:

  • Using < ensures that when the count equals MaxActiveChildBountyCount, the check fails before incrementing
  • Removing as u32 cast is a cleanup since MaxActiveChildBountyCount::get() already returns u32

Test Coverage

File: substrate/frame/child-bounties/src/tests.rs

  1. New test: max_active_child_bounty_count_is_strictly_enforced()

    • Creates exactly MaxActiveChildBountyCount (2 in test config) child bounties successfully
    • Verifies the count equals MaxActiveChildBountyCount
    • Attempts to create one more child bounty and verifies it fails with Error::TooManyChildBounties
    • Verifies the count remains at MaxActiveChildBountyCount (not incremented)
    • Tests recovery scenario: closes one child bounty and verifies a new one can be added
  2. Fixed existing test: accept_curator_handles_different_deposit_calculations

    • Added cleanup to close a child bounty before Case 3 to prevent hitting the limit
    • Ensures the test focuses on deposit calculations, not limit enforcement

Testing

  • ✅ New comprehensive test covers the edge case where count equals the limit
  • ✅ All existing tests pass
  • ✅ Test verifies error handling and recovery scenarios
  • ✅ CI will run full test suite automatically

Code Quality

  • No breaking changes
  • No API changes
  • Clean code improvement (removed unnecessary cast)
  • Well-documented with inline comments

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)
    • Use /cmd label T1-FRAME D0-easy I2-bug to add labels after PR creation
  • I have made corresponding changes to the documentation (PRDoc file included: prdoc/pr_10652.prdoc)
  • I have added tests that prove my fix is effective or that my feature works

Bot Commands

After creating the PR, use these commands in a comment:

Labeling:

/cmd label T1-FRAME D0-easy I2-bug

Verify PRDoc (optional):

/cmd prdoc

Format code (if needed):

/cmd fmt

- Change comparison operator from <= to < in add_child_bounty validation
- Remove unnecessary as u32 cast (MaxActiveChildBountyCount::get() already returns u32)
- Add comprehensive test max_active_child_bounty_count_is_strictly_enforced()
- Fix existing test to prevent hitting limit in accept_curator_handles_different_deposit_calculations

This ensures MaxActiveChildBountyCount is strictly enforced and prevents
creating MaxActiveChildBountyCount + 1 child bounties.
@kunal-drall kunal-drall requested a review from a team as a code owner December 24, 2025 09:56
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Dec 24, 2025

User @kunal-drall, please sign the CLA here.

@kunal-drall
Copy link
Author

/cmd label T1-FRAME D0-easy I2-bug

@paritytech-cmd-bot-polkadot-sdk paritytech-cmd-bot-polkadot-sdk bot added D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. I2-bug The node fails to follow expected behavior. T1-FRAME This PR/Issue is related to core FRAME, the framework. labels Dec 24, 2025
@bkchr bkchr enabled auto-merge December 27, 2025 20:53
@bkchr bkchr added T2-pallets This PR/Issue is related to a particular pallet. and removed I2-bug The node fails to follow expected behavior. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T1-FRAME This PR/Issue is related to core FRAME, the framework. labels Dec 27, 2025
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

None yet

Development

Successfully merging this pull request may close these issues.

Child Bounties Count Exceeds MaxActiveChildBountyCount Limit

3 participants