lightning: fix anchor channel backup - #10852
Conversation
da57099 to
bf0f96d
Compare
| static_remotekey = kwargs.pop('static_remotekey') | ||
| static_payment_key = kwargs.pop('static_payment_key') | ||
| channel_type = kwargs.pop('channel_type', None) | ||
| payment_basepoint = kwargs.pop('payment_basepoint', None) |
There was a problem hiding this comment.
can we add:
assert (static_remotekey is not None) + (static_payment_key is not None) + (payment_basepoint is not None) == 1Also, in the last else branch, "ending up here likely indicates an issue", just raise an exception.
There was a problem hiding this comment.
Added an assert in https://github.com/spesmilo/electrum/compare/bf0f96dcc0452397cecaf2d52355d799c57ae8c8..a8a5b2d2ad1c2defdc98adf374a3983ae57c8dfc
But we can't assert == 1 or raise at the else clause as v0 channel backups contain none of the fields.
The LocalConfig.from_seed() method is a bit confusing in general, IIUC this else clause was useful for pre-srk channels (for which channel backups never got exposed) and then wasn't removed later on.
For v0 backups / srk channels the derivation is basically nonsense?
So I added a commit which makes this explicit instead of misleading the reader: 8d9544c
| csv_delay = 5 # demanded of alice by bob | ||
| self.config.LIGHTNING_TO_SELF_DELAY_CSV = csv_delay | ||
| alice = self.create_non_deterministic_xprv_wallet("alice") | ||
| await self.pay_to_address(alice.get_receiving_address(), 1 * COIN) | ||
| await self.mine_blocks(1) | ||
|
|
||
| chan = await self.open_channel(alice, self.bob) | ||
| self.assertTrue(chan.has_anchors()) | ||
| self.assertEqual(csv_delay, chan.config[REMOTE].to_self_delay) |
There was a problem hiding this comment.
This should not work.
self.config is Alice's config, and setting config.LIGHTNING_TO_SELF_DELAY_CSV = 5 means Alice will impose a 5 lock CSV on Bob when Bob force-closes.
But in this case Alice will force-close. So the timelock is chosen by Bob. So how come the rest of the test works if Bob is supposed to be choosing the default of 1008 blocks but after Alice force-closes and we wait only 5 blocks and then Alice can sweep?
Well it's because Alice and Bob share the ToyNetwork, which has a config object, so they sort-of share a config object. (well Alice has a single config object, and Bob has his own but also has a reference to Alice's through the network object... confusing)
This assert should not fail ideally:
diff --git a/electrum/lnworker.py b/electrum/lnworker.py
index 58cc77c330..3f4e6b8876 100644
--- a/electrum/lnworker.py
+++ b/electrum/lnworker.py
@@ -424,6 +424,7 @@ class LNPeerManager(Logger, EventListener, NetworkRetryManager[LNPeerAddr]):
assert network
assert self.network is None, "already started"
self.network = network
+ assert network.config is self.config
self._add_peers_from_config()
asyncio.run_coroutine_threadsafe(self.main_loop(), get_asyncio_loop())
if listen:There was a problem hiding this comment.
Right, good catch, this is confusing. I changed the ToyServerTestCase in 1362998 so it now handles separate instances (similar to a separate Electrum process) with their own configs (see ToyInstance). This separates the owners of the config transparently.
useful for unit tests where it is easy to slip-up and break this invariant when creating a MockNetwork ref #10852 (comment)
Call `.exception()` on the channel reestablish future after we set an exception on it so asyncio marks the exception retrieved and doesn't pollute the log output with huge "exception was never retrieved" tracebacks during unittests.
Extends the channel backup format by including the payment_basepoint so that anchor channel to_remote outputs can be claimed even without deterministic wallet seeds.
When importing a v0 channel backup `LocalConfig.from_seed()` would derive a `payment_basepoint` from the `channel_seed`. This `payment_basepoint` is not useful for static remote key channels, so the derivation is only confusing. Pre-srk channels, the only channel type that could use this derivation, never exported channel backups.
Construct a ChannelBackup in the lnutil tests, this is cheap and catches issues where the `ChannelBackup.from_seed()` call would raise.
a8a5b2d to
1b11674
Compare
|
@SomberNight as discussed I also added more unittests in c649d51, some might overlap a bit, e.g. the local force close tests are taking a pretty similar path to claim the |
Now we have unittests, so this shouldn't be important anymore
now that we have channel_type available in the channel backup, the `ChannelBackup.has_anchors()` method can utilize it.
Don't allow requesting a remote force close if we cannot sweep the to-remote output by not showing the option to request the force close in the UI.
Show a warning in the UI when trying to import a channel backup that cannot be used to properly request a force close.
Implement a mechanism to show one-time warnings to the user on startup of the wallet to inform them about critical changes. Use this mechanism to inform them about the changed lightning channel backup scheme.
2 participants
Add missing information to lightning channel backup so users of non-deterministic
lightning wallets trying to recover anchor channels are able to sweep the to_remote
output of a remote ctx.
Also shows a warning to affected users, urging them to export a new backup.
Fixes #10785