Somewhat a follow-up to 649ce979ab.
This adds some safety belts so we don't accidentally sign a tx that
contains a dummy address.
Specifically we check that tx does not contain output for dummy addr:
- in wallet.sign_transaction
- in network.broadcast_transaction
The second one is perhaps redundant, but I think it does not hurt.
Plugins.stop() is now part of the normal shutdown flow (since 90f39bce88),
so this shaves up to 100 ms off that (avg: 50 ms).
It is also waited on in around ~60 unit tests: this saves 3 sec.
Plugins.stop was never called, so the Plugins thread only stopped
because of the is_running() check in run(), which triggers too late:
the Plugins thread was stopping after the main thread stopped.
E.g. playing around in the qt wizard with wallet creation for a Trezor,
and closing the wizard (only window):
``` 24.85 | E | p/plugin.Plugins |
Traceback (most recent call last):
File "/home/user/wspace/electrum/electrum/util.py", line 386, in run_jobs
job.run()
File "/home/user/wspace/electrum/electrum/plugin.py", line 430, in run
client.timeout(cutoff)
File "/home/user/wspace/electrum/electrum/plugin.py", line 363, in wrapper
return run_in_hwd_thread(partial(func, *args, **kwargs))
File "/home/user/wspace/electrum/electrum/plugin.py", line 355, in run_in_hwd_thread
fut = _hwd_comms_executor.submit(func)
File "/usr/lib/python3.10/concurrent/futures/thread.py", line 167, in submit
raise RuntimeError('cannot schedule new futures after shutdown')
RuntimeError: cannot schedule new futures after shutdown
```
This clears up log spam for regtest tests.
related:
- https://bugs.python.org/issue44665
- https://github.com/python/cpython/issues/88831
- https://textual.textualize.io/blog/2023/02/11/the-heisenbug-lurking-in-your-async-code/
- https://github.com/python/cpython/issues/91887#issuecomment-1434816045
- "Task was destroyed but it is pending!"
Perhaps we should inspect all our usages of
- asyncio.create_task
- loop.create_task
- asyncio.ensure_future
- asyncio.run_coroutine_threadsafe
?
Example log for running a regtest test:
```
$ python3 -m unittest electrum.tests.regtest.TestLightningAB.test_collaborative_close
***** test_collaborative_close ******
initializing alice
--- Logging error ---
Traceback (most recent call last):
File "/usr/lib/python3.10/logging/__init__.py", line 1100, in emit
msg = self.format(record)
File "/usr/lib/python3.10/logging/__init__.py", line 943, in format
return fmt.format(record)
File "/home/user/wspace/electrum/electrum/logging.py", line 44, in format
record = copy.copy(record) # avoid mutating arg
File "/usr/lib/python3.10/copy.py", line 92, in copy
rv = reductor(4)
ImportError: sys.meta_path is None, Python is likely shutting down
Call stack:
File "/usr/lib/python3.10/asyncio/base_events.py", line 1781, in call_exception_handler
self._exception_handler(self, context)
File "/home/user/wspace/electrum/electrum/util.py", line 1535, in on_exception
loop.default_exception_handler(context)
File "/usr/lib/python3.10/asyncio/base_events.py", line 1744, in default_exception_handler
logger.error('\n'.join(log_lines), exc_info=exc_info)
Message: "Task was destroyed but it is pending!\ntask: <Task pending name='Task-2' coro=<Abstract_Wallet.on_event_adb_set_up_to_date() running at /home/user/wspace/electrum/electrum/wallet.py:485> wait_for=<Future finished result=0> cb=[_chain_future.<locals>._call_set_state() at /usr/lib/python3.10/asyncio/futures.py:392]>"
Arguments: ()
[--- SNIP --- more of the same --- SNIP ---]
--- Logging error ---
Traceback (most recent call last):
File "/usr/lib/python3.10/logging/__init__.py", line 1100, in emit
msg = self.format(record)
File "/usr/lib/python3.10/logging/__init__.py", line 943, in format
return fmt.format(record)
File "/home/user/wspace/electrum/electrum/logging.py", line 44, in format
record = copy.copy(record) # avoid mutating arg
File "/usr/lib/python3.10/copy.py", line 92, in copy
rv = reductor(4)
ImportError: sys.meta_path is None, Python is likely shutting down
Call stack:
File "/usr/lib/python3.10/asyncio/base_events.py", line 1781, in call_exception_handler
self._exception_handler(self, context)
File "/home/user/wspace/electrum/electrum/util.py", line 1535, in on_exception
loop.default_exception_handler(context)
File "/usr/lib/python3.10/asyncio/base_events.py", line 1744, in default_exception_handler
logger.error('\n'.join(log_lines), exc_info=exc_info)
Message: "Task was destroyed but it is pending!\ntask: <Task pending name='Task-31' coro=<Abstract_Wallet.on_event_adb_set_up_to_date() running at /home/user/wspace/electrum/electrum/wallet.py:485> wait_for=<Future pending cb=[_chain_future.<locals>._call_check_cancel() at /usr/lib/python3.10/asyncio/futures.py:385, Task.task_wakeup()]> cb=[_chain_future.<locals>._call_set_state() at /usr/lib/python3.10/asyncio/futures.py:392]>"
Arguments: ()
true
true
true
true
funding alice
```
- standardize_path is made more lenient: it no longer calls os.path.realpath,
as that was causing issues on Windows with some mounted drives
- daemon._wallets is still keyed on the old strict standardize_path, but
filesystem operations in WalletStorage use the new lenient standardize_path.
- daemon._wallets is strict to forbid opening the same logical file twice concurrently
- fs operations however work better on the non-resolved paths, so they use those
closes https://github.com/spesmilo/electrum/issues/8495
decorators (instead of overloading JsonDB._convert_dict and
_convert_value)
- stored_in for elements of a StoreDict
- stored_as for singletons
- extra register methods are defined for key conversions
This commit was adapted from the jsonpatch branch
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
```
- case 1: in version 4.4.1, 4.4.2, the qml GUI wizard allowed creating multisig wallets with an old_mpk as cosigner.
- case 2: in version 4.4.0, 4.4.1, 4.4.2, the qml GUI wizard allowed creating multisig wallets with mixed xpub/Ypub/Zpub.
The corresponding missing input validation was a bug in the wizard, it was unintended behaviour. Validation was added in d2cf21fc2b. Note however that there might be users who created such wallet files.
Re case 1 wallet files: there is no version of Electrum that allows spending from such a wallet. Coins received at addresses are not burned, however it is technically challenging to spend them. (unless the multisig can spend without needing the old_mpk cosigner in the quorum).
Re case 2 wallet files: it is possible to create a corresponding spending wallet for such a multisig, however it is a bit tricky. The script type for the addresses in such a heterogeneous xpub wallet is based on the xpub_type of the first keystore. So e.g. given a wallet file [Yprv1, Zpub2] it will have sh(wsh()) scripts, and the cosigner should create a wallet file [Ypub1, Zprv2] (same order).
Technically case 2 wallet files could be "fixed" automatically by converting the xpub types as part of a wallet_db upgrade. However if the wallet files also contain seeds, those cannot be converted ("standard" vs "segwit" electrum seed).
Case 1 wallet files are not possible to "fix" automatically as the cosigner using the old_mpk is not bip32 based.
It is unclear if there are *any* users out there affected by this. I suspect for case 1 it is very likely there are none (not many people have pre-2.0 electrum seeds which were never supported as part of a multisig who would also now try to create a multisig using them); for case 2 however there might be.
This commit breaks both case 1 and case 2 wallets: these wallet files can no longer be opened in new Electrum, an error message is shown and the crash reporter opens. If any potential users opt to send crash reports, at least we will know they exist and can help them recover.
When called via jsonrpc (but not via cli) with non-string amounts,
there could be a rounding error resulting in sending 1 sat less.
example:
```
$ ./run_electrum --testnet -w ~/.electrum/testnet/wallets/test_segwit_2 paytomany '[["tb1q6k5h4cz6ra8nzhg90xm9wldvadgh0fpttfthcg", 0.00033389]]' --fee 0
02000000000101b9e6018acb16952e3c9618b069df404dc85544eda8120e5f6e7cd7e94ce5ae8d0100000000fdffffff02fd8100000000000016001410c5b97085ec1637a9f702852f5a81f650fae1566d82000000000000160014d5a97ae05a1f4f315d0579b6577daceb5177a42b024730440220251d2ce83f6e69273de8e9be8602fbcf72b9157e1c0116161fa52f7e04db6e4302202d84045cc6b7056a215d1db3f59884e28dadd5257e1a3960068f90df90b452d1012102b0eff3bf364a2ab5effe952cba33521ebede81dac88c71951a5ed598cb48347b3a022500
$ curl --data-binary '{"id":"curltext","method":"paytomany","params":{"outputs":[["tb1q6k5h4cz6ra8nzhg90xm9wldvadgh0fpttfthcg", 0.00033389]], "fee": 0, "wallet": "/home/user/.electrum/testnet/wallets/test_segwit_2"}}' http://user:pass@127.0.0.1:7777
{"id": "curltext", "jsonrpc": "2.0", "result": "02000000000101b9e6018acb16952e3c9618b069df404dc85544eda8120e5f6e7cd7e94ce5ae8d0100000000fdffffff02fe8100000000000016001410c5b97085ec1637a9f702852f5a81f650fae1566c82000000000000160014d5a97ae05a1f4f315d0579b6577daceb5177a42b0247304402206ef66b845ca298c14dc6e8049cba9ed19db1671132194518ce5d521de6f5df8802205ca4b1aee703e3b98331fb9b88210917b385560020c8b2a8a88da38996b101c4012102b0eff3bf364a2ab5effe952cba33521ebede81dac88c71951a5ed598cb48347b39022500"}
```
^ note that first tx has output for 0.00033389, second tx has output for 0.00033388
fixes https://github.com/spesmilo/electrum/issues/8274
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)
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.
With a PyCharm debugger attached, sometimes the python process is so
CPU-starved for me that create_and_start_event_loop() returned
before the event loop actually started, resulting in weird errors.
I guess this could happen even without a debugger attached on a
sufficiently slow CPU.
```
...\electrum\electrum\wallet.py:3580: in restore_wallet_from_text
wallet = Wallet(db, storage, config=config)
...\electrum\electrum\wallet.py:3501: in __new__
wallet = WalletClass(db, storage, config=config)
...\electrum\electrum\wallet.py:3345: in __init__
Deterministic_Wallet.__init__(self, db, storage, config=config)
...\electrum\electrum\wallet.py:3135: in __init__
self.synchronize()
...\electrum\electrum\wallet.py:3283: in synchronize
count += self.synchronize_sequence(False)
...\electrum\electrum\wallet.py:3267: in synchronize_sequence
self.create_new_address(for_change)
...\electrum\electrum\wallet.py:3254: in create_new_address
self.adb.add_address(address)
...\electrum\electrum\address_synchronizer.py:213: in add_address
self.up_to_date_changed()
...\electrum\electrum\address_synchronizer.py:680: in up_to_date_changed
util.trigger_callback('adb_set_up_to_date', self)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <electrum.util.CallbackManager object at 0x000002B1788AD6F0>
event = 'adb_set_up_to_date'
args = (<electrum.address_synchronizer.AddressSynchronizer object at 0x000002B17A687670>,)
def trigger_callback(self, event, *args):
"""Trigger a callback with given arguments.
Can be called from any thread. The callback itself will get scheduled
on the event loop.
"""
if self.asyncio_loop is None:
self.asyncio_loop = get_asyncio_loop()
> assert self.asyncio_loop.is_running(), "event loop not running"
E AssertionError: event loop not running
...\electrum\electrum\util.py:1734: AssertionError
```
Always use "." as decimal point, and " " as thousands separator.
Previously,
- for decimal point, we were using
- "." in some places (e.g. AmountEdit, most fiat amounts), and
- `locale.localeconv()['decimal_point']` in others.
- for thousands separator, we were using
- "," in some places (most fiat amounts), and
- " " in others (format_satoshis)
I think it is better to be consistent even if whatever we pick differs from the locale.
Using whitespace for thousands separator (vs comma) is probably less confusing for people
whose locale would user "." for ts and "," for dp (as in e.g. German).
The alternative option would be to always use the locale. Even if we decide to do that later,
this refactoring should be useful.
closes https://github.com/spesmilo/electrum/issues/2629
We would reject bip21 URIs that contained both an "address=" and a "lightning=" key with a bolt11 invoice,
where the bolt11 invoice did not contain a fallback address.
fixes https://github.com/spesmilo/electrum/issues/7952
see in particular https://github.com/spesmilo/electrum/issues/7952#issuecomment-1227225602
> So the issue is with the aiorpcx monkey patch in util.py, as it
> relies on side-effecting the asyncio.Task, and it patches Task.cancel.
> However, aiohttp also uses Task.cancel for its own timeouting of the
> http request, with the same Task object, and this confuses timeout_after.
> Ultimately FxThread.run exits.
related https://github.com/kyuupichan/aiorpcX/pull/47
---
note: I am not content at all with this monkey-patching approach,
but at the same time I don't see how to properly fix this handling all
edge-cases in aiorpcx.
python 3.11 is finally adding an implementation of TaskGroup [0] and
an async timeout context manager [1] in the asyncio module of the stdlib.
Looking at the implementations, they look unfeasible to backport:
much of the implementation of asyncio.Task had to be changed for them
to work, and TaskGroup in particular relies on the new ExceptionGroups.
Some of these edge cases we are battling with aiorpcx.curio look
difficult to fix without those stdlib changes...
Anyway, when we bump the min python to 3.11, I look forward to switching
to that code instead, and ripping this stuff out.
[0]: https://docs.python.org/3.11/library/asyncio-task.html#task-groups
[1]: https://docs.python.org/3.11/library/asyncio-task.html#asyncio.timeout
If keystore.check_password is called with some pw on a keystore that does not have a password set,
it now raises better exceptions: it should now always raise InvalidPassword, and with a nicer msg.
Previously the exc type would depend on the ks type.
Examples before change:
```
>>> wallet.keystore.check_password("asd")
Traceback (most recent call last):
File "/home/user/wspace/electrum/electrum/keystore.py", line 580, in check_password
xprv = pw_decode(self.xprv, password, version=self.pw_hash_version)
File "/home/user/wspace/electrum/electrum/crypto.py", line 311, in pw_decode
plaintext_bytes = pw_decode_bytes(data, password, version=version)
File "/home/user/wspace/electrum/electrum/crypto.py", line 270, in pw_decode_bytes
data_bytes = bytes(base64.b64decode(data))
File "/usr/lib/python3.10/base64.py", line 87, in b64decode
return binascii.a2b_base64(s)
binascii.Error: Incorrect padding
```
```
>>> wallet.keystore.check_password("asd")
Traceback (most recent call last):
s = aes_decrypt_with_iv(secret, iv, e)
File "/home/user/wspace/electrum/electrum/crypto.py", line 157, in aes_decrypt_with_iv
data = decryptor.update(data) + decryptor.finalize()
File "/usr/lib/python3/dist-packages/cryptography/hazmat/primitives/ciphers/base.py", line 148, in finalize
data = self._ctx.finalize()
File "/usr/lib/python3/dist-packages/cryptography/hazmat/backends/openssl/ciphers.py", line 193, in finalize
raise ValueError(
ValueError: The length of the provided data is not a multiple of the block length.
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/home/user/wspace/electrum/electrum/gui/qt/console.py", line 254, in exec_command
result = eval(command, self.namespace, self.namespace)
File "<string>", line 1, in <module>
File "/home/user/wspace/electrum/electrum/keystore.py", line 248, in check_password
self.get_private_key(pubkey, password)
File "/home/user/wspace/electrum/electrum/keystore.py", line 267, in get_private_key
sec = pw_decode(self.keypairs[pubkey], password, version=self.pw_hash_version)
File "/home/user/wspace/electrum/electrum/crypto.py", line 311, in pw_decode
plaintext_bytes = pw_decode_bytes(data, password, version=version)
File "/home/user/wspace/electrum/electrum/crypto.py", line 271, in pw_decode_bytes
return _pw_decode_raw(data_bytes, password, version=version)
File "/home/user/wspace/electrum/electrum/crypto.py", line 255, in _pw_decode_raw
raise InvalidPassword() from e
electrum.util.InvalidPassword: Incorrect password
```
-----
Examples after change:
```
>>> wallet.keystore.check_password("asd")
Traceback (most recent call last):
return binascii.a2b_base64(s)
binascii.Error: Incorrect padding
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "...\electrum\keystore.py", line 68, in wrapper
return check_password_fn(self, password)
File "...\electrum\keystore.py", line 605, in check_password
xprv = pw_decode(self.xprv, password, version=self.pw_hash_version)
File "...\electrum\crypto.py", line 311, in pw_decode
plaintext_bytes = pw_decode_bytes(data, password, version=version)
File "...\electrum\crypto.py", line 267, in pw_decode_bytes
raise CiphertextFormatError("ciphertext not valid base64") from e
electrum.crypto.CiphertextFormatError: ciphertext not valid base64
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "...\electrum\gui\qt\console.py", line 254, in exec_command
result = eval(command, self.namespace, self.namespace)
File "<string>", line 1, in <module>
File "...\electrum\keystore.py", line 76, in wrapper
raise InvalidPassword("password given but keystore has no password") from e
electrum.util.InvalidPassword: password given but keystore has no password
```
```
>>> wallet.keystore.check_password("asd")
Traceback (most recent call last):
s = aes_decrypt_with_iv(secret, iv, e)
File "...\electrum\crypto.py", line 158, in aes_decrypt_with_iv
data = cipher.decrypt(data)
File "...\Python310\site-packages\Cryptodome\Cipher\_mode_cbc.py", line 246, in decrypt
raise ValueError("Data must be padded to %d byte boundary in CBC mode" % self.block_size)
ValueError: Data must be padded to 16 byte boundary in CBC mode
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "...\electrum\keystore.py", line 68, in wrapper
return check_password_fn(self, password)
File "...\electrum\keystore.py", line 272, in check_password
self.get_private_key(pubkey, password)
File "...\electrum\keystore.py", line 291, in get_private_key
sec = pw_decode(self.keypairs[pubkey], password, version=self.pw_hash_version)
File "...\electrum\crypto.py", line 311, in pw_decode
plaintext_bytes = pw_decode_bytes(data, password, version=version)
File "...\electrum\crypto.py", line 268, in pw_decode_bytes
return _pw_decode_raw(data_bytes, password, version=version)
File "...\electrum\crypto.py", line 249, in _pw_decode_raw
raise InvalidPassword() from e
electrum.util.InvalidPassword: Incorrect password
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "...\electrum\gui\qt\console.py", line 254, in exec_command
result = eval(command, self.namespace, self.namespace)
File "<string>", line 1, in <module>
File "...\electrum\keystore.py", line 76, in wrapper
raise InvalidPassword("password given but keystore has no password") from e
electrum.util.InvalidPassword: password given but keystore has no password
```
