Timelock Recovery Extension #9589
Conversation
oren-z0
force-pushed
the
timelockrecovery
branch
5 times, most recently
from
bc9ebf2 to
fcf81f1
Compare
|
Note: PTMono font files are already present in the repository for QML. We could move those up a directory to make them available for desktop GUI. |
|
@accumulator That would be great, just move them to a global directory and I'll add a fix. |
There was a problem hiding this comment.
In general, I have some code maintainability concerns. This is a lot of code, and without tests or type hints.
- try to avoid duplicating code (both existing code and new code you write)
- try to avoid tightly interacting with internal implementation details of the main code
- add some type hints. That significantly helps IDEs (and hence maintainers) find bugs and notice what is affected by future refactors.
- split out the non-GUI stuff, at least the tx construction, and write tests for that (see separate comment)
| make_tx = lambda fee_est, *, confirmed_only=False: self.wallet.make_unsigned_transaction( | ||
| coins=[tx_input], | ||
| outputs=[output for output in self.outputs if output.value != 0], | ||
| fee=fee_est, | ||
| is_sweep=False, | ||
| ) |
There was a problem hiding this comment.
Please factor out the tx construction bits into separate non-GUI functions, and write "unit tests" for them. e.g. create a wallet, fund it with a few utxos, and then construct the alert/recovery/cancel txs and test that they look as expected (e.g. compare txids against hardcoded values).
See e.g.
electrum/tests/test_wallet_vertical.py
Line 862 in fc093f8
See also how e.g. the "revealer" plugin is split into non-GUI and GUI parts, and how some non-GUI parts have tests.
There was a problem hiding this comment.
Refactored. It was more convenient to implement non-GUI functions in a "Context" object that moves between the dialog function calls, rather than in the singleton TimelockRecoveryPlugin.
The relevant functions are for allocating addresses in the original wallet (alert-address, cancellation-address), and for making unsigned transactions (one of them, the "recovery" transaction, is unique for having a non-default nSequence).
| "recovery_txid": self.recovery_tx.txid(), | ||
| } | ||
| # Simple checksum to ensure the file is not corrupted by foolish users | ||
| json_data["checksum"] = hashlib.sha256(json.dumps(sorted(json_data.items()), separators=(',', ':')).encode()).hexdigest() |
There was a problem hiding this comment.
Most of the time it is better to avoid duplicating code. Move this checksum calc to its own function.
Also, note that json.dumps() has more arguments you might want to explicitly set. You would not want this hash to implicitly depend on the python version just because default values change over time.
E.g. indent=None
There is also sort_keys=True btw.
There was a problem hiding this comment.
Refactored to specify all arguments.
I prefer converting the json_data dict to a list of key-value pairs, because this method is consistent with other
programming languages that might not have a sort_keys feature - i.e. for a service that monitors/broadcasts the
transactions.
| painter.end() | ||
|
|
||
| self.download_dialog.show_message(_("File saved successfully")) | ||
| except Exception as e: |
There was a problem hiding this comment.
This generic try-except is over 300 lines long. There are two of these which are so long that they do not fit on my screen. This is not okay.
Most of the time try-excepts should be narrow, ideally trying to catch specific exceptions.
E.g. if there is a SyntaxError here, we want that to propagate out to the crash reporter so that user might send it to the issue tracker and we can fix it.
There was a problem hiding this comment.
Refactored, and removed most code duplication.
The pdf generation is still a long process because it has a lot of details.
The try-catch now looks only for IOError and MemoryError - I was trying to check if there are problems writing to the disk.
I also improved the mechanism to create a temp file first, and then rename it to the final destination name.
| def _paint_qr(self, qr): | ||
| matrix = qr.get_matrix() | ||
| k = len(matrix) | ||
| border_color = Qt.GlobalColor.white |
There was a problem hiding this comment.
This function is mostly just copy-pasted from the revealer plugin.
electrum/electrum/plugins/revealer/qt.py
Line 818 in fc093f8
and there is yet another function for drawing QR codes in common_qt/util
Please don't introduce even more duplication. Feel free to try to move or refactor (if needed) the existing code and just use that.
There was a problem hiding this comment.
Refactored to avoid code duplication, by using draw_qr.
(P.S. I blame this code duplication on Cursor AI autocompletion...).
| for output in self.outputs | ||
| ] | ||
| make_tx = lambda fee_est, *, confirmed_only=False: self.wallet.make_unsigned_transaction( | ||
| coins=window.parent().get_coins(nonlocal_only=False, confirmed_only=confirmed_only), |
There was a problem hiding this comment.
What's the thinking behind nonlocal_only=False?
There was a problem hiding this comment.
I actually can't recall myself, since it is False by default anyways.
| def format_sats_as_btc(value): | ||
| return f"{(Decimal(value) / Decimal(COIN)):.8f}" |
There was a problem hiding this comment.
just use config.format_amount, respecting the user settings
| self.wallet = window.parent().wallet | ||
| self.wallet_name = str(self.wallet) |
There was a problem hiding this comment.
Note that the Plugin instance is a singleton, but there might be multiple ElectrumWindows (main_window) open each with their own wallet. There might be weird bugs if the user interacts with this plugin from multiple wallet windows if you store fields in the Plugin instance.
There was a problem hiding this comment.
done. Using a TimelockRecoveryContext object that is passed between the dialogs and collects the results.
| nsequence=nsequence, | ||
| ) | ||
| tx_input.utxo = self.alert_tx | ||
| tx_input.witness_utxo = prevout |
There was a problem hiding this comment.
This looks incorrect. What are the types here?
There was a problem hiding this comment.
I've added the types.
The Trezor plugin and some other HW wallet plugins expect the utxo field to be set to the previous transaction:
https://github.com/spesmilo/electrum/blob/master/electrum/plugins/trezor/trezor.py#L94-L95
utxo is a bad name in my opinion (usually "utxo" refers to a pair of transaction-id and index), but it makes sense that in pre-segwit transaction, the HW wallet will require to see the entire previous transaction, and verify that the relevant outpoint really contains the amount of Bitcoin that is being signed.
Otherwise a hacked computer can make a user sign a valid transaction with a very different fee that what he intended.
This could be slow when the transaction being signed has a lot of inputs, and each one comes from a large transaction on its own - a lot of data needs to be transferred over the USB cable.
Segwit fixed this by adding the amount to the signature, so if a hacked computer makes the user sign an input that actually has a different amount, this transaction will be invalid.
I see that the safe_t plugin is actually handling this case correctly, and allows utxo to be None when it's a segwit transaction.
https://github.com/spesmilo/electrum/blob/master/electrum/plugins/safe_t/safe_t.py#L49-L50
|
Many thanks for the quick response. I will work on your notes this week. |
oren-z0
force-pushed
the
timelockrecovery
branch
16 times, most recently
from
dfbcea0 to
023bb8c
Compare
see #9595 |
oren-z0
force-pushed
the
timelockrecovery
branch
3 times, most recently
from
9d35281 to
b203da7
Compare
oren-z0
force-pushed
the
timelockrecovery
branch
11 times, most recently
from
512c11c to
7fad62f
Compare
|
@SomberNight I hope my refactoring is better now. Also added a few tests. Please let me know if you have any comments. |
oren-z0
force-pushed
the
timelockrecovery
branch
6 times, most recently
from
bc21622 to
96bdb69
Compare
oren-z0
force-pushed
the
timelockrecovery
branch
from
96bdb69 to
9be9aa9
Compare
Finished implemented my Timelock Recovery extension.
A demo can be seen here: https://v.nostr.build/vVbih1XoqIfKAaZi.mp4 (UI can be seen at 10:00). More details at: https://timelockrecovery.com .
I will be happy to go on a video-chat with you guys to explain what I built.
Closes #9414 .