Merge pull request #10888 from spesmilo/lnchannel_funding_height_downgrade

### electrum/lnchannel.py
@@ -221,6 +221,7 @@ def get_state(self) -> ChannelState:
 
     def is_funded(self) -> bool:
         # NOTE: also true for unfunded zeroconf channels (OPEN > FUNDED)
+        #     - also true for unfunded channel in ChannnelState.FORCE_CLOSING
         return self.get_state() >= ChannelState.FUNDED
 
     def is_open(self) -> bool:
@@ -262,24 +263,25 @@ def need_to_subscribe(self) -> bool:
     def get_close_options(self) -> Sequence[ChanCloseOption]:
         pass
 
-    def save_funding_height(self, *, txid: str, height: int, timestamp: Optional[int]) -> None:
-        self.storage['funding_height'] = txid, height, timestamp
+    def _maybe_save_tx_height(self, *, name, txid: str, info: TxMinedInfo) -> None:
+        height = info.height()
+        h = self.storage.get(name)
+        prev_height = h[1] if h else TX_HEIGHT_LOCAL
+        if prev_height > 0 and height <= 0:
+            # do not revert from confirmed to unconfirmed
+            return
+        if prev_height <= 0 and height == TX_HEIGHT_LOCAL:
+            # clear unconfirmed transaction evicted from mempool
+            self.storage.pop(name, None)
+            return
+        self.storage[name] = txid, height, info.timestamp
 
     def get_funding_height(self) -> Optional[Tuple[str, int, Optional[int]]]:
         return self.storage.get('funding_height')
 
-    def delete_funding_height(self):
-        self.storage.pop('funding_height', None)
-
-    def save_closing_height(self, *, txid: str, height: int, timestamp: Optional[int]) -> None:
-        self.storage['closing_height'] = txid, height, timestamp
-
     def get_closing_height(self) -> Optional[Tuple[str, int, Optional[int]]]:
         return self.storage.get('closing_height')
 
-    def delete_closing_height(self):
-        self.storage.pop('closing_height', None)
-
     def create_sweeptxs_for_our_ctx(self, ctx: Transaction) -> Dict[str, MaybeSweepInfo]:
         return sweep_our_ctx(chan=self, ctx=ctx)
 
@@ -329,8 +331,12 @@ def extract_preimage_from_htlc_txin(self, txin: TxInput, *, is_deeply_mined: boo
 
     def update_onchain_state(self, *, funding_txid: str, funding_height: TxMinedInfo,
                              closing_txid: str, closing_height: TxMinedInfo, keep_watching: bool) -> None:
-        # note: state transitions are irreversible, but
-        # save_funding_height, save_closing_height are reversible
+
+        # first save funding and closing height
+        self._maybe_save_tx_height(name='funding_height', txid=funding_txid, info=funding_height)
+        self._maybe_save_tx_height(name='closing_height', txid=closing_txid, info=closing_height)
+
+        # update state. funding/closing tx may be unconfirmed
         if funding_height.height() == TX_HEIGHT_LOCAL:
             self.update_unfunded_state()
         elif closing_height.height() == TX_HEIGHT_LOCAL:
@@ -346,8 +352,6 @@ def update_onchain_state(self, *, funding_txid: str, funding_height: TxMinedInfo
                 keep_watching=keep_watching)
 
     def update_unfunded_state(self) -> None:
-        self.delete_funding_height()
-        self.delete_closing_height()
         state = self.get_state()
         if state in [ChannelState.PREOPENING, ChannelState.OPENING, ChannelState.FORCE_CLOSING]:
             if self.is_initiator():
@@ -369,7 +373,7 @@ def update_unfunded_state(self) -> None:
                             self.set_state(ChannelState.REDEEMED)
                             break
             elif self.has_funding_timed_out():
-                self.logger.warning(f"dropping incoming channel, funding tx not found in mempool")
+                self.logger.warning(f"dropping incoming channel, funding tx taking too long to reach req num conf")
                 self.lnworker.remove_channel(self.channel_id)
         elif self.is_zeroconf() and state in [ChannelState.OPEN, ChannelState.CLOSING, ChannelState.FORCE_CLOSING]:
             # handling zeroconf channels with no funding tx, can happen if broadcasting fails on LSP side
@@ -401,8 +405,6 @@ def update_unfunded_state(self) -> None:
                         f"JIT provider: {self.lnworker.config.ZEROCONF_TRUSTED_NODE} or he didn't use our preimage")
 
     def update_funded_state(self, *, funding_txid: str, funding_height: TxMinedInfo) -> None:
