synchronizer: fix race during concurrent status updates - #10870
Merged
Conversation
f321x
marked this pull request as draft
August 18, 2026 15:56
f321x
force-pushed
the
fix_synchronizer_race
branch
from
August 19, 2026 08:21
ab06ea1 to
917930f
Compare
f321x
marked this pull request as ready for review
August 19, 2026 08:22
f321x
marked this pull request as draft
August 19, 2026 12:08
This ensures the order of responses on the subscription queue is either duplicated or increasing in recency but not decreasing. Before it could happen that `handle_request` already receives a newer notification and puts it on the subscription queue, and then `send_response` returning the initial response and also putting it on the queue. Causing the less recent `send_response` result to pop from the queue after the newer one. The following synchronizer commit depends on the messages arriving in order, otherwise it might drop a newer status in favor of an older one that was put on the queue after the newer one.
We would incorrectly disconnect from a server when this race occurs:
1. We receive S1 (e.g. tx in mempool)
2. We request history H1 for S1 -> req1
3. We receive S2 (e.g. tx got mined) concurrently
4. We request history H2 for S2 -> req2
5. We receive H2 for S2 (before req1 got satisfied)
6. req1 receives H2 for current status (S2) from the server as well
7. _on_address_status schedules disconnect because H2 mismatches S1
8. S2 got already handled before, we never cancel the disconnect
```
DEBUG electrum.tests.toyserver.toyserver.ToyServerSession:toyserver.py:465 127.0.0.1:42170 disconnected
ERROR electrum.interface.Interface.[127.0.0.1:46059]:util.py:1231 Exception in run: SynchronizerFailure('timeout reached waiting for addr bcrt1qyt2w5jw5ktkr0ta96zuque93l4p86u3k2tj36cxcvs6gzl6wyj5q04p7y3: history still stale')
Traceback (most recent call last):
File "/home/runner/work/electrum/electrum/electrum/util.py", line 1225, in wrapper
return await func(*args, **kwargs)
File "/home/runner/work/electrum/electrum/electrum/interface.py", line 756, in wrapper_func
return await func(self, *args, **kwargs)
File "/home/runner/work/electrum/electrum/electrum/interface.py", line 782, in run
await self.open_session(ssl_context=ssl_context)
File "/home/runner/work/electrum/electrum/electrum/interface.py", line 1072, in open_session
async with self.taskgroup as group:
File "/opt/hostedtoolcache/Python/3.10.20/x64/lib/python3.10/site-packages/aiorpcx/curio.py", line 304, in __aexit__
await self.join()
File "/home/runner/work/electrum/electrum/electrum/util.py", line 1430, in join
task.result()
File "/home/runner/work/electrum/electrum/electrum/util.py", line 1564, in run_tasks_wrapper
await self._run_tasks(taskgroup=taskgroup)
File "/home/runner/work/electrum/electrum/electrum/synchronizer.py", line 79, in _run_tasks
async with taskgroup as group:
File "/opt/hostedtoolcache/Python/3.10.20/x64/lib/python3.10/site-packages/aiorpcx/curio.py", line 304, in __aexit__
await self.join()
File "/home/runner/work/electrum/electrum/electrum/util.py", line 1430, in join
task.result()
File "/home/runner/work/electrum/electrum/electrum/synchronizer.py", line 216, in disconnect_if_still_stale
raise SynchronizerFailure(f"timeout reached waiting for addr {addr}: history still stale")
electrum.synchronizer.SynchronizerFailure: timeout reached waiting for addr bcrt1qyt2w5jw5ktkr0ta96zuque93l4p86u3k2tj36cxcvs6gzl6wyj5q04p7y3: history still stale
```
If the status reverts to the one our stored history already has, _on_address_status returns early and the task disconnect stays active, so we drop a server that behaved correctly. Also cancel previous disconnect task before scheduling to prevent the following (rather unlikely) scenario: 1. We receive S1 -> req1 2. We receive S2 -> req2 3. req2 answers mismatching S2 -> disconnect task (_stale_histories[addr] = DT2) 4. We receive S1 again, req1 is still in flight, so (addr, S1) is still in requested_histories -> _on_address_status returns early and does not cancel DT2 5. req1 answers mismatching S1, S1 is the current status, so this is not obsolete -> we arm DT1 and assign _stale_histories[addr] = DT1, dropping DT2 6. A later status with matching history cancels _stale_histories[addr], which is DT1. DT2 is unreachable, fires, and disconnects a server that behaved correctly.
f321x
force-pushed
the
fix_synchronizer_race
branch
from
August 19, 2026 13:35
917930f to
8d81870
Compare
f321x
marked this pull request as ready for review
August 19, 2026 13:55
Member
|
I am wondering if this could be improved by changes at the protocol level. If the |
Member
|
The PR looks good to me. For the record:
|
Member
Author
Tagging spesmilo/electrum-protocol#16 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We would incorrectly disconnect from a server when this race occurs:
This causes the failing unittest in #10852: