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
ec65c53 replaces the usage of `PeerInTest` with `Peer` in
test_lnpeer.py.
PeerInTests sets `Peer.DELAY_INC_MSG_PROCESSING_SLEEP` to 0 so all
incoming messages get processed immediately. Because `Peer` instead of
`TestInPeer` was used the delay caused `test_reestablish_with_old_state`
to fail regularly because bob receives the old channel state and kills
the OldTaskGroup of the unittest with GracefulDisconnect before Alice
processed the answer of Bob and is still in ChannelState.REESTABLISHING.
```
FAILED tests/test_lnpeer.py::TestPeerDirect::test_reestablish_with_old_state - AssertionError: <PeerState.REESTABLISHING: 1> != <PeerState.BAD: 3>
```
- LNWallet no longer "is-an" LNWorker, instead LNWallet "has-an" LNWorker
- the motivation is to make the unit tests nicer, and allow writing unit tests for more things
- I hope this makes it possible to e.g. test lnsweep in the unit tests
- some stuff we would previously have to write a regtest for, maybe we can write a unit test for, now
- in unit tests, MockLNWallet now
- inherits LNWallet
- the Wallet is no longer being mocked
Lightning addresses with 'lightning:' do occur in the wild and make
sense (how else would e.g. the smartphone know to open a lightning
wallet instead of the e-mail client). So we should allow this.
When creating a new wallet in a Electrum instance with existing wallets
this change forces the user to reuse a password of any existing wallet
if `SimpleConfig.WALLET_USE_SINGLE_PASSWORD` is True.
This prevents the amount of different passwords from increasing and
guides the user towards a single wallet password (the intended default).
The new test
TestPeerForwarding::test_dont_settle_htlcs_receiver_and_forwarder covers
the receiver as well as the forwarder. So this unittest becomes obsolete
as it only tests the receiver.
Oren