Repro steps:
- in qt gui, with network enabled, open wallet1
- open wizard, create wallet2 (restore from seed something that has mined history)
- close both wallets, stop electrum
- start electrum with "-o" offline flag, open wallet2
- observe all txs in history tab show up as "unconfirmed"
The cause is that "stored_height" only gets updated ~on new blocks.
So if you created a wallet and closed it soon, its db would not contain "stored_height."
There was a race incorrectly counting transactions with one
confirmations to the unconfirmed balance instead of the confirmed
balance.
This happened because the balance cache of AddressSynchronizer got
invalidated after `on_event_blockchain_updated` and then again after
`receive_history_callback`->`add_transaction`, however when calling
`AddressSynchronizer.get_balance()` before the tx got spv verified the
height would still be counted as 0 (unconfirmed), populating the balance
cache again with the unconfirmed balance.
I noticed this only on QML due to timing differences to Qt.
Invalidating the cache in `AddressSynchronizer.add_verified_tx()` after
the tx got verified causes the balance to get recalculated and shown
correctly.
- if fee estimates are high atm, some outputs are not worth to sweep
- however, fee estimates might be only-temporarily very high
- previously in such a case lnwatcher would just discard outputs as dust,
and mark the channel REDEEMED (and hence never watch it or try again)
- now, instead, if the outputs would not be dust if fee estimates were lower,
lnwatcher will keep watching the channel
- and if estimates go down, lnwatcher will sweep them then
- relatedly, previously txbatcher.is_dust() used allow_fallback_to_static_rates=True,
and it erroneously almost always fell back to the static rates (150 s/b) during
startup (race: lnwatcher was faster than the network managed to get estimates)
- now, instead, txbatcher.is_dust() does not fallback to static rates,
and the callers are supposed to handle NoDynamicFeeEstimates.
- I think this makes much more sense. The previous meaning of "is_dust"
with the fallback was weird. Now it means: "is dust at current feerates".
fixes https://github.com/spesmilo/electrum/issues/9980
get_utxos() is called pretty often, both spuriously,
and on focus change, on tab switch, &c.
It blocks as it iterates, functionally, /every/ address the wallet knows of.
On large wallets (like testnet
vpub5VfkVzoT7qgd5gUKjxgGE2oMJU4zKSktusfLx2NaQCTfSeeSY3S723qXKUZZaJzaF6YaF8nwQgbMTWx54Ugkf4NZvSxdzicENHoLJh96EKg
from #6625 with 11k TXes and 10.5k addresses),
this takes 1.3s of 100%ed CPU usage,
basically in a loop from the UI thread.
get_utxos() is 50-70% of the flame-graph when sampling
a synced wallet process.
This data is a function of the block-chain state,
and we have hooks that notify us of when the block-chain state changes:
we can just cache the result and only re-compute it then.
For example, here's a trace log where get_utxos() has
print(end - start, len(domain), block_height)
and a transaction is clearing:
1.3775344607420266 10540 4507192
0.0010390589013695717 10540 4507192 cached!
0.001393263228237629 10540 4507192 cached!
0.0009001069702208042 10540 4507192 cached!
0.0010241391137242317 10540 4507192 cached!
...
0.00207632128149271 10540 4507192 cached!
0.001397700048983097 10540 4507192 cached!
invalidate_cache
1.4686454269103706 10540 4507192
0.0012429207563400269 10540 4507192 cached!
0.0015075239352881908 10540 4507192 cached!
0.0010459059849381447 10540 4507192 cached!
0.0009669591672718525 10540 4507192 cached!
...
on_event_blockchain_updated
invalidate_cache
1.3897203942760825 10540 4507193
0.0010689008049666882 10540 4507193 cached!
0.0010420521721243858 10540 4507193 cached!
...
invalidate_cache
1.408584670163691 10540 4507193
0.001336586195975542 10540 4507193 cached!
0.0009196233004331589 10540 4507193 cached!
0.0009176661260426044 10540 4507193 cached!
...
about 30s of low activity.
Without this patch, the UI is prone to freezing,
running behind, and I wouldn't be surprised if UI thread blocking
on Windows ends up crashing to some extent as the issue notes.
In the log, this manifests as a much slower but consistent
stream of full 1.3-1.4s updates during use,
and every time the window is focused.
get_tx_height previously did not consider whether the walletDB has the full tx for
the corresponding txid, and could consider a tx even "mined" and spv-ed, even if
we were missing the raw tx.
Now if the full tx is missing or the tx in the db is partial,
get_tx_height considers it to be LOCAL.
In particular the txbatcher, in `run_iteration`,
- first saves a tx as local *unsigned* (to reserve UTXOs),
- then signs it and tries to broadcast it.
The history tx will later transition to local and signed,
after we get back the broadcasted tx via the Synchronizer dance.
In the meantime there is a race where we have an unsigned tx in the history,
but the txid could already transition to mempool or even to mined,
before we download the full raw tx from the server.
During that time window, it makes the adb state more consistent
to just consider the history tx to be local, as done here.
tx_height comes from the `get_history` RPC, we then call the `get_transaction` RPC.
By the time the `get_transaction` RPC returns, we might have received another
scripthash status update, called `get_history` again, and updated height for the txid.
Synchronizer._get_transaction() should not call adb.receive_tx_callback() with
the old tx_height (but it was doing exactly that).
Patch to trigger, e.g. regtest failures:
(e.g. for tests.regtest.TestLightningAB.test_extract_preimage)
```
diff --git a/electrum/interface.py b/electrum/interface.py
index 8649652b9c..fce7a1c6de 100644
--- a/electrum/interface.py
+++ b/electrum/interface.py
@@ -991,6 +991,7 @@ class Interface(Logger):
return res
async def get_transaction(self, tx_hash: str, *, timeout=None) -> str:
+ await asyncio.sleep(3)
if not is_hash256_str(tx_hash):
raise Exception(f"{repr(tx_hash)} is not a txid")
raw = await self.session.send_request('blockchain.transaction.get', [tx_hash], timeout=timeout)
```
Adds a new config option: `WALLET_FREEZE_REUSED_ADDRESS_UTXOS`.
This is based on Bitcoin Core's "avoid_reuse" wallet flag. [0]
This opt-in feature, if enabled:
> Automatically freeze coins received to already used addresses.
> This can eliminate a serious privacy issue where a malicious user can track your spends by sending small payments
> to a previously-paid address of yours that would then be included with unrelated inputs in your future payments.
Note that currently we only have a single coinchooser policy, `CoinChooserPrivacy`,
which interacts well with this option, as it spends all coins from any selected address.
However, if we later add a different coinchooser policy, which allowed "partial spends",
care should be taken re e.g. disallowing using that when this option is set.
Also note that this PR adds this as a config option, but arguably it could be wallet-specific instead,
such as `use_change`.
[0]: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.19.0.1.md#wallet
closes https://github.com/spesmilo/electrum/issues/7497
Previously calling add_transaction with a malformed Transaction obj could
result in an exception late in the flow, after the walletdb was already side-effected.
Rollback of such side-effects is not implemented :/
but this small patch should at least cover and prevent some common cases.
```
File "/opt/electrum/electrum/address_synchronizer.py", line 358, in add_transaction
self.db.add_transaction(tx_hash, tx)
File "/opt/electrum/electrum/json_db.py", line 42, in wrapper
return func(self, *args, **kwargs)
File "/opt/electrum/electrum/wallet_db.py", line 1434, in add_transaction
tx = tx_from_any(str(tx))
File "/opt/electrum/electrum/transaction.py", line 1339, in tx_from_any
raise SerializationError(f"Failed to recognise tx encoding, or to parse transaction. "
```
Instead of some functions operating with hex strings,
and others using bytes, this consolidates most things to use bytes.
This mainly focuses on bitcoin.py and transaction.py,
and then adapts the API usages in other files.
Notably,
- scripts,
- pubkeys,
- signatures
should be bytes in almost all places now.
I am only making this change as it makes it simpler to do manual testing of CPFP-ing *local* txs.
That is, by changing a few easy-to-find lines of code, I can make the GUI allow CPFP-ing a local tx, but
without this change it errors out with "unknown fee for parent transaction".
If one has an incoming local tx saved in the wallet, adb.get_tx_fee(txid) returns None
(if the tx gets into the mempool, it will use the server-reported value instead).
In contrast, wallet.get_tx_info(tx).fee calls both wallet.get_wallet_delta(tx) and adb.get_tx_fee(txid),
making it more expensive but more complete.
A new config API is introduced, and ~all of the codebase is adapted to it.
The old API is kept but mainly only for dynamic usage where its extra flexibility is needed.
Using examples, the old config API looked this:
```
>>> config.get("request_expiry", 86400)
604800
>>> config.set_key("request_expiry", 86400)
>>>
```
The new config API instead:
```
>>> config.WALLET_PAYREQ_EXPIRY_SECONDS
604800
>>> config.WALLET_PAYREQ_EXPIRY_SECONDS = 86400
>>>
```
The old API operated on arbitrary string keys, the new one uses
a static ~enum-like list of variables.
With the new API:
- there is a single centralised list of config variables, as opposed to
these being scattered all over
- no more duplication of default values (in the getters)
- there is now some (minimal for now) type-validation/conversion for
the config values
closes https://github.com/spesmilo/electrum/pull/5640
closes https://github.com/spesmilo/electrum/pull/5649
Note: there is yet a third API added here, for certain niche/abstract use-cases,
where we need a reference to the config variable itself.
It should only be used when needed:
```
>>> var = config.cv.WALLET_PAYREQ_EXPIRY_SECONDS
>>> var
<ConfigVarWithConfig key='request_expiry'>
>>> var.get()
604800
>>> var.set(3600)
>>> var.get_default_value()
86400
>>> var.is_set()
True
>>> var.is_modifiable()
True
```
Previously if a local tx got broadcast, it was still displayed as local
in the history until it got mined (or some other action forced a full refresh).
- they are now distinguished from local by the status text "in %d blocks"
- this status text needs updating occasionally: on new blocks,
and some lnwatcher interactions, hence new event: "adb_set_future_tx"
- wallet.add_input_info() previously had a fallback to download parent
prev txs from the network (after a lookup in wallet.db failed).
wallet.add_input_info() is not async, so the network request cannot
be done cleanly there and was really just a hack.
- tx.add_info_from_wallet() calls wallet.add_input_info() on each txin,
in which case these network requests were done sequentially, not concurrently
- the network part of wallet.add_input_info() is now split out into new method:
txin.add_info_from_network()
- in addition to tx.add_info_from_wallet(), there is now also tx.add_info_from_network()
- callers of old tx.add_info_from_wallet() should now called either
- tx.add_info_from_wallet(), then tx.add_info_from_network(), preferably in that order
- tx.add_info_from_wallet() alone is sufficient if the tx is complete,
or typically when not in a signing context
- callers of wallet.bump_fee and wallet.dscancel are now expected to have already
called tx.add_info_from_network(), as it cannot be done in a non-async context
(but for the common case of all-inputs-are-ismine, bump_fee/dscancel should work regardless)
- PartialTxInput.utxo was moved to the baseclass, TxInput.utxo
fixes https://github.com/spesmilo/electrum/issues/8240#8240 was triggering an AssertionError in wallet.get_invoice_status,
as code there was assuming conf >= 0. To trigger, force-close
a LN channel, and while the sweep is waiting on the CSV, try to
make a payment in the Send tab to the ismine change address used
for the sweep in the future_tx. (order of events can also be reversed)
- add a new event, 'adb_removed_tx'
- new wallet method: get_tx_parents
- number of parents is shown in coins tab
- detailed list of parents is shown in dialog
ShortIDs were originally designed for lightning channels, and are now
understood by some block explorers.
This allows to remove one column in the UTXO tab (height is redundant).
In the transaction dialog, the space saving ensures that all inputs fit
into one line (it was not the case previously with p2wsh addresses).
For clarity and consistency, the ShortID is displayed for both inputs
and outputs in the transaction dialog.
ThomasV