Better error handling for undecryptable key requests

[corrideat]

Closes #64

---

[taoeffect]

/crush_fast

---

AI review started.

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5

Click to expand review

I'll review these changes by examining the full context of the code. Let me first look at the relevant sections of the OP_KEY_REQUEST handler.Let me check how respondToAllKeyRequests works and how pendingKeyshares is cleaned up:Let me also look at what the types say about the context structure:Now let me examine the type definition for SignedDataContext to understand the context structure better:Based on my thorough review of the changes, here are my findings:

---

Issue 1: No logging for decryption failures

• Addressed
• Dismissed

🟡 Medium importance, high confidence

Location: src/internals.ts:1532-1539

The issue description states:

"Additionally, the logging should be improved to include specific message details."

And:

"The proposed solution is to refactor the validation logic to accurately report decryption failures rather than signature errors."

However, the new code does not log anything when decryption fails for V2 requests. When context is undefined (which indicates decryption failure), the code silently continues without any error logging:

// Lines 1532-1539
if (Array.isArray(context) && context[0] !== originatingContractID) {
  logEvtError(
    message,
    'Ignoring OP_KEY_REQUEST because it is signed by the wrong contract',
    originatingContractID + ' !== ' + context[0]
  )
  return
}

The misleading error is correctly avoided, but there's no replacement logging for the decryption failure case. This could make debugging difficult when investigating why a key request wasn't processed.

Suggested improvement:

// Used as a proxy to tell whether a request could be decrypted
const context = v.replyWith.context

if (Array.isArray(context)) {
  if (context[0] !== originatingContractID) {
    logEvtError(
      message,
      'Ignoring OP_KEY_REQUEST because it is signed by the wrong contract',
      originatingContractID + ' !== ' + context[0]
    )
    return
  }
} else {
  // V1: Note that this branch applies to V1 requests as well, which will be removed
  // in a future version. V2 requests should always have a context if properly decrypted.
  logEvtError(
    message,
    'Ignoring OP_KEY_REQUEST because it could not be decrypted',
    'No context available for message at height ' + message.height()
  )
  // Continue to store for record-keeping, but don't attempt to respond
}

---

Issue 2: Missing V1-specific comment

• Addressed
• Dismissed

⚪️ Low importance, high confidence

Location: src/internals.ts:1529-1530

The issue summary states:

"Developers must explicitly comment on any logic specific to V1, as V1 support is planned for removal in the future."

The new comment explains that context is used as a proxy for decryption success, but doesn't clarify that this applies to both V1 and V2 formats:

// Used as a proxy to tell whether a request could be decrypted
const context = v.replyWith.context

While the logic correctly handles both versions, a clarifying comment would help future maintainers understand the V1 behavior before its removal.

Suggested improvement:

// Used as a proxy to tell whether a request could be decrypted.
// For V1: outer layer decryption failure results in undefined context
// For V2: inner layer decryption failure results in undefined context
// Note: V1 support is planned for removal
const context = v.replyWith.context

---

Summary

The core fix correctly addresses the misleading error message issue by changing the condition from:

// Old: data && (!Array.isArray(context) || context[0] !== originatingContractID)
// New: Array.isArray(context) && context[0] !== originatingContractID

This ensures the "wrong contract" error is only logged when context is actually an array (i.e., decryption succeeded). The use of context instead of data for the post-sync operation check (if (context) at line 1558) is also correct, as data is always truthy for V2 due to its unencrypted outer layer.

The 3-element pendingKeyshares format (stored when decryption fails) is documented in types.ts:300 and appears intentional for record-keeping while preventing response attempts. The existing processing functions (respondToAllKeyRequests at line 2471, respondToKeyRequest at line 2501, and OP_KEY_REQUEST_SEEN handler at line 1584) correctly skip these entries by checking for 4 or 7 elements.

---

Review generated using glm-5 via Z.AI. Comment /crush_fast to re-run.

[Copilot]

Pull request overview

This PR aims to improve error handling and logging around processing OP_KEY_REQUEST messages that cannot be decrypted (notably V2 cases), addressing incorrect “wrong contract” reporting and tightening when follow-up response work is scheduled.

Changes:

• Adjust OP_KEY_REQUEST handling to avoid treating undecryptable V2 requests as “wrong contract,” and add extra mismatch detail to the log when that condition occurs.
• Gate post-sync key-request responding based on the presence of request “context” rather than data.
• Bump package version to 1.4.2 (and update lockfile / dist outputs accordingly).

Reviewed changes

Copilot reviewed 2 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/internals.ts	Refines OP_KEY_REQUEST context checks and when respondToAllKeyRequests is scheduled.
package.json	Version bump to 1.4.2.
package-lock.json	Lockfile updated for version bump and transitive dependency resolution (includes flatted change).
dist/esm/internals.mjs	Built ESM output reflecting internals.ts changes.
dist/cjs/internals.cjs	Built CJS output reflecting internals.ts changes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[taoeffect]

👏