[pranavjain97]

Adds opt-in upgrading of v1 user keys to v2 (argon2) encryption when a user signs. The upgrade runs in the background, after signing, and never blocks or fails the signing call.

Enabling it

• New BitGo constructor option upgradeEncryptionOnSign (default false), read through bitgo.isUpgradeEncryptionOnSignEnabled().
• New per-call upgradeEncryption?: boolean on signing options, which overrides the constructor setting. It is never sent to the server.

Where it runs

• wallet.signTransaction (multisig, TSS, OFC), wallet.signMessage, wallet.signTypedData
• pendingApproval.approve (TSS recreate and sign)
• Lightning payInvoice and withdrawOnchain (user auth key)

Safety

• The new encryption is decrypted again and compared with the original before it is written.
• The write includes the current encryptedPrv, so the server rejects it if the key changed in the meantime. That case is reported as skipped: keyChanged.
• Concurrent upgrades of the same key share one request.
• After a failure the key stays as it was, and the next sign tries again.

Issue Number

WCN-285

[linear-code]

WCN-285

[github-actions]

⚠️ Compatibility checks are failing on Node.js 27-nightly (experimental, non-blocking). This does not block merge, but it signals an incompatibility to fix before this release becomes supported.

View run

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[s84krish]

lgtm

[nayandas190]

@claude review

[github-actions]

Claude finished @nayandas190's task in 44s —— View job

---

Review of #10129: upgrade v1 user keys to argon2 on sign

I read the sdk-core, sdk-api and abstract-lightning source changes. I did not run the tests or read the test files closely. I couldn't fetch origin/master in this checkout, so I diffed against HEAD~7.

Overall: The design is sound and I found nothing blocking. The upgrade is opt-in. It verifies the new envelope with a decrypt round-trip before writing. It uses existingEncryptedPrv for optimistic concurrency, and upgradeEncryption never rejects. baseCoin.keychains() returns a per-coin singleton, so the in-flight dedup map is shared across calls on the same coin.

Worth considering

1. The upgrade starts before signing, not after. The PR description says it runs "after signing". In wallet.ts (signTransaction, signTypedData, signMessage, and the OFC path) and in pendingApproval.ts, startUserKeyEncryptionUpgrade fires right after getUserPrv and before signTransactionTss or baseCoin.signTransaction. In lightning.ts it fires before the payment or withdrawal request goes out.

   • If signing then fails (a validation error or a bad tx), the key has already been rewritten to v2.
   • That is harmless, since the passphrase was correct, but it does not match the documented behaviour.
   • Either move the calls to after a successful sign, or reword the description and JSDoc. I'd prefer moving them. A key rewrite triggered by a call that ultimately failed is surprising, and the current placement forces the const prv = ... split in each path.

2. Fire-and-forget in short-lived processes. In a CLI or a quickly exiting script, the process may end before the PUT completes. That is safe because the next sign retries, but opt-in users may never see the upgrade happen. The explicit wallet.upgradeUserKeyEncryption and lightning.upgradeUserAuthKeyEncryption methods cover this. The docs on the constructor option should mention it.

3. pendingApproval.ts relies on an assumption. The comment says getPrv decrypts the first keychain with an encryptedPrv, "which is this same user keychain". If getPrv's selection logic ever changes, this would upgrade a different key than the one decrypted. It is safe today. The roundTrip check in runEncryptionUpgrade would fail with a wrong passphrase anyway, so the worst case is a wasted attempt, not corruption. Calling getUserPrv with childUserKeychain directly would remove the coupling.

4. Dedup is only per process and per Keychains instance. Two processes, or two BitGo instances, can race. The server's 409 on existingEncryptedPrv handles this, and it surfaces as skipped: keyChanged. Correctness is fine. The keyChanged result can also appear when the same key is upgraded twice in a row from a stale keychain read. A short note in the enum JSDoc would avoid confusing UI consumers.

Minor

• modules/abstract-lightning/src/codecs/api/wallet.ts adds import type { IRequestTracer } from '@bitgo/sdk-core' to a codec file. It is type-only, so there is no runtime impact. Check that it fits the file's existing import conventions.
• upgradeEncryption returns Error for an unusable keychain or passphrase, such as a missing id. That is reasonable, but it is indistinguishable from a wrong-passphrase failure. This is fine for now.
• The Node 27-nightly compatibility failure is flagged as experimental and non-blocking. It looks unrelated to this change, but I didn't investigate it.

I'd go with item 1 as the one change before merge. The rest are optional.

[claude]

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🔴 modules/sdk-core/src/bitgo/keychain/upgradeEncryption.ts — With an older sdk-api or a custom BitGoBase, signing and Lightning payments with a passphrase now throw a TypeError. Base signs normally. shouldUpgradeEncryption calls bitgo.isUpgradeEncryptionOnSignEnabled() without checking it exists. It runs inside the signing call, before the upgrade, which is the only part that never throws. Fix: treat a missing method as false (typeof bitgo.isUpgradeEncryptionOnSignEnabled === 'function' && ...) so all 5 call sites keep signing. Sites: wallet.ts:655, pendingApproval.ts:493, lightning.ts:295, lightning.ts:425. [also at: modules/sdk-core/src/bitgo/wallet/wallet.ts:681 - Signing can now fail with a TypeError when the BitGo object lacks isUpgradeEncryptionOnSignEnabled. That breaks the promise that the upgrade never affects signing.; modules/sdk-core/src/bitgo/wallet/wallet.ts:2660 - A signing call can now fail with a TypeError when the BitGo object has no isUpgradeEncryptionOnSignEnabled method.]

  Why this was flagged

  Trigger: a BitGoBase object that does not define the new isUpgradeEncryptionOnSignEnabled method. Examples are an older @ bitgo/sdk-api BitGoAPI installed next to a newer @ bitgo/sdk-core, or a custom or mock BitGoBase. On the common path (walletPassphrase given, keychain has an encryptedPrv, no prv), Wallet.startUserKeyEncryptionUpgrade (wallet.ts:655) calls shouldUpgradeEncryption synchronously inside signTransaction, signTypedData and signMessage. LightningWallet.payInvoice and withdrawOnchain call it with no condition (lightning.ts:295, 425), and so does PendingApproval.recreateAndSignTSSTransaction (pendingApproval.ts:493). The call throws TypeError: bitgo.isUpgradeEncryptionOnSignEnabled is not a function, so the sign, payment or approval fails before it reaches the server. On the base branch these calls succeed. The PR says the upgrade never blocks or fails signing, but the guard sits outside the never-throw upgradeEncryption promise, so that promise does not protect this call.

  Verification: In modules/sdk-core/src/bitgo/keychain/upgradeEncryption.ts the function sits at about line 14. It is return upgradeEncryption ?? bitgo.isUpgradeEncryptionOnSignEnabled();. It does not check that the method exists, so it throws a TypeError whenever no per-call upgradeEncryption is given. On the base commit these calls sign normally.