-        self.save_funding_height(txid=funding_txid, height=funding_height.height(), timestamp=funding_height.timestamp)
-        self.delete_closing_height()
         if funding_height.conf>0:
             self.set_short_channel_id(ShortChannelID.from_components(
                 funding_height.height(), funding_height.txpos, self.funding_outpoint.output_index))
@@ -430,8 +432,6 @@ def update_funded_state(self, *, funding_txid: str, funding_height: TxMinedInfo)
 
     def update_closed_state(self, *, funding_txid: str, funding_height: TxMinedInfo,
                             closing_txid: str, closing_height: TxMinedInfo, keep_watching: bool) -> None:
-        self.save_funding_height(txid=funding_txid, height=funding_height.height(), timestamp=funding_height.timestamp)
-        self.save_closing_height(txid=closing_txid, height=closing_height.height(), timestamp=closing_height.timestamp)
         if funding_height.conf>0:
             self.set_short_channel_id(ShortChannelID.from_components(
                 funding_height.height(), funding_height.txpos, self.funding_outpoint.output_index))
@@ -851,8 +851,10 @@ def can_be_deleted(self) -> bool:
         return self.is_redeemed()
 
     def has_funding_timed_out(self):
-        funding_height = self.get_funding_height()
-        if self.is_initiator() or funding_height and funding_height[1] > TX_HEIGHT_UNCONFIRMED:
+        if self.is_initiator():
+            return False
+        # remote is the initiator/funder.
+        if self.is_funded() and not self.is_zeroconf():
             return False
         if self.lnworker.network.blockchain().is_tip_stale() or not self.lnworker.wallet.is_up_to_date():
             return False

### tests/test_lnchannel.py
@@ -42,6 +42,8 @@
 )
 from electrum.logging import console_stderr_handler
 from electrum.lnchannel import ChannelState, Channel
+from electrum.util import TxMinedInfo
+from electrum.address_synchronizer import TX_HEIGHT_LOCAL
 from electrum.lnsweep import SweepInfo
 from electrum.transaction import PartialTransaction, PartialTxOutput, Transaction, TxInput, tx_from_any
 
@@ -582,6 +584,162 @@ def test_unfunded_channel_can_be_removed(self):
         self.alice_channel._state = ChannelState.OPENING
         self.assertFalse(self.alice_channel.can_be_deleted())
 
