Skip to content

fix(sdk-api): replace naive keychain batching with FFD bin packing#8390

Merged
vibhavgo merged 1 commit intomasterfrom
WP-8343
Apr 1, 2026
Merged

fix(sdk-api): replace naive keychain batching with FFD bin packing#8390
vibhavgo merged 1 commit intomasterfrom
WP-8343

Conversation

@vibhavgo
Copy link
Copy Markdown
Contributor

@vibhavgo vibhavgo commented Apr 1, 2026

Description

The changePassword flow re-encrypts all of a user's keychains under the new password and sends them to the server in a single (or batched) payload. The original batching logic split keychains into N equal groups by count, where N came from the server's noOfBatches response. This breaks completely for mixed size keychains payloads.

Root cause: Count-based equal-split cannot account for this because the server does not know individual keychain sizes — it only sees the total payload.

Solution: FFD bin packing

This PR replaces count-based batching with First Fit Decreasing (FFD) bin packing:

  1. Server returns maxBatchSizeKB (e.g. 900 KB) instead of noOfBatches
  2. Client sorts all keychains by serialised byte size, largest first
  3. Each keychain is placed into the first open bin with enough remaining capacity
  4. No bin ever exceeds maxBatchSizeKB — guaranteed by construction
  5. Any keychain that individually exceeds the limit throws immediately with a clear error

This correctly handles mixed payloads regardless of how large and small keys are interleaved.

Changes

  • changePassword now passes batchingFlowCheck.maxBatchSizeKB (instead of noOfBatches) to the batching method
  • New private packKeychainsFFD method implements the FFD algorithm; V1 and V2 keychains are kept in separate maps per bin to match the existing PUT /user/keychains API contract
  • processKeychainPasswordUpdatesInBatches signature changed from noOfBatches to maxBatchSizeKB; it now delegates bin construction to packKeychainsFFD

Issue Number

WP-8343

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Unit tests added in modules/sdk-api/test/unit/bitgoAPI.ts covering:

  • packKeychainsFFD: empty inputs, single-bin fit, multi-bin split, oversized item throws, correct V1/V2 separation, FFD ordering (large item packed first)
  • processKeychainPasswordUpdatesInBatches: single batch, two-bin split, retry-then-succeed, retries exhausted on HTTP errors, retries exhausted on failed-keychains response
  • changePassword batching flow: batching enabled (PUT + keychain-free POST), batching disabled (legacy POST with keychains), batching check failure (falls back to legacy)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My commits follow Conventional Commits and BREAKING CHANGES are described
  • The ticket was included in the commit message as a reference
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes

Ticket: WP-8343

- Replace count-based equal-split batching with FFD bin packing
- Bound each batch by maxBatchSizeKB from server response
- Add packKeychainsFFD private method with per-keychain size guard
- Add packKeychainsFFD, processKeychainPasswordUpdatesInBatches and changePassword tests

Ticket: WP-8343
@vibhavgo vibhavgo merged commit 5b82c98 into master Apr 1, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants