[vibhavgo]

Summary

Aligns the EdDSA MPCv2 offline signing rounds (EddsaMPCv2Round2/EddsaMPCv2Round3) with the existing keygen helpers:

• The BitGo GPG public key is validated against the known BitGo key table in the same envs that already enforce this for keygen.
• Round session state is now tracked so each encrypted session blob is processed at most once, sharing the existing express ConsumedShareRegistry mechanism with the ECDSA MuDelta/S shares.
• No public API, wire-format, or route-contract changes; existing integrations keep working.

Linear: WAL-2455

Test Plan

• EddsaMPCv2 unit suite in sdk-core
• Express offline-signer end-to-end flow
• eslint, tsc, and full monorepo build clean

[linear-code]

WAL-2455

[vibhavgo]

@claude review always

[github-actions]

Claude finished @vibhavgo's task in 26s —— View job

---

Review of PR #10127 (WAL-2455)

I read the diff only and ran no tests. Overall the approach is sound: it pins the BitGo GPG key the same way the keygen and other helpers already do, and it makes the session blobs single-use. I found no blocking issues, only the points below.

What looks right

• The pin check and consumeSingleUseEncryptedSession both run before any decryption or DSG state restore. That is the correct order for CWE-323: a failed call can't leave a usable nonce path.
• The pinning uses the same envRequiresBitgoPubGpgKeyConfig + isBitgoEddsaMpcv2PubKey pattern as the existing call sites (eddsaMPCv2.ts lines 145, 297, 795, 856 and 943). The assert messages match.
• The registry is module-level, which is needed because express builds a new EddsaMPCv2Utils per request. Digests are SHA-256 of the ciphertext, so no secret material sits in the map.
• The tests cover replay and prod-env pinning for both rounds. The express test replays the final round end to end.

Observations (non-blocking)

1. Consumption happens before validation.
   • Any later failure burns the session. Examples are a mistyped walletPassphrase, a bad txRequest, or a failed getSignableHexAndDerivationPath.
   • The signer must then restart from round 1. This fits the at-most-once goal, but the PR's tradeoffs section only mentions lost HTTP responses.
   • Add the wrong-passphrase case to the tradeoffs list, and ideally assert it in a test so the behavior is deliberate.
   • A cheap middle ground is to consume only after the pin check and the cheap txRequest parsing, and before decrypt. That still guards the nonce, but typos in txRequest no longer cost a session.
2. Age pruning only runs when the map exceeds 100k entries. pruneConsumed… returns early below the cap, so the 30-day age window isn't actually enforced for a low-traffic signer. That is harmless for correctness, since entries just stay longer, which is the safer direction. The "30-day age prune" wording in the PR description and the doc comment is slightly misleading, though. Either prune on every call (it is O(expired)) or reword the comment.
3. The break relies on insertion order matching chronological order. This holds with Date.now() except across a backwards clock adjustment. It is fine, but the comment could say so.
4. Tests that mutate shared module state.
   • The registry persists across the whole mocha process.
   • Fresh createOfflineRound1Share calls produce unique ciphertexts, so this is fine today.
   • Any future test that reuses a fixed encryptedRound*Session fixture across cases will fail with the replay error. A test-only reset hook, or a comment in the test helpers, would help.
5. Express test flakiness check. The test now relies on BitgoMpcGpgPubKeys.getBitgoMpcGpgPubKey('test', 'onprem', 'eddsaMpcv2') being accepted as isBitgoEddsaMpcv2PubKey for env test. This should be the case, since the PR says it passes. Confirm that CI covers it.

Follow-ups already acknowledged
The in-memory registry doesn't survive restarts (the express ConsumedShareRegistry consolidation), and the 100k cap can evict entries under a flood (WAL-2458). Both are fair to defer as written.

No required changes. Item 1 is worth documenting or deciding on before merge.

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

[vibhavgo]

@claude review

[github-actions]

Claude finished @vibhavgo's task in 30s —— View job

---

Review of PR #10127 (WAL-2455), commit fb27cfa

I read the diff for sdk-core and express and checked it against sdk-api/src/encrypt.ts and decryptV1.ts. I did not run the tests.

Verdict: No blocking issues. The earlier review findings are addressed: the digest is formatting-independent, the mark moved to just before restoreSession, and the durable express registry is wired in. The notes below are all minor.

What looks right

