- AddressSynchronizer no longer has its own state re up_to_date,
it defers to Synchronizer/Verifier instead
- Synchronizer is now tracking address sync states throughout their lifecycle:
and Synchronizer.is_up_to_date() checks all states
- Synchronizer.add_queue (internal) is removed as it was redundant
- should fix wallet.is_up_to_date flickering during sync due to race
related:
dc6c4814061c20a29a22
This reverts commit dc6c481406 as it introduced its own issue:
while add_address was running on one thread, synchronizer._reset could be running on another,
and by the time the "enqueue" coro would run, it would use a new add_queue and
addr would not be in requested_addrs anymore...
```
I/w | wallet.Standard_Wallet.[test_segwit_2] | starting taskgroup.
I | lnworker.LNWallet.[test_segwit_2] | starting taskgroup.
E/i | interface.[testnet.qtornado.com:51002] | Exception in run: KeyError('tb1q3wmgf8n5eettnj50pzgnfrrpdpjmwn37x7nzsc5780kk4je9v4hspym8mu')
Traceback (most recent call last):
File ".../electrum/electrum/util.py", line 1243, in wrapper
return await func(*args, **kwargs)
File ".../electrum/electrum/interface.py", line 506, in wrapper_func
return await func(self, *args, **kwargs)
File ".../electrum/electrum/interface.py", line 529, in run
await self.open_session(ssl_context)
File ".../electrum/electrum/interface.py", line 679, in open_session
async with self.taskgroup as group:
File ".../aiorpcX/aiorpcx/curio.py", line 304, in __aexit__
await self.join()
File ".../electrum/electrum/util.py", line 1339, in join
task.result()
File ".../electrum/electrum/synchronizer.py", line 80, in _run_tasks
async with taskgroup as group:
File ".../aiorpcX/aiorpcx/curio.py", line 304, in __aexit__
await self.join()
File ".../electrum/electrum/util.py", line 1339, in join
task.result()
File ".../electrum/electrum/synchronizer.py", line 127, in subscribe_to_address
self.requested_addrs.remove(addr)
KeyError: 'tb1q3wmgf8n5eettnj50pzgnfrrpdpjmwn37x7nzsc5780kk4je9v4hspym8mu'
```
AddressSynchronizer.add_address called synchronizer.add, which would only
schedule adding the addr to the Synchronizer in the next event loop iter.
If during that time, the synchronizer called adb.set_up_to_date(True),
the wallet would falsely believe and advertise itself as up_to_date
(as the wallet would see wallet.synchronize not creating new addresses,
and adb (via synchronizer) telling it is up_to_date).
Moments later, the synchronizer._add_address is finally executed and
up_to_date=False propagates out synchronizer->adb->wallet.
- separate AddressSynchronizer from Wallet and LNWatcher
- the AddressSynchronizer class is referred to as 'adb' (address database)
- Use callbacks to replace overloaded methods
aiorpcx 0.20 changed the behaviour/API of TaskGroups.
When used as a context manager, TaskGroups no longer propagate
exceptions raised by their tasks. Instead, the calling code has
to explicitly check the results of tasks and decide whether to re-raise
any exceptions.
This is a significant change, and so this commit introduces "OldTaskGroup",
which should behave as the TaskGroup class of old aiorpcx. All existing
usages of TaskGroup are replaced with OldTaskGroup.
closes https://github.com/spesmilo/electrum/issues/7446
follow-up to 4346d2fc76
It's not just about the Synchronizer, the Verifier should not starve other jobs either...
(previously I thought the Verifier is not too important as it only makes
requests if there are new txs; however with LNWatcher its progress is not persisted)
When receiving the history of an address, the client behaved unexpectedly
if either of two checks failed.
The client checked that the txids in the history are unique, and that the
history matches the previously announced status. If either failed, it
would just log a line and do nothing. Importantly, the synchronizer could
even consider itself is_up_to_date, i.e. the GUI could show the wallet is
synced.
This is now changed such that:
- if the txid uniqueness test fails, we simply disconnect
- if the history is not consistent with previously announced status,
we wait a bit, make sure is_up_to_date is False in the meantime,
and then potentially disconnect
See rationale for these in the comments.
related: https://github.com/spesmilo/electrum/issues/7058#issuecomment-783613084
Scenario (prior to change):
User opens wallet1 with 10k addresses, and then immediately opens wallet2
with 100 addresses.
wallet1 will synchronise first, fully, and only then will wallet2 start syncing.
Now, wallet1 and wallet2 will sync concurrently (and wallet2 will finish much
sooner as expected).
as discussed in issue #6697, users with large wallets or slow
connections may never see their initial request-missing-tx run complete,
if we always use the same sync order.
This commit shuffles the addresses being requested every time a new
request round happens, so that (if enough time passes and enough initial
state requests are attempted) we will always get the latest state for
each address, regardless of how quickly the connection times out on us
some basic sanity checks
Previously if the server sent back a malformed response, it could partially corrupt a wallet file.
(as sometimes the response would get persisted, and issues would only arise later when the values were used)
Before this, we were subscribing to our addresses in their bip32 order,
leaking this information to servers. While this leak seems mostly harmless,
it is trivial to fix.
Note that there is a slight distinction between
`not tx.is_complete()` and `isinstance(tx, PartialTransaction)`,
which is that technically you can have a PSBT that is already complete
but was not yet converted to a standard bitcoin tx.
Triggering needs two consecutive scripthash status changes
in very quick succession. Client gets notification from server,
but then response to "blockchain.scripthash.get_history" will already contain
the changed-again history that has a different status.
20190627T101547.902638Z | INFO | synchronizer.[default_wallet] | receiving history mwXtx49BCGAiy4tU1r7MBX5VVLWSdtasCL 1
20190627T101547.903262Z | INFO | synchronizer.[default_wallet] | error: status mismatch: mwXtx49BCGAiy4tU1r7MBX5VVLWSdtasCL
User has wallet file with history that includes some txid; corresponding
raw tx is not in the "transactions" dict in the file however.
When the synchronizer starts up, it requests this "missing" txn from
the server... but what if the server does not know about it?
Maybe it was reorged and is not in the new best chain,
and not even in mempool. This was not handled previously.
fix#5122
The synchronizer would sometimes not send 'wallet_updated' triggers
if it was fast enough to do all the work between two 0.1 sec ticks.
(is_up_to_date() would return True both before and after)
related: 41e088693d
If our guess of a txn getting confirmed at the same height in the new chain
as it was at in the old chain is incorrect, there is a race between the
verifier and the synchronizer. If the verifier wins, the exception will cause
us to disconnect.