+
+    def test_funded_channel_cannot_be_removed_by_lying_server(self):
+        """
+        Test that a malicious server cannot get a funded channel removed by claiming
+        that the funding tx, which we have verified to be mined, is unconfirmed again.
+        """
+        self.current_height = 800_000
+        chan = self.bob_channel  # non-initiator, so it can time out
+        chan.storage['init_height'] = self.current_height
+        chan.storage['init_timestamp'] = int(time.time())
+
+        mock_lnworker = mock.Mock()
+        mock_blockchain = mock.Mock()
+        mock_lnworker.wallet = mock.Mock()
+        mock_lnworker.wallet.is_up_to_date = lambda: True
+        mock_blockchain.is_tip_stale = lambda: False
+        mock_lnworker.network.blockchain = lambda: mock_blockchain
+        mock_lnworker.network.get_local_height = lambda: self.current_height
+        chan.lnworker = mock_lnworker
+        chan.is_funding_tx_mined = lambda funding_height: (
+            funding_height.conf >= chan.funding_txn_minimum_depth())
+
+        # we start in the OPENING state
+        chan.set_state(ChannelState.OPENING, force=True)
+        self.assertFalse(chan.is_initiator())
+        self.assertFalse(chan.can_be_deleted())
+        self.assertFalse(chan.is_funded())
+
+        # the funding tx gets mined deep enough
+        funding_txid = chan.funding_outpoint.txid
+        funding_timestamp = chan.storage['init_timestamp']
+        self.current_height += chan.funding_txn_minimum_depth()
+        funding_confirmed_height = self.current_height
+        chan.update_onchain_state(
+            funding_txid=funding_txid,
+            funding_height=TxMinedInfo(_height=funding_confirmed_height, conf=chan.funding_txn_minimum_depth(), timestamp=funding_timestamp, txpos=1),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
+        self.assertTrue(chan.is_funded())
+        self.assertFalse(chan.can_be_deleted())
+        self.assertEqual((funding_txid, funding_confirmed_height, funding_timestamp), chan.get_funding_height())
+
+        # the channel is now older than the funding timeout
+        self.current_height += lnutil.CHANNEL_OPENING_TIMEOUT_BLOCKS + 1
+        chan.storage['init_timestamp'] -= CHANNEL_OPENING_TIMEOUT_SEC + 1
+
+        # the server claims the funding tx is unconfirmed again
+        chan.update_onchain_state(
+            funding_txid=funding_txid,
+            funding_height=TxMinedInfo(_height=0, conf=0),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
+
+        # the saved funding height must not be overwritten, and the channel must not be removed
+        self.assertEqual((funding_txid, funding_confirmed_height, funding_timestamp), chan.get_funding_height())
+        self.assertTrue(chan.is_funded())
+        self.assertFalse(chan.has_funding_timed_out())
+        self.assertFalse(chan.can_be_deleted())
+        mock_lnworker.remove_channel.assert_not_called()
+
+        # the server now omits the funding tx entirely, so that we forget the saved height
+        chan.update_onchain_state(
+            funding_txid=None,
+            funding_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
+
+        # the saved height must have survived, and the channel must not be removed
+        self.assertEqual((funding_txid, funding_confirmed_height, funding_timestamp), chan.get_funding_height())
+        self.assertTrue(chan.is_funded())
+        self.assertFalse(chan.has_funding_timed_out())
+        self.assertFalse(chan.can_be_deleted())
+        mock_lnworker.remove_channel.assert_not_called()
+
+    def test_incoming_funded_channel_can_timeout_even_if_it_was_mined_at_some_point_but_not_deeply(self):
+        """The funding tx gets 1 conf (but fewer than funding_txn_minimum_depth)
+        and then gets reorged out and never mined again.
+        If we are not the funder, after sufficient time we should be able to delete the chan.
+        """
+        self.current_height = 800_000
+        chan = self.bob_channel  # non-initiator, so it can time out
+        chan.storage['init_height'] = self.current_height
+        chan.storage['init_timestamp'] = int(time.time())
+
+        mock_lnworker = mock.Mock()
+        mock_blockchain = mock.Mock()
+        mock_lnworker.wallet = mock.Mock()
+        mock_lnworker.wallet.is_up_to_date = lambda: True
+        mock_blockchain.is_tip_stale = lambda: False
+        mock_lnworker.network.blockchain = lambda: mock_blockchain
+        mock_lnworker.network.get_local_height = lambda: self.current_height
+        chan.lnworker = mock_lnworker
+        chan.is_funding_tx_mined = lambda funding_height: (
+            funding_height.conf >= chan.funding_txn_minimum_depth())
+
+        # we start in the OPENING state
+        chan.set_state(ChannelState.OPENING, force=True)
+        self.assertFalse(chan.is_initiator())
+        self.assertFalse(chan.can_be_deleted())
+        self.assertFalse(chan.is_funded())
+
+        # the funding tx gets mined but only 1 conf
+        funding_txid = chan.funding_outpoint.txid
+        funding_timestamp = chan.storage['init_timestamp']
+        self.current_height += 1
+        assert 1 < chan.funding_txn_minimum_depth()
+        funding_confirmed_height = self.current_height
+        chan.update_onchain_state(
+            funding_txid=funding_txid,
+            funding_height=TxMinedInfo(_height=funding_confirmed_height, conf=1, timestamp=funding_timestamp, txpos=1),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
+        self.assertFalse(chan.is_funded())
+        self.assertFalse(chan.can_be_deleted())
+        self.assertEqual((funding_txid, funding_confirmed_height, funding_timestamp), chan.get_funding_height())
+
+        # the server claims the funding tx is unconfirmed again. Either it got reorged, or the server is lying.
+        chan.update_onchain_state(
+            funding_txid=funding_txid,
+            funding_height=TxMinedInfo(_height=0, conf=0),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
+        self.assertFalse(chan.can_be_deleted())  # still, it cannot be deleted yet
+        mock_lnworker.remove_channel.assert_not_called()
+
+        # the channel is now older than the funding timeout
+        self.current_height += lnutil.CHANNEL_OPENING_TIMEOUT_BLOCKS + 1
+        chan.storage['init_timestamp'] -= CHANNEL_OPENING_TIMEOUT_SEC + 1
+
+        # As we *never* saw the incoming channel reach the required number confs, chan can now be deleted.
+        self.assertFalse(chan.is_funded())
+        self.assertTrue(chan.has_funding_timed_out())
+        self.assertTrue(chan.can_be_deleted())
+        mock_lnworker.remove_channel.assert_not_called()
+
+        # New tick: no change to onchain state.
+        chan.update_onchain_state(
+            funding_txid=funding_txid,
+            funding_height=TxMinedInfo(_height=0, conf=0),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
+        mock_lnworker.remove_channel.assert_called()  # chan now auto-deleted.
+        self.assertIsNone(self.bob_lnwallet.get_channel_by_id(chan.channel_id))
+
     async def test_update_unfunded_zeroconf_channel(self):
         """Cover the zeroconf branch of update_unfunded_state"""
         chan = self.bob_channel
@@ -601,7 +759,13 @@ async def test_update_unfunded_zeroconf_channel(self):
         self.assertEqual(chan.balance(LOCAL), 500000000000)
         bob.config.ZEROCONF_TRUSTED_NODE = trusted_node
 
-        chan.update_unfunded_state()
+        chan.update_onchain_state(
+            funding_txid=None,
+            funding_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
 
         # assert nothing happened
         self.assertIsNotNone(bob.get_channel_by_id(chan.channel_id))
@@ -613,7 +777,13 @@ async def test_update_unfunded_zeroconf_channel(self):
         chan.storage['init_timestamp'] -= ZEROCONF_TIMEOUT + 1
         bob.wallet.is_up_to_date = lambda: False
 
-        chan.update_unfunded_state()
+        chan.update_onchain_state(
+            funding_txid=None,
+            funding_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
 
         # assert nothing happened again
         self.assertIsNotNone(bob.get_channel_by_id(chan.channel_id))
@@ -625,7 +795,14 @@ async def test_update_unfunded_zeroconf_channel(self):
         # now her wallet is synced, and the channel is still unfunded
         bob.wallet.is_up_to_date = lambda: True
 
-        chan.update_unfunded_state()
+        self.assertTrue(chan.is_zeroconf())
+        chan.update_onchain_state(
+            funding_txid=None,
+            funding_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
 
         # check zeroconf provider gets unset
         self.assertEqual(bob.config.ZEROCONF_TRUSTED_NODE, "")
@@ -635,8 +812,16 @@ async def test_update_unfunded_zeroconf_channel(self):
         # time out funding (~2 weeks)
         chan.storage['init_timestamp'] -= CHANNEL_OPENING_TIMEOUT_SEC + 1
         self.assertTrue(chan.has_funding_timed_out())
-
-        chan.update_unfunded_state()
+        self.assertTrue(chan.is_zeroconf())
+        self.assertTrue(chan.can_be_deleted())
+
+        chan.update_onchain_state(
+            funding_txid=None,
+            funding_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            closing_txid=None,
+            closing_height=TxMinedInfo(_height=TX_HEIGHT_LOCAL, conf=0),
+            keep_watching=True,
+        )
 
         # check that channel got removed, now that funding has timed out
         self.assertIsNone(self.alice_lnwallet.get_channel_by_id(chan.channel_id))