Skip to content

Fix sends racing with disconnect and handshake cancellation - #1374

Merged
NicolasDorier merged 1 commit into
masterfrom
fix/send-disconnect-race
Nov 6, 2026
Merged

NicolasDorier merged 1 commit into
masterfrom
fix/send-disconnect-race

Conversation

@NicolasDorier

@NicolasDorier NicolasDorier commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Related to btcpayserver/NBXplorer#578 and btcpayserver/NBXplorer#579.

SendMessageAsync checks IsConnected before invoking the outgoing filters and enqueueing the message. If the sender stops in between, it fails the messages it can see and replaces the queue. A late enqueue can land in either the already-drained queue or its replacement, leaving the returned task pending forever.

VersionHandshakeAsync awaits its sends without observing its cancellation token, so this race can also leave a handshake stuck even after cancellation.

Changes

  • Keep one send queue for the lifetime of the connection and call CompleteAdding before draining it during sender shutdown.
  • Fail the in-flight message and drain queued messages. A racing enqueue is either accepted before completion and drained, or rejected and its task faulted with the existing disconnected-peer exception.
  • Observe the handshake cancellation token while awaiting all four handshake sends, using the existing cross-target WithCancellation helper. Cancellation stops waiting; it does not retract an already accepted send.

This fixes NBitcoin's lost-send race; NBXplorer still needs its separate fix for an old node's disconnect handler disposing a replacement connection.

Validation

The regression tests used for validation were removed from the final diff at the maintainer's request. Only the production fix is included.

  • Confirmed the initial three regression cases (direct send, handshake disconnect, and cancellation of the first handshake send) time out against the original implementation.
  • Before removing the tests, passed all 15 selected tests on .NET 10: six regression cases and nine existing protocol unit cases.
  • Before removing the tests, passed all six regression cases with TargetFrameworkOverride=netstandard2.0, exercising the legacy cancellation and receive paths on the .NET 10 test runner.
  • Built NBitcoin successfully for all four targets: net10.0, net472, netstandard2.1, and netstandard2.0.
  • Did not run the full test suite or execute tests on the .NET Framework runtime.

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.

1 participant