This test was flaky: the mpp_set resolution gets set to SETTLING several asyncio event loop iterations before the hold invoice callback "cb" gets called.
If the 0.1 sec polling triggers just in the middle of that interval, `assert cb_got_called` fails.
```
async def check_mpp_state():
async def wait_for_resolution():
while True:
await asyncio.sleep(0.1)
if payment_key not in bob_w.received_mpp_htlcs:
continue
if not bob_w.received_mpp_htlcs[payment_key].resolution == RecvMPPResolution.SETTLING:
continue
return
await util.wait_for2(wait_for_resolution(), timeout=2)
> assert cb_got_called
E assert False
tests/test_lnpeer.py:1898: AssertionError
```
see https://github.com/spesmilo/electrum/blob/16c8cb50e38c274cce8f9f66f28d8dd453f9f074/electrum/lnpeer.py#L3136-L3137
fixes https://github.com/spesmilo/electrum/issues/10589
-----
diff to reproduce the failure without present patch:
```
diff --git a/tests/test_lnpeer.py b/tests/test_lnpeer.py
index 8669931c24..e15973d68f 100644
--- a/tests/test_lnpeer.py
+++ b/tests/test_lnpeer.py
@@ -1885,6 +1885,7 @@ class TestPeerDirect(TestPeer):
cb_got_called = False
async def cb(_payment_hash):
self.logger.debug(f"hold invoice callback called. {bob_w.network.get_local_height()=}")
+ await asyncio.sleep(1)
nonlocal cb_got_called
cb_got_called = True
```
Due to how the txid-commitment merkle tree used in the block headers is constructed, we need an extra check to be able to validate the *position* of a txid in a block.
I think this is low severity for us.
See https://bitcointalk.org/?topic=102395 :
> The Merkle hash implementation that Bitcoin uses to calculate the Merkle
> root in a block header is flawed in that one can easily construct multiple
> lists of hashes that map to the same Merkle root.
> For example, merkle_hash([a, b, c]) and merkle_hash([a, b, c, c]) yield
> the same result. This is because, at every iteration, the Merkle hash
> function pads its intermediate list of hashes with the last hash if the
> list is of odd length, in order to make it of even length.
>
> And so, the Merkle root function can be effectively preimaged by
> changing the input so that one of the intermediate lists is of even
> length with the last two elements equal (where originally it was
> of odd length with a last element equal to the earlier mentioned two).
> As was later noted, this extends to any input length that is
> not a power of two:
> merkle_hash([a, b, c, d, e, f]) == merkle_hash([a, b, c, d, e, f, e, f]).
> Note that to maintain the same root hash, the only flexibility that
> exists is duplication of elements.
Ported from https://github.com/Electron-Cash/Electron-Cash/commit/165146362b4cb0ad74770b36aca1f9acb2800195
Co-authored-by: bitcoincashautist <80100588+A60AB5450353F40E@users.noreply.github.com>
Don't include first hop of the path,
this is the hop from us to the first node and we don't
need a payload for ourselves.
Also adds unittest checking this.
Add unittest that verifies we only include r_tags for trampoline nodes
if we signal trampoline support in the invoice_features and only signal
trampoline support if we use trampoline or have only open trampoline
channels.
- could not find a single project that still actually cares about bip70 [0]
- well except maybe BitPay.
- but I cannot test with BitPay:
- they have a testnet3 staging environment on test.bitpay.com
- but the SSL cert they use for bip70 has expired in 2021
- the webUI probably also has not been updated since then...
- they claim to have added LN support in 2022 in a blog post,
but it's not there on test.bitpay.com
- on mainnet, they require KYC before payment
- < ... angry noises >
- their loss then, I don't care.
- this is code that no one wants to maintain
- this does not yet delete the signed bip70 payment data for historical txs
- but it is no longer possible to export it from the GUI
[0]: https://bitcoinops.org/en/topics/bip70-payment-protocol/
If SwapManager.percentage was a 0.2 float, rounding differences would
cause an exception in the fee calculation inverse sanity check when entering 20
000 sats into the SwapDialog. By making self.percentage a decimal we can
prevent this kind of issue.
```
File "/home/user/code/vibecoding_vm/electrum/electrum/gui/qt/swap_dialog.py", line 294, in on_send_edited
recv_amount = self.swap_manager.get_recv_amount(send_amount, is_reverse=self.is_reverse)
File "/home/user/code/vibecoding_vm/electrum/electrum/submarine_swaps.py", line 1320, in get_recv_amount
if abs(send_amount - inverted_send_amount) > 1:
~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
TypeError: unsupported operand type(s) for -: 'int' and 'NoneType'
```
As there are two htlcs, the `alice_htlc_resolved` Event might get set either once or twice by the time `alice_htlc_resolved.wait()` returns. The previous code was assuming that both htlcs are resolved by then, but it could happen that only one htlc was resolved, due to timing.
"When should we reveal preimages onchain?"
This commit tries to simplify the thinking by making the observation:
- we can reveal preimages (actually in any context) if they are already public
- a preimage is public if any other lightning node knows it besides us
- if we learn the preimage from another LN node, it is public
- if we send update_fulfill_htlc, it becomes public
- if we see a preimage onchain, it is public
- in lnsweep._maybe_reveal_preimage_for_htlc:
- partial mpp check is not relevant if preimage is already public
- let's just always do KeepWatchingTXO, for sanity/safety
Co-authored-by: ThomasV <thomasv@electrum.org>
Test that Abstract_Wallet.bump_fee() raises if the given feerate
of the replacement is equal to the feerate of the tx to bump as this
wouldn't be accepted to the mempool.
Adds unittest to check the fee increase when adding outputs to a base
tx. Supposed to prevent creating transactions that don't get accepted
like in this traceback:
```
broadcast_transaction error [DO NOT TRUST THIS MESSAGE]: "RPCError(1, 'the transaction was rejected by network rules.\\n\\ninsufficient fee, rejecting replacement ceeaef5ac7f82286e42ebd530e965fa4c7a6c11933d6b89d6d6f0ee2c69db839; new feerate 0.00001109 BTC/kvB <= old feerate 0.00001110 BTC/kvB
```
I noticed CLN is sending our own channel update to us on
reestablishment, we then assume it to be the remote nodes
update and try to verify the signature against their pubkey
which fails and throws `InvalidGossipMsg`.
This adds a check preventing us from trying to save our own
channel updates as remote update.
Adds unittest for Abstract_Wallet.export_history_to_file that
compares the output against reference files. This
should help to prevent regressions and ensure the layout
of the export stays static over time.
When gossip is enabled we waste a lot of time trying to connect
to onion peers if we don't have a proxy enabled. We should just skip
them and try to connect to clearnet peers instead.
LNPeerManager.add_peer would only check if self.network.proxy is set,
which it is always as Network is initialized with self.proxy =
ProxySettings(). Instead it should check if proxy is set and enabled.
Don't set the NETWORK_AUTO_CONNECT config if the user checked the custom
server config checkbox and then cancels the wizard, so the next time the
user starts the wizard they again will have the choice instead of
silently being forced into a random server.
Currently if the user cancels the wizard during the network config the
welcome component will already have set the NETWORK_AUTO_CONNECT config
to False (as the user selected the custom server checkbox), preventing
the wizard from starting again on the next startup.
This patch changes the CallbackManager to use WeakMethods (weakrefs) to
break the ref cycle and allow the GC to clean up the wallet objects.
unregister_callbacks() will also get called automatically, from
EventListener.__del__, to clean up the CallbackManager.
I also added a few unit tests for this.
fixes https://github.com/spesmilo/electrum/issues/10427
-----
original problem:
In many subclasses of `EventListener`, such as `Abstract_Wallet`, `LNWatcher`,
`LNPeerManager`, we call `register_callbacks()` in `__init__`.
`unregister_callbacks()` is usually called in the `stop()` method.
Example - consider the wallet object:
- `Abstract_Wallet.__init__()` calls `register_callbacks()`
- there is a `start_network()` method
- there is a `stop()` method, which calls `unregister_callbacks()`
- typically the wallet API user only calls `stop()` if they also called
`start_network()`.
This means the callbacks are often left registered, leading to the wallet
objects not getting GC-ed. The GC won't clean them up as
`util.callback_mgr.callbacks` stores strong refs to instance methods
of `Abstract_Wallet`, hence strong refs to the `Abstract_Wallet` objects.
An annoying example is `daemon.check_password_for_directory`, which
potentially creates wallet objects for all wallet files in the datadir.
It simply constructs the wallets, does not call `start_network()` and
neither does it call `stop()`.
- to gracefully take duplicate calls of register_callbacks(): should be idempotent now
- as a side-effect, the order of the callbacks is changed and not guaranteed
- not like anyone should have been relying on it before though
ThomasV