Skip to content

Long poll TSS sessions - #4243

Open
kajoseph wants to merge 16 commits into
bitpay:masterfrom
kajoseph:longPollTssSessions
Open

Long poll TSS sessions#4243
kajoseph wants to merge 16 commits into
bitpay:masterfrom
kajoseph:longPollTssSessions

Conversation

@kajoseph

@kajoseph kajoseph commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Description

Add a long poll for the TSS session get endpoints to reduce both latency and the number of API calls

Changelog

  • Add long poll logic to the TSS get endpoints
  • Reduce noise for TSS signing sessions

Testing Notes


Checklist

  • I have read CONTRIBUTING.md and verified that this PR follows the guidelines and requirements outlined in it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Long-poll listeners can leak or miss updates, storage failures can become unhandled rejections, and CLI retries can loop without backoff.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds broker-driven long polling to TSS retrieval endpoints to reduce polling latency and API traffic.

Changes:

  • Adds TSS session notifications, long-poll waiting, and heartbeat responses.
  • Extends broker typing/testing and updates TSS integration tests.
  • Adjusts CLI retry handling and fixes ancillary documentation/error naming.
File summaries
File Description
packages/bitcore-wallet-service/test/messagebroker.test.ts Adds broker lifecycle tests.
packages/bitcore-wallet-service/test/integration/server.test.ts Uses configured TSS version.
packages/bitcore-wallet-service/src/lib/tss.ts Implements event-driven TSS waits.
packages/bitcore-wallet-service/src/lib/storage.ts Refines type-only imports.
packages/bitcore-wallet-service/src/lib/server.ts Exposes broker and updates notifications.
packages/bitcore-wallet-service/src/lib/routes/tss.ts Adds long-poll HTTP responses.
packages/bitcore-wallet-service/src/lib/routes/helpers/error.ts Ends streamed responses on errors.
packages/bitcore-wallet-service/src/lib/model/notification.ts Supports string notification IDs.
packages/bitcore-wallet-service/src/lib/messagebroker.ts Types messages and adds unsubscribe.
packages/bitcore-wallet-service/README.md Updates installation guidance.
packages/bitcore-wallet-client/test/tss.test.ts Shortens waits in TSS tests.
packages/bitcore-wallet-client/src/lib/payproV2.ts Corrects an error identifier.
packages/bitcore-wallet-client/src/lib/errors/spec.ts Renames the transaction error.
packages/bitcore-cli/src/tss.ts Retries transient signing failures.
Review details

Suppressed comments (3)

packages/bitcore-wallet-service/src/lib/tss.ts:558

  • This duplicated signing path has the same listener leak as the helper above: timeout or an incomplete update settles the request without unsubscribing handler. Each poll then leaves another broker listener and future signing messages trigger redundant storage fetches. Remove the handler in finally on every exit.
      session = await Promise.race([
        new Promise<TssSigGenModel>(r => events.once('session', r)),
        new Promise<TssSigGenModel>(r => timer = setTimeout(() => r(session), maxWaitTime))
      ]);
      clearTimeout(timer);

packages/bitcore-wallet-service/src/lib/tss.ts:545

  • A rejection from this async storage fetch is not handled by EventEmitter; it becomes an unhandled promise rejection and never rejects the pending request. Propagate the failure through the long-poll promise and clean up the broker listener instead.
        const _session = await storage.fetchTssSigSession({ id: session.id });

packages/bitcore-wallet-service/src/lib/tss.ts:552

  • The signing endpoint has the same lost-wakeup window: the route passes a previously fetched incomplete session, and a completing message can be stored and published before this listener is attached. With no subsequent event, the request waits until timeout and returns stale data. Register the listener and then immediately re-fetch/check the session before beginning the timed wait.
      messageBroker.onMessage(handler);
  • Files reviewed: 14/14 changed files
  • Comments generated: 5
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/bitcore-cli/src/tss.ts
Comment thread packages/bitcore-wallet-service/src/lib/tss.ts Outdated
Comment thread packages/bitcore-wallet-service/src/lib/tss.ts Outdated
Comment thread packages/bitcore-wallet-service/src/lib/tss.ts Outdated
Comment thread packages/bitcore-wallet-service/src/lib/tss.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The retry loop can restart a successfully created signing session, and the long-poll implementation has stale-state and resource-retention issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

packages/bitcore-cli/src/tss.ts:79

  • After one connection failure sets isTransientError to true, a later successful start() does not reset it, so the do/while immediately starts the same session again. That retry can hit TSS_ROUND_ALREADY_DONE and incorrectly report an interrupted session; exit the loop once the session has been stored.
    packages/bitcore-wallet-client/src/lib/errors/spec.ts:150
  • Renaming this generated public error removes Errors.NO_TRASACTION and changes the error's exported name for existing consumers. Keep a deprecated compatibility alias while introducing NO_TRANSACTION, then remove it only in a documented breaking release.
  • Files reviewed: 14/14 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread packages/bitcore-wallet-service/src/lib/tss.ts
Comment thread packages/bitcore-wallet-service/src/lib/tss.ts Outdated
Comment thread packages/bitcore-wallet-service/src/lib/tss.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Broker lifecycle, compressed heartbeats, unbounded retries, and public API compatibility need correction.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

packages/bitcore-wallet-service/src/lib/routes/tss.ts:137

  • The app installs compression() globally (routes/setup.ts:13), so this one-byte write can remain buffered in the compression stream instead of reaching the client. That defeats the heartbeat for gzip-capable clients and can let idle timeouts terminate the long poll. Flush the compression stream after each heartbeat (or disable compression for this response).
        interval = setInterval(() => res.write('\n'), 1000);

packages/bitcore-cli/src/tss.ts:93

  • A connection refusal can make start() reject immediately, and this branch retries forever with no delay. While BWS is unavailable the CLI will repeatedly rebuild the signing state and issue requests in a tight loop, consuming CPU and hammering the endpoint. Add a bounded/exponential backoff (and preferably a retry or cancellation bound) before the next iteration.
        } else if (err instanceof Errors.CONNECTION_ERROR) {
          isTransientError = true;
          connErrs = connResilience(connErrs, err);
  • Files reviewed: 17/17 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread packages/bitcore-wallet-client/src/lib/errors/spec.ts
Comment thread packages/bitcore-wallet-service/src/lib/tss.ts
Comment thread packages/bitcore-wallet-service/src/lib/routes/tss.ts Outdated
// Flush ensures the heartbeat is sent immediately to keep the connection alive.
res.writeHead(200, { 'Content-Type': 'application/json' });
interval = setInterval(() => { res.write('\n'); res.flush(); }, 1000);
req.on('close', () => clearInterval(interval));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to clean up the heartbeat when the client disconnects?
For Node 22, req's close is emitted when the request completes. res.close is emitted when the response is completed or its underlying connection was terminated prematurely.

Consider res.once('close',...).

Same thing on '/v1/tss/sign/:id/:round'.

https://nodejs.org/docs/latest-v22.x/api/http.html#event-close_3
https://nodejs.org/docs/latest-v22.x/api/http.html#event-close_2

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.

3 participants