-
Notifications
You must be signed in to change notification settings - Fork 3.4k
backend support for 'close reason' #10578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ class Columns(MyTreeView.BaseColumnsEnum): | |
| LOCAL_BALANCE = enum.auto() | ||
| REMOTE_BALANCE = enum.auto() | ||
| CHANNEL_STATUS = enum.auto() | ||
| CLOSE_REASON = enum.auto() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it does not make sense to add a column for this. It takes a lot of horizontal space and adds noise.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree. I only added because I thought someone might want to search for channels closed with a given reason. But if that's the case, we can fix it later. I will remove. |
||
| LONG_CHANID = enum.auto() | ||
|
|
||
| headers = { | ||
|
|
@@ -51,13 +52,15 @@ class Columns(MyTreeView.BaseColumnsEnum): | |
| Columns.LOCAL_BALANCE: _('Can send'), | ||
| Columns.REMOTE_BALANCE: _('Can receive'), | ||
| Columns.CHANNEL_STATUS: _('Status'), | ||
| Columns.CLOSE_REASON: _('Close reason'), | ||
| } | ||
|
|
||
| filter_columns = [ | ||
| Columns.SHORT_CHANID, | ||
| Columns.LONG_CHANID, | ||
| Columns.NODE_ALIAS, | ||
| Columns.CHANNEL_STATUS, | ||
| Columns.CLOSE_REASON, | ||
| ] | ||
|
|
||
| _default_item_bg_brush = None # type: Optional[QBrush] | ||
|
|
@@ -109,6 +112,7 @@ def format_fields(self, chan: AbstractChannel) -> Dict['ChannelsList.Columns', s | |
| self.Columns.LOCAL_BALANCE: '' if closed else labels[LOCAL], | ||
| self.Columns.REMOTE_BALANCE: '' if closed else labels[REMOTE], | ||
| self.Columns.CHANNEL_STATUS: status, | ||
| self.Columns.CLOSE_REASON: '' if not closed else chan.get_close_reason_for_GUI(), | ||
| } | ||
|
|
||
| def on_channel_closed(self, txid): | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -102,6 +102,13 @@ class PeerState(IntEnum): | |||||
| BAD = 3 | ||||||
|
|
||||||
|
|
||||||
| class ChanCloseReason(IntEnum): | ||||||
| LOCAL_FORCE = 0 # we broadcast our own commitment tx | ||||||
| REMOTE_FORCE = 1 # remote broadcast their commitment tx | ||||||
| LOCAL_COOP = 2 # we initiated the mutual close | ||||||
| REMOTE_COOP = 3 # remote initiated the mutual close | ||||||
|
|
||||||
|
|
||||||
| cs = ChannelState | ||||||
| state_transitions = [ | ||||||
| (cs.PREOPENING, cs.OPENING), | ||||||
|
|
@@ -192,6 +199,21 @@ class AbstractChannel(Logger, ABC): | |||||
| _state: ChannelState | ||||||
| _who_closed: Optional[int] = None # HTLCOwner (1 or -1). 0 means "unknown" | ||||||
|
|
||||||
| def save_close_reason(self, reason: ChanCloseReason) -> None: | ||||||
| self.storage['close_reason'] = reason.name | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would make the names of the enum members part of the wallet db. That is, if someone later renames e.g. IMO it is preferable to use the names and not the int values in the wallet db, so this is fine, but it is completely unintuitive and error-prone for the programmer, so at least a comment must be added. Just duplicate this comment: electrum/electrum/lnchannel.py Lines 77 to 78 in 1235b4a
|
||||||
|
|
||||||
| def get_close_reason(self) -> Optional[ChanCloseReason]: | ||||||
| value = self.storage.get('close_reason') | ||||||
| if value is None: | ||||||
| return None | ||||||
| return ChanCloseReason[value] | ||||||
|
|
||||||
| def get_close_reason_for_GUI(self) -> str: | ||||||
| reason = self.get_close_reason() | ||||||
| if reason is None: | ||||||
| return '' | ||||||
| return reason.name | ||||||
|
|
||||||
| def set_short_channel_id(self, short_id: ShortChannelID) -> None: | ||||||
| self.short_channel_id = short_id | ||||||
| self.storage["short_channel_id"] = short_id | ||||||
|
|
@@ -319,6 +341,7 @@ def get_ctx_sweep_info(self, ctx: Transaction) -> Tuple[bool, Dict[str, MaybeSwe | |||||
| self.logger.info(f'we (local) force closed') | ||||||
| elif who_closed == REMOTE: | ||||||
| self.logger.info(f'they (remote) force closed.') | ||||||
| self.save_close_reason(ChanCloseReason.REMOTE_FORCE) | ||||||
|
Comment on lines
343
to
+348
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should also set the close reason in the LOCAL case here. We could keep the other call in |
||||||
| else: | ||||||
| self.logger.info(f'not sure who closed. maybe co-op close?') | ||||||
| is_local_ctx = who_closed == LOCAL | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ | |
| from electrum.lntransport import LNPeerAddr | ||
| from electrum.crypto import privkey_to_pubkey | ||
| from electrum.lnutil import Keypair, PaymentFailure, LnFeatures, HTLCOwner, PaymentFeeBudget, RECEIVED | ||
| from electrum.lnchannel import ChannelState, PeerState, Channel | ||
| from electrum.lnchannel import ChannelState, ChanCloseReason, PeerState, Channel | ||
| from electrum.lnrouter import LNPathFinder, PathEdge, LNPathInconsistent | ||
| from electrum.channel_db import ChannelDB, InvalidGossipMsg | ||
| from electrum.lnworker import LNWallet, NoPathFound, SentHtlcInfo, PaySession, LNPeerManager | ||
|
|
@@ -1617,6 +1617,7 @@ async def pay(): | |
| await p2.received_commitsig_event.wait() | ||
| # alice closes | ||
| await p1.close_channel(alice_channel.channel_id) | ||
| self.assertEqual(alice_channel.get_close_reason(), ChanCloseReason.LOCAL_COOP) | ||
| gath.cancel() | ||
| async def set_settle(): | ||
| await asyncio.sleep(0.1) | ||
|
|
@@ -1625,6 +1626,29 @@ async def set_settle(): | |
| with self.assertRaises(asyncio.CancelledError): | ||
| await gath | ||
|
|
||
| async def test_close_reason(self): | ||
| # Verify ChanCloseReason is set correctly on both sides for each close type. | ||
| graph = self.prepare_chans_and_peers_in_graph(self.GRAPH_DEFINITIONS['single_chan']) | ||
| p1, p2 = graph.peers.values() | ||
| w1, w2 = graph.workers.values() | ||
| alice_channel, bob_channel = graph.channels.values() | ||
| w1.network.config.TEST_SHUTDOWN_FEE = 100 | ||
| w2.network.config.TEST_SHUTDOWN_FEE = 100 | ||
| w1.network.config.TEST_SHUTDOWN_LEGACY = True | ||
| w2.network.config.TEST_SHUTDOWN_LEGACY = True | ||
| async def action(): | ||
| await util.wait_for2(p1.initialized, 1) | ||
| await util.wait_for2(p2.initialized, 1) | ||
| await p1.close_channel(alice_channel.channel_id) | ||
| # alice's side is completed, but bob need to wait to see the reason. | ||
| await asyncio.sleep(1) # FIXME: use a better wait | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On my machine, the test passes even with this sleep completely removed. Is that not the case for you? Anyway, the simplest less-wasteful thing that could be done is: async with util.async_timeout(1):
while not bob_channel.is_closed_or_closing():
await asyncio.sleep(0.01)Something more complicated: diff --git a/tests/test_lnpeer.py b/tests/test_lnpeer.py
index a4950ba9d5..b65f78a041 100644
--- a/tests/test_lnpeer.py
+++ b/tests/test_lnpeer.py
@@ -1636,12 +1636,21 @@ class TestPeerDirect(TestPeer):
w2.network.config.TEST_SHUTDOWN_FEE = 100
w1.network.config.TEST_SHUTDOWN_LEGACY = True
w2.network.config.TEST_SHUTDOWN_LEGACY = True
+
+ any_chan_changed = asyncio.Event()
+ async def on_chan_changed(*args):
+ any_chan_changed.set()
+ any_chan_changed.clear()
+ util.register_callback(on_chan_changed, ["channel"])
+
async def action():
await util.wait_for2(p1.initialized, 1)
await util.wait_for2(p2.initialized, 1)
await p1.close_channel(alice_channel.channel_id)
# alice's side is completed, but bob need to wait to see the reason.
- await asyncio.sleep(1) # FIXME: use a better wait
+ async with util.async_timeout(1):
+ while not bob_channel.is_closed_or_closing():
+ await any_chan_changed.wait()
self.assertEqual(alice_channel.get_close_reason(), ChanCloseReason.LOCAL_COOP)
self.assertEqual(bob_channel.get_close_reason(), ChanCloseReason.REMOTE_COOP)
gath.cancel()
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed it does, but that's will not be necessarily true for all runs. |
||
| self.assertEqual(alice_channel.get_close_reason(), ChanCloseReason.LOCAL_COOP) | ||
| self.assertEqual(bob_channel.get_close_reason(), ChanCloseReason.REMOTE_COOP) | ||
| gath.cancel() | ||
| gath = asyncio.gather(action(), p1._message_loop(), p2._message_loop(), p1.htlc_switch(), p2.htlc_switch()) | ||
| with self.assertRaises(asyncio.CancelledError): | ||
| await gath | ||
|
|
||
| async def test_warning(self): | ||
| graph = self.prepare_chans_and_peers_in_graph(self.GRAPH_DEFINITIONS['single_chan']) | ||
| p1, p2 = graph.peers.values() | ||
|
|
@@ -1745,6 +1769,7 @@ async def test_channel_usage_after_closing(self): | |
| await w1.force_close_channel(alice_channel.channel_id) | ||
| # check if a tx (commitment transaction) was broadcasted: | ||
| assert w1.network.tx_queue.qsize() == 1 | ||
| self.assertEqual(alice_channel.get_close_reason(), ChanCloseReason.LOCAL_FORCE) | ||
|
|
||
| with self.assertRaises(NoPathFound) as e: | ||
| await w1.create_routes_from_invoice(lnaddr.get_amount_msat(), decoded_invoice=lnaddr) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: this would make the names of the enum members part of the external API
also see other comment re wallet db upgrades
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re the comment
# Harder to read, but keeps it within a single line.-- ok, but the comment itself is not needed IMO