Add method to note transaction unconfirmed/reorged-out #853

TheBlueMatt · 2021-03-22T22:12:50Z

This is based on #838, and adds two new methods transactions_unconfirmed() and get_relevant_txids() to ChannelManager, allowing an electrum client to explicitly notify us that the funding transaction has been reorganized out of the chain. Needs more docs and a more careful analysis of whether we can make this API work for ChannelMonitor, but for now we may ship a Java bindings binary with this.

codecov · 2021-03-22T22:47:09Z

Codecov Report

Merging #853 (d6e7116) into main (9577928) will increase coverage by 0.00%.
The diff coverage is 92.17%.

❗ Current head d6e7116 differs from pull request most recent head 6982434. Consider uploading reports for the commit 6982434 to get more accurate results

@@           Coverage Diff           @@
##             main     #853   +/-   ##
=======================================
  Coverage   90.18%   90.19%           
=======================================
  Files          55       55           
  Lines       28913    28985   +72     
=======================================
+ Hits        26075    26142   +67     
- Misses       2838     2843    +5

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`87.24% <64.28%> (-0.12%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.06% <91.42%> (+0.06%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.18% <94.11%> (+0.04%)`	⬆️
lightning/src/ln/reorg_tests.rs	`99.62% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.84% <0.00%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9577928...6982434. Read the comment docs.

jkczyz

Concept ACK. Will investigate the ChannelMonitor side.

lightning/src/ln/channelmanager.rs

jkczyz · 2021-03-24T03:39:57Z

lightning/src/ln/channelmanager.rs

@@ -3477,6 +3479,30 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}

+	/// Gets the set of txids which should be monitored for their confirmation state. XXX: Docs
+	pub fn get_relevant_txids(&self) -> Vec<Txid> {


Would this be considered an alternative interface to chain::Filter::register_tx then?

In the ChannelManager context, yes, but it wouldn't work for ChannelMonitor - ChannelManager only ever cares about one, known-in-advance-txid transaction, whereas ChannelMonitor doesn't know about txids that it cares about until they are confirmed (or, it shouldn't expose them, the list is potentially huge).

lightning/src/ln/channel.rs

lightning/src/ln/reorg_tests.rs

jkczyz · 2021-03-24T04:32:26Z

lightning/src/ln/reorg_tests.rs

+		if use_funding_unconfirmed {
+			let relevant_txids = nodes[0].node.get_relevant_txids();
+			assert_eq!(relevant_txids.len(), 1);
+			assert_eq!(&relevant_txids[..], &[chan.3.txid()]);
+			nodes[0].node.transaction_unconfirmed(&relevant_txids[0]);
+		} else {
+			disconnect_all_blocks(&nodes[0]);
+		}
+		if connect_style == ConnectStyle::FullBlockViaListen && !use_funding_unconfirmed {
+			handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Funding transaction was un-confirmed. Locked at 6 confs, now have 2 confs.");
+		} else {
+			handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.");
+		}
+		check_added_monitors!(nodes[1], 1);


This test keeps growing... Would it make sense to separate the various behaviors into their own tests at this point? It isn't clear to me that the cartesian product needs to be tested.

Hmm, we could clone the test and run the full set of connection styles against something that drops the reload code, but we'd still end up with basically the same thing duplicated. I definitely think we need to test the set of full connection/disconnection styles, since I found unexpected bugs in this test when doing so across the full test suite :).

TheBlueMatt · 2021-04-05T22:14:12Z

Rebased now that #838 has been merged.

ariard

SGTM modulo small comments

lightning/src/ln/channel.rs

ariard · 2021-04-09T21:29:08Z

lightning/src/ln/channelmanager.rs

@@ -850,6 +850,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}

+	/// Gets the current configuration applied to all new channels,  as
+	pub fn get_current_default_configuration(&self) -> &UserConfig {


Need to correct comment, "as" what ?

Yeah, still needs to be fixed.

lightning/src/ln/channel.rs

TheBlueMatt · 2021-04-13T01:35:56Z

Addressed comments and added requisite documentation, this should be ready to go now.

valentinewallace · 2021-04-13T19:45:01Z

lightning/src/ln/channel.rs

+	/// Indicates the funding transaction is no longer confirmed in the main chain. This may
+	/// force-close the channel, but may also indicate a harmless reorganization of a block or two.
+	pub fn funding_transaction_unconfirmed(&mut self) -> Result<(), msgs::ErrorMessage> {
+		if self.funding_tx_confirmation_height != 0 {


Hm, I don't see funding_tx_confirmation_height being reset back to 0 at any point -- should it be?

Hmm, we do reset it back to zero in update_best_block in the funding_tx_confirmation_height == 0 check.

Hmm, that doesn't reset self.funding_tx_confirmation_height I think? https://i.imgur.com/kgXgXLV.png

Also, should we be resetting funding_tx_confirmed_in and short_channel_id as well? (And maybe unit test this if it's easy?)

Currently we'll always close the channel in this case due to the if funding_tx_confirmations < self.minimum_depth as i64 / 2 { check. I'll add a test and stop doing that in a followup.

valentinewallace · 2021-04-13T19:58:56Z

lightning/src/ln/channelmanager.rs

+	/// [`transactions_confirmed`] if they are confirmed, but a small subset of it.
+	///
+	/// [`transactions_confirmed`]: Self::transactions_confirmed
+	/// [`transaction_unconfirmed`]: Self::transaction_unconfirmed


Hm I think it's not obvious to me what exact situations this is supposed to be used.
Are Electrum users supposed to call it on startup so they can register these txs w/ Electrum?

Tried to address this, let me know if that's clearer.

FWIW, I'll try to unify and clean-up the docs for this and those in #858 when making a trait for these to implement.

Yep, sounds good. good to have an example here to pull from/merge from, too.

lightning/src/ln/channelmanager.rs

jkczyz · 2021-04-14T07:12:41Z

lightning/src/ln/channelmanager.rs

+	/// [`transactions_confirmed`] if they are confirmed, but a small subset of it.
+	///
+	/// [`transactions_confirmed`]: Self::transactions_confirmed
+	/// [`transaction_unconfirmed`]: Self::transaction_unconfirmed


FWIW, I'll try to unify and clean-up the docs for this and those in #858 when making a trait for these to implement.

jkczyz · 2021-04-14T07:21:24Z

lightning/src/ln/channelmanager.rs

+	/// with an Electrum server or for which an electrum server needs to be polled to determine
+	/// transaction confirmation state.
+	///
+	/// This may update after any [`transactions_confirmed`] or [`block_connected`] call.


What's the use case for calling this with block_connected?

jkczyz · 2021-04-14T07:24:55Z

lightning/src/ln/channel.rs

 	/// Indicates the funding transaction is no longer confirmed in the main chain. This may
-	/// force-close the channel, but may also indicate a harmless reorganization of a block or two.
+	/// force-close the channel, but may also indicate a harmless reorganization of a block or two
+	/// before the channel has reached funding_locked and we can just wait for more blocks.


Second sentence is a bit wordy / run-on. But isn't it really the caller that will close the channel upon receiving an error back?

valentinewallace · 2021-04-14T14:20:14Z

lightning/src/ln/channel.rs

+			}
+		} else {
+			// We never learned about the funding confirmation anyway, just ignore
+			Ok(())


Is this theoretically supposed to be unreachable, but it seems harmless so we can just Ok(()) in practice?

Yea, I think it should be really rare, but you could see some async process getting delayed and ending up calling transaction_unconfirmed twice in a row which could hit this.

valentinewallace · 2021-04-14T14:23:47Z

lightning/src/ln/channel.rs

+	/// Indicates the funding transaction is no longer confirmed in the main chain. This may
+	/// force-close the channel, but may also indicate a harmless reorganization of a block or two.
+	pub fn funding_transaction_unconfirmed(&mut self) -> Result<(), msgs::ErrorMessage> {
+		if self.funding_tx_confirmation_height != 0 {


Also, should we be resetting funding_tx_confirmed_in and short_channel_id as well? (And maybe unit test this if it's easy?)

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

valentinewallace · 2021-04-14T14:45:44Z

lightning/src/ln/channelmanager.rs

+	/// Note that this is NOT the set of transactions which must be included in calls to
+	/// [`transactions_confirmed`] if they are confirmed, but a small subset of it.


I assume the rest will be registered in one of the register_tx calls? Could be worth adding a note, since this line may come across naively as a bit scary but unactionable...

Nw though if Jeff is addressing the docs more thoroughly in #858.

I believe the plan is still for @jkczyz to do a followup PR after this + #858 that will add a new top-level trait similar to chain::Listen and merge all the docs/functions then.

lightning/src/ln/channelmanager.rs

valentinewallace · 2021-04-14T14:58:36Z

lightning/src/ln/channelmanager.rs

+	/// be very weary of race-conditions wherein the final state of a transaction indicated via
+	/// these APIs is not the same as its state on the blockchain.


Found this race condition description a bit confusing -- I assume "these APIs" == transaction_confirmed/get_relevant_txids?

"the final state of a transaction indicated by these APIs is not the same as its state on the blockchain" == just because a tx is included or not included in get_relevant_txids, doesn't tell you anything about this state on the blockchain? If that's correct, then sure, but I guess it's not obvious to me how that'd lead to a race condition.

TheBlueMatt · 2021-04-14T17:39:43Z

Addressed a number of doc comments, but for the top-level docs on transaction_unconfirmed, lets plan on cleaning those up in a followup? Since they'll move to a trait/merge with #858 anyway.

TheBlueMatt · 2021-04-14T18:27:23Z

Squashed with one doc-bug issue CI pointed out:

$ git diff-tree -U1 d6e7116 6982434c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 81cd95ef..f9e00bc5 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -3550,2 +3550,3 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// [`transaction_unconfirmed`]: Self::transaction_unconfirmed
+	/// [`block_connected`]: chain::Listen::block_connected
 	pub fn get_relevant_txids(&self) -> Vec<Txid> {

TheBlueMatt force-pushed the 2021-03-transaction_unconfirmed branch from 5719ac7 to c263bcb Compare March 22, 2021 22:28

jkczyz reviewed Mar 24, 2021

View reviewed changes

jkczyz mentioned this pull request Apr 2, 2021

Electrum interface for ChannelMonitor #858

Merged

TheBlueMatt added this to the 0.0.14 milestone Apr 5, 2021

TheBlueMatt force-pushed the 2021-03-transaction_unconfirmed branch from 83fc5d1 to b6d8de4 Compare April 5, 2021 22:13

ariard reviewed Apr 9, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-03-transaction_unconfirmed branch 3 times, most recently from bb0dd01 to 495196e Compare April 13, 2021 01:35

TheBlueMatt marked this pull request as ready for review April 13, 2021 01:35

TheBlueMatt force-pushed the 2021-03-transaction_unconfirmed branch from 495196e to cb09003 Compare April 13, 2021 01:46

valentinewallace self-requested a review April 13, 2021 19:16

valentinewallace reviewed Apr 13, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-03-transaction_unconfirmed branch from cb09003 to a442990 Compare April 14, 2021 01:44

jkczyz reviewed Apr 14, 2021

View reviewed changes

valentinewallace reviewed Apr 14, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-03-transaction_unconfirmed branch from a442990 to d6e7116 Compare April 14, 2021 17:37

jkczyz approved these changes Apr 14, 2021

View reviewed changes

valentinewallace approved these changes Apr 14, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-03-transaction_unconfirmed branch from d6e7116 to b6a8560 Compare April 14, 2021 18:15

TheBlueMatt added 3 commits April 14, 2021 14:26

Add method to note transaction unconfirmed/reorged-out

bdbba5e

Expose ChannelManager's current config and use it in reload in tests

46ac4c7

Test new transactions_unconfirmed

6982434

TheBlueMatt force-pushed the 2021-03-transaction_unconfirmed branch from b6a8560 to 6982434 Compare April 14, 2021 18:26

TheBlueMatt merged commit 9397db6 into lightningdevkit:main Apr 14, 2021

		/// Note that this is NOT the set of transactions which must be included in calls to
		/// [`transactions_confirmed`] if they are confirmed, but a small subset of it.

		/// be very weary of race-conditions wherein the final state of a transaction indicated via
		/// these APIs is not the same as its state on the blockchain.

Add method to note transaction unconfirmed/reorged-out #853

Add method to note transaction unconfirmed/reorged-out #853

Uh oh!

Conversation

TheBlueMatt commented Mar 22, 2021

Uh oh!

codecov bot commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Apr 5, 2021

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Apr 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Apr 14, 2021

codecov bot commented Mar 22, 2021 •

edited

Loading

TheBlueMatt commented Apr 14, 2021 •

edited

Loading