• Order of operations. In both rounds the pin check runs first, then the digest and replay check, then validation and decryption. The mark and restoreSession follow, and the mark and the has check sit in one synchronous block with no await between them. That closes the concurrent-replay race.
• Digest canonicalization. It matches what decrypt() consumes: v === 2 goes to GCM with the full iv, and anything else goes to v1 with the iv truncated to ccmNonceLength. I confirmed the truncation logic mirrors decryptV1.ts:54-81. Leaving out salt, iter, ks and adata is safe because changing any of them breaks the key or the authentication tag. A relay therefore cannot get a successful decrypt under a different digest.
• Express. The durable key reuses the sdk-core digest, and the registry key includes the share type, so round 2 and round 3 do not collide.

Observations (non-blocking)

1. v1 ts default can differ from the decrypt path.
   • computeEncryptedSessionDigest treats a missing ts as 0 tag bytes (eddsaMPCv2.ts, the tagBytes line).
   • decryptV1.ts derives the tag size from the envelope and may use a default such as 64 bits. I did not read that default.
   • If the two disagree, the nonce length can differ from decrypt's in the narrow case where the ciphertext length sits near a 2^16 boundary.
   • Real SJCL envelopes carry ts, so this is theoretical today. Reusing the same default as decryptV1.ts, or exporting one shared helper from sdk-api, would remove the "keep in sync" burden.
2. Malformed input returns a 500 from express. computeEncryptedSessionDigest throws a plain Error. markConsumedEddsaSession calls it, so an unparseable blob surfaces as a 500 rather than the 400 you use for a missing blob. Catching it and rethrowing ApiResponseError(…, 400) would be consistent.
3. Express marks before the pin check and any validation. Any caller of the unauthenticated route (WAL-2458) can burn a victim's session with a valid blob and a junk request. This is already acknowledged in the PR as a tradeoff. When WAL-2458 lands, it would be worth moving the express mark behind the same checks the sdk-core layer does first.
4. PR description vs code. The description says the digest covers iv/salt/hkdfSalt/ct. The code covers the decoded iv and ct only, and the doc comment explains why salt is left out. Updating the description would avoid confusion.
5. Test hygiene. The in-process map lives for the whole mocha run. That is fine while every test calls createOfflineRound1Share for a fresh blob. A comment in the test helpers saying so would help future fixtures.

I agree with deferring fingerprint-based key comparison, since it matches the keygen helpers. A ticket for it would be good.

[claude]

Still open from earlier reviews (2):

• Unresolved: 1 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[vibhavgo]

@claude review

[vibhavgo]

@claude review

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

[vibhavgo]

@claude review

[claude]

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

[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.}

[Charan-Ratlan]

@claude do a thorough review of this and give an approval rating

[Charan-Ratlan]

@claude review

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[Marzooqa]

Overall: Solid — the post-authentication closure pattern for durable consumption is the right shape (mirrors the GG18 MuDelta/S closures from #10126): junk is rejected with a 400 before touching the registry, transient failures don't burn the session, and the durable → in-memory → restoreSession ordering means a concurrent replay can't double-restore. Composes cleanly with the merged #10126 registry, and the digest's canonical-base64 / salt-omission / v1 iv-truncation handling is well-reasoned. Pin rejection and replay rejection paths are both covered by tests.

One security finding and one config question inline — neither changes the overall shape.

(Context: #10155 hoists this module into a shared encryptedSessionSingleUse.ts for the ECDSA MPCv2 rounds — the timestamp finding below would be natural to fix in either place, whichever lands first.)

[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]

No new blocking issues. 1 optional suggestion (a nit or a note on pre-existing code) was found and not posted. Nothing in this review needs a push before merging.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[Marzooqa]

Approving. Both findings from the earlier review were addressed correctly — the session-blob expiry (created stamp in the authenticated adata at all four mint sites, skew tolerance, age check before the registry is consulted) and the startup warning for unpinned environments. Re-review also confirmed the mark/restore ordering, the digest canonicalization against re-serialization, and the adata state-prefix discrimination all hold.

The cross-transcript scenario (a restored session blob driven with a different session's peer messages) has been verified by the author as rejected by the DSG state machine.

Three small non-blocking asks:

• Since that scenario was verified manually, consider landing the test itself — a restored round-1 session fed another session's round-1 messages, asserting rejection — so the property stays pinned across future sdk-lib-mpc upgrades, where a regression would otherwise fail silently.
• DSG_SESSION_MAX_AGE_MS could alias the exported OFFLINE_SIGNING_SHARE_MAX_AGE_MS rather than duplicating the value, making the expiry/retention invariant structural instead of comment-enforced.
• One README/release-note line: the blob lifetime assumes a single signer process with a consistent clock — a backward clock step >60s mid-session or clock spread across processes rejects in-flight sessions (fail-closed), sessions minted before deploy restart at rollout, and in-flight sessions span at most two blob lifetimes (round hops each get their own 30-day window).