lightning: support error and warning messages#7645
Conversation
| # channel_id of zero means that the error refers to all channels | ||
| if channel_id == bytes(32): | ||
| for channel_id in self.channels: | ||
| await self.lnworker.force_close_channel(channel_id) |
There was a problem hiding this comment.
shouldn't this also be guarded with force_close_channel?
There was a problem hiding this comment.
I have channel force closing in both scenarios. The case with all-zero bits is a bit dangerous.
There was a problem hiding this comment.
Sorry, I'm not sure I understand.
My question is this: What is the expected behaviour of calling peer.send_error(bytes(32), force_close_channel=False)?
Right now, AFAICT, it would fclose the channels.
There was a problem hiding this comment.
Ah sorry, I misunderstood, it is guarded now. The spec says we must force close upon sending an error (individual or all channels), but I would keep the force_close_channel parameter nevertheless.
There was a problem hiding this comment.
Another question is if we really should allow force closing of all channels if we receive the zero channel_id, it looks dangerous.
There was a problem hiding this comment.
The BOLT says we must fail channels with the sending node. If the sending node wants all their channels closed, there is not much we can do... Are you concerned about malicious behaviour by the remote, or programming bugs in the remote's implementation, or something else?
Re malicious behaviour, I was thinking we might introduce Peer.force_close_channel and Peer.try_force_closing as wrappers for LNWorker.force_close_channel and LNWorker.try_force_closing, that would enforce that the channel_id corresponds to a channel with this Peer. This does not have to be a part of this PR though.
There was a problem hiding this comment.
The BOLT says we must fail channels with the sending node. If the sending node wants all their channels closed, there is not much we can do... Are you concerned about malicious behaviour by the remote, or programming bugs in the remote's implementation, or something else?
Yes, mostly about something that could trigger another node sending the zero message (or all nodes in the network across multiple implementations :P).
Re malicious behaviour, I was thinking we might introduce Peer.force_close_channel and Peer.try_force_closing as wrappers for LNWorker.force_close_channel and LNWorker.try_force_closing, that would enforce that the channel_id corresponds to a channel with this Peer. This does not have to be a part of this PR though.
Sounds useful, added the wrapper and used it instead.
There was a problem hiding this comment.
Should we also loop over the temporary channel ids as well? Oh and needs a rebase...
bitromortac
force-pushed
the
2112-warning-messages
branch
from
e94e44b to
67fcf85
Compare
bitromortac
force-pushed
the
2112-warning-messages
branch
2 times, most recently
from
6338701 to
120f71e
Compare
bitromortac
force-pushed
the
2112-warning-messages
branch
3 times, most recently
from
3cce707 to
257d1c6
Compare
bitromortac
force-pushed
the
2112-warning-messages
branch
2 times, most recently
from
4f84c37 to
3a4b977
Compare
* adds methods for sending protocol errors/warnings * handling of warning messages
The exceptions are meant to be raised in places where the BOLTs require the sending of warning or error messages. They are necessary to handle protocol failures occuring helper functions that check constraints.
bitromortac
force-pushed
the
2112-warning-messages
branch
from
3a4b977 to
e67070c
Compare
|
Please note that before this PR, an exception is raised in |
1 similar comment
|
Please note that before this PR, an exception is raised in |
- rm the `_get_channel_ids` abstraction as each of its usages needs subtle differences. Some code duplication is preferable in this case. - raise exceptions in `wait_for_message`, so that callers such as the GUI can show user-feedback - on_error/on_warning were dropping messages with temp_chan_ids if they were not stored in `temp_id_to_id` - which was only done once the mapping was known (so the normal chan_id was known). To fix this, we now store temp_chan_ids into `temp_id_to_id` early. related: spesmilo#7645 (and related commits) ----- example before commit: ``` D/P | lnpeer.Peer.[LNWallet, 03933884aa-3b53e4ab] | Sending OPEN_CHANNEL D/P | lnpeer.Peer.[LNWallet, 03933884aa-3b53e4ab] | Received ERROR I/P | lnpeer.Peer.[LNWallet, 03933884aa-3b53e4ab] | remote peer sent error [DO NOT TRUST THIS MESSAGE]: invalid funding_satoshis=10000 sat (min=400000 sat max=1500000000 sat) E | gui.qt.main_window.[test_segwit_2] | Could not open channel Traceback (most recent call last): File "...\electrum\electrum\util.py", line 1160, in wrapper return await func(*args, **kwargs) File "...\electrum\electrum\lnpeer.py", line 661, in wrapper return await func(self, *args, **kwargs) File "...\electrum\electrum\lnpeer.py", line 742, in channel_establishment_flow payload = await self.wait_for_message('accept_channel', temp_channel_id) # File "...\electrum\electrum\lnpeer.py", line 315, in wait_for_message name, payload = await asyncio.wait_for(q.get(), LN_P2P_NETWORK_TIMEOUT) File "...\Python39\lib\asyncio\tasks.py", line 468, in wait_for await waiter asyncio.exceptions.CancelledError During handling of the above exception, another exception occurred: Traceback (most recent call last): File "...\Python39\lib\asyncio\tasks.py", line 492, in wait_for fut.result() asyncio.exceptions.CancelledError The above exception was the direct cause of the following exception: Traceback (most recent call last): File "...\electrum\electrum\gui\qt\util.py", line 914, in run result = task.task() File "...\electrum\electrum\gui\qt\main_window.py", line 1875, in task return self.wallet.lnworker.open_channel( File "...\electrum\electrum\lnworker.py", line 1075, in open_channel chan, funding_tx = fut.result() File "...\Python39\lib\concurrent\futures\_base.py", line 445, in result return self.__get_result() File "...\Python39\lib\concurrent\futures\_base.py", line 390, in __get_result raise self._exception File "...\electrum\electrum\util.py", line 1160, in wrapper return await func(*args, **kwargs) File "...\electrum\electrum\lnworker.py", line 1006, in _open_channel_coroutine chan, funding_tx = await asyncio.wait_for(coro, LN_P2P_NETWORK_TIMEOUT) File "...\Python39\lib\asyncio\tasks.py", line 494, in wait_for raise exceptions.TimeoutError() from exc asyncio.exceptions.TimeoutError ``` example after commit: ``` D/P | lnpeer.Peer.[LNWallet, 03933884aa-ff3a866f] | Sending OPEN_CHANNEL D/P | lnpeer.Peer.[LNWallet, 03933884aa-ff3a866f] | Received ERROR I/P | lnpeer.Peer.[LNWallet, 03933884aa-ff3a866f] | remote peer sent error [DO NOT TRUST THIS MESSAGE]: invalid funding_satoshis=10000 sat (min=400000 sat max=1500000000 sat). chan_id=124ca21fa6aa2993430ad71f465f0d44731ef87f7478e4b31327e4459b5a3988 E | lnworker.LNWallet.[test_segwit_2] | Exception in _open_channel_coroutine: GracefulDisconnect('remote peer sent error [DO NOT TRUST THIS MESSAGE]: invalid funding_satoshis=10000 sat (min=400000 sat max=1500000000 sat)') Traceback (most recent call last): File "...\electrum\electrum\util.py", line 1160, in wrapper return await func(*args, **kwargs) File "...\electrum\electrum\lnworker.py", line 1006, in _open_channel_coroutine chan, funding_tx = await asyncio.wait_for(coro, LN_P2P_NETWORK_TIMEOUT) File "...\Python39\lib\asyncio\tasks.py", line 481, in wait_for return fut.result() File "...\electrum\electrum\lnpeer.py", line 673, in wrapper return await func(self, *args, **kwargs) File "...\electrum\electrum\lnpeer.py", line 755, in channel_establishment_flow payload = await self.wait_for_message('accept_channel', temp_channel_id) File "...\electrum\electrum\lnpeer.py", line 326, in wait_for_message raise GracefulDisconnect( electrum.interface.GracefulDisconnect: remote peer sent error [DO NOT TRUST THIS MESSAGE]: invalid funding_satoshis=10000 sat (min=400000 sat max=1500000000 sat) I/P | lnpeer.Peer.[LNWallet, 03933884aa-ff3a866f] | Disconnecting: GracefulDisconnect() ```
- rm the `_get_channel_ids` abstraction as each of its usages needs subtle differences. Some code duplication is preferable in this case. - raise exceptions in `wait_for_message`, so that callers such as the GUI can show user-feedback - on_error/on_warning were dropping messages with temp_chan_ids if they were not stored in `temp_id_to_id` - which was only done once the mapping was known (so the normal chan_id was known). To fix this, we now store temp_chan_ids into `temp_id_to_id` early. - `schedule_force_closing` only works if the chan_id is already in `channels` related: spesmilo#7645 (and related commits) ----- example before commit: ``` D/P | lnpeer.Peer.[LNWallet, 03933884aa-3b53e4ab] | Sending OPEN_CHANNEL D/P | lnpeer.Peer.[LNWallet, 03933884aa-3b53e4ab] | Received ERROR I/P | lnpeer.Peer.[LNWallet, 03933884aa-3b53e4ab] | remote peer sent error [DO NOT TRUST THIS MESSAGE]: invalid funding_satoshis=10000 sat (min=400000 sat max=1500000000 sat) E | gui.qt.main_window.[test_segwit_2] | Could not open channel Traceback (most recent call last): File "...\electrum\electrum\util.py", line 1160, in wrapper return await func(*args, **kwargs) File "...\electrum\electrum\lnpeer.py", line 661, in wrapper return await func(self, *args, **kwargs) File "...\electrum\electrum\lnpeer.py", line 742, in channel_establishment_flow payload = await self.wait_for_message('accept_channel', temp_channel_id) # File "...\electrum\electrum\lnpeer.py", line 315, in wait_for_message name, payload = await asyncio.wait_for(q.get(), LN_P2P_NETWORK_TIMEOUT) File "...\Python39\lib\asyncio\tasks.py", line 468, in wait_for await waiter asyncio.exceptions.CancelledError During handling of the above exception, another exception occurred: Traceback (most recent call last): File "...\Python39\lib\asyncio\tasks.py", line 492, in wait_for fut.result() asyncio.exceptions.CancelledError The above exception was the direct cause of the following exception: Traceback (most recent call last): File "...\electrum\electrum\gui\qt\util.py", line 914, in run result = task.task() File "...\electrum\electrum\gui\qt\main_window.py", line 1875, in task return self.wallet.lnworker.open_channel( File "...\electrum\electrum\lnworker.py", line 1075, in open_channel chan, funding_tx = fut.result() File "...\Python39\lib\concurrent\futures\_base.py", line 445, in result return self.__get_result() File "...\Python39\lib\concurrent\futures\_base.py", line 390, in __get_result raise self._exception File "...\electrum\electrum\util.py", line 1160, in wrapper return await func(*args, **kwargs) File "...\electrum\electrum\lnworker.py", line 1006, in _open_channel_coroutine chan, funding_tx = await asyncio.wait_for(coro, LN_P2P_NETWORK_TIMEOUT) File "...\Python39\lib\asyncio\tasks.py", line 494, in wait_for raise exceptions.TimeoutError() from exc asyncio.exceptions.TimeoutError ``` example after commit: ``` D/P | lnpeer.Peer.[LNWallet, 03933884aa-ff3a866f] | Sending OPEN_CHANNEL D/P | lnpeer.Peer.[LNWallet, 03933884aa-ff3a866f] | Received ERROR I/P | lnpeer.Peer.[LNWallet, 03933884aa-ff3a866f] | remote peer sent error [DO NOT TRUST THIS MESSAGE]: invalid funding_satoshis=10000 sat (min=400000 sat max=1500000000 sat). chan_id=124ca21fa6aa2993430ad71f465f0d44731ef87f7478e4b31327e4459b5a3988 E | lnworker.LNWallet.[test_segwit_2] | Exception in _open_channel_coroutine: GracefulDisconnect('remote peer sent error [DO NOT TRUST THIS MESSAGE]: invalid funding_satoshis=10000 sat (min=400000 sat max=1500000000 sat)') Traceback (most recent call last): File "...\electrum\electrum\util.py", line 1160, in wrapper return await func(*args, **kwargs) File "...\electrum\electrum\lnworker.py", line 1006, in _open_channel_coroutine chan, funding_tx = await asyncio.wait_for(coro, LN_P2P_NETWORK_TIMEOUT) File "...\Python39\lib\asyncio\tasks.py", line 481, in wait_for return fut.result() File "...\electrum\electrum\lnpeer.py", line 673, in wrapper return await func(self, *args, **kwargs) File "...\electrum\electrum\lnpeer.py", line 755, in channel_establishment_flow payload = await self.wait_for_message('accept_channel', temp_channel_id) File "...\electrum\electrum\lnpeer.py", line 326, in wait_for_message raise GracefulDisconnect( electrum.interface.GracefulDisconnect: remote peer sent error [DO NOT TRUST THIS MESSAGE]: invalid funding_satoshis=10000 sat (min=400000 sat max=1500000000 sat) I/P | lnpeer.Peer.[LNWallet, 03933884aa-ff3a866f] | Disconnecting: GracefulDisconnect() ```
3 participants
Partially takes care about #7624.
Adds sending of error and warning messages as well as handling of warning messages.
Currently in the codebase ~anytime we encounter errors we just raise exceptions, but most of the time we should send errors (and force close) or send a warning and close the connection. How this could be done is to directly call
send_errororsend_warningor, if some violation in a helper function happens, one could raiseLNProtocolError/LNProtocolWarningand convert them to the messages in the outer scope. In principle, one should go through the entire spec and handle those failures, however, because I wanted to first get feedback about the API, I only included handling of theopen_channelandshutdownmessages as examples. When converting exceptions one should be careful not to send secret data.