Remove some duplicata of broadcast txn from ChannelMonitor #461

ariard · 2020-01-24T05:26:38Z

First part of refactoring ChannelMonitor, removing duplicata transaction for single-signed transactions. Doing it separately from actual ChannelMonitor to ease review.

Removing duplicata avoid some pitfalls

locking twice bumping inputs if we have to CPFP some transactions (not-yet-implemented)
transaction origin inferences if tx broadcaster doesn't dedup tx itself
useless signing operations
undefined behavior with an external signer implementing some signature replay protections

A second refactoring should move remaining dual-signed transactions, to do latter or while implementing option_simplified_commitment.

TheBlueMatt

Nice. Needs rebase but should be easy.

TheBlueMatt · 2020-01-26T20:41:56Z

lightning/src/ln/channelmonitor.rs

@@ -1772,11 +1772,11 @@ impl ChannelMonitor {
 					let mut inputs_info = Vec::new();

 					macro_rules! sign_input {
-						($sighash_parts: expr, $input: expr, $amount: expr, $preimage: expr) => {
+						($sighash_parts: expr, $input: expr, $amount: expr, $preimage: expr, $idx: expr) => {


If you're gonna stop relying on the stupid hack of shoving the idx in the sequence field (which is a good cleanup), can you do it in its own commit and fully remove it instead of only partially?

That was a quick fix, given than in #462, the macro and a bunch of codes around is going away. Split in its own commit anyway (bundled there because of the very basic dedup parser in test_utils relying on nSequence)

Right, but its easier to review if you just do it all the way...this patch seems to pass tests:

diff --git a/lightning/src/ln/channelmonitor.rs b/lightning/src/ln/channelmonitor.rs index d55db634..8a393be9 100644 --- a/lightning/src/ln/channelmonitor.rs +++ b/lightning/src/ln/channelmonitor.rs @@ -1805,17 +1805,16 @@ impl ChannelMonitor { } if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) { if htlc.offered { - let mut input = TxIn { + let input = TxIn { previous_output: BitcoinOutPoint { txid: commitment_txid, vout: transaction_output_index, }, script_sig: Script::new(), - sequence: idx as u32, // reset to 0xfffffffd before sign_input + sequence: 0xff_ff_ff_fd, witness: Vec::new(), }; if htlc.cltv_expiry > height + CLTV_SHARED_CLAIM_BUFFER { - input.sequence = 0xff_ff_ff_fd; inputs.push(input); inputs_desc.push(if htlc.offered { InputDescriptors::OfferedHTLC } else { InputDescriptors::ReceivedHTLC }); inputs_info.push((payment_preimage, tx.output[transaction_output_index as usize].value, htlc.cltv_expiry, idx)); @@ -1834,8 +1833,6 @@ impl ChannelMonitor { let height_timer = Self::get_height_timer(height, htlc.cltv_expiry); let mut used_feerate; if subtract_high_prio_fee!(self, fee_estimator, single_htlc_tx.output[0].value, predicted_weight, used_feerate) { - let idx = single_htlc_tx.input[0].sequence; - single_htlc_tx.input[0].sequence = 0xff_ff_ff_fd; let sighash_parts = bip143::SighashComponents::new(&single_htlc_tx); let (redeemscript, htlc_key) = sign_input!(sighash_parts, single_htlc_tx.input[0], htlc.amount_msat / 1000, payment_preimage.0.to_vec(), idx); assert!(predicted_weight >= single_htlc_tx.get_weight()); @@ -1868,7 +1865,7 @@ impl ChannelMonitor { vout: transaction_output_index, }, script_sig: Script::new(), - sequence: idx as u32, + sequence: 0xff_ff_ff_fd, witness: Vec::new(), }; let mut timeout_tx = Transaction { @@ -1884,8 +1881,6 @@ impl ChannelMonitor { let height_timer = Self::get_height_timer(height, htlc.cltv_expiry); let mut used_feerate; if subtract_high_prio_fee!(self, fee_estimator, timeout_tx.output[0].value, predicted_weight, used_feerate) { - let idx = timeout_tx.input[0].sequence; - timeout_tx.input[0].sequence = 0xff_ff_ff_fd; let sighash_parts = bip143::SighashComponents::new(&timeout_tx); let (redeemscript, htlc_key) = sign_input!(sighash_parts, timeout_tx.input[0], htlc.amount_msat / 1000, vec![0], idx); assert!(predicted_weight >= timeout_tx.get_weight());

TheBlueMatt · 2020-01-27T17:09:53Z

Still needs rebase, I think.

ariard · 2020-01-30T02:46:42Z

Is it ready or are you expecting #461 (comment) sorry ?

TheBlueMatt · 2020-01-30T18:38:01Z

I'd prefer to see the that patch included just cause it makes review much more obvious.

ariard · 2020-01-30T20:03:03Z

Somehow took your patch

TheBlueMatt

Looks good. There's a number of places where the comment is(+was) incorrect which I'd prefer to see cleaned up. At some point this tx broadcast checking mess is going to need to be cleaned up.

TheBlueMatt · 2020-02-03T02:58:21Z

lightning/src/ln/functional_tests.rs

-		assert_eq!(node_txn[3], node_txn[10]);
-		assert_eq!(node_txn[4], node_txn[11]);
-		assert_eq!(node_txn[3], node_txn[5]); //local commitment tx + htlc timeout tx broadcasted by ChannelManger
+		assert_eq!(node_txn.len(), 26); // ChannelManager : 2, ChannelMontitor: 8 (1 standard revoked output, 2 revocation htlc tx, 1 local commitment tx + 1 htlc timeout tx) * 2 (block-rescan) + 5 * (1 local commitment tx + 1 htlc timeout tx)


Why did the comment not change (and why does it seem to indicate we should have 23)?

Updated with better comment, see log commented if you want to check : ariard@6fa4bd0

TheBlueMatt · 2020-02-03T03:00:04Z

lightning/src/ln/functional_tests.rs

-		assert_eq!(node_txn[3], node_txn[5]); //local commitment tx + htlc timeout tx broadcasted by ChannelManger
+		assert_eq!(node_txn.len(), 26); // ChannelManager : 2, ChannelMontitor: 8 (1 standard revoked output, 2 revocation htlc tx, 1 local commitment tx + 1 htlc timeout tx) * 2 (block-rescan) + 5 * (1 local commitment tx + 1 htlc timeout tx)
+
+		// 3 = 5 = 7 = 9 = 14 = 17 = 20 = 23


Whats the intuition between these all being the same (can you add a comment for it?)? The comment doesn't seem to indicate they should be.

It's for local commitment tx + HTLC-Timeout tx duplicata, right now they are still duplicated (even as of #466, planning to do it in its own PR because transaction generation of this class of txn isn't actually a thing in as-of-master bump_claim_tx

TheBlueMatt · 2020-02-03T03:02:24Z

lightning/src/ln/functional_tests.rs

-		($node: expr, $htlc_offered: expr, $commitment_tx: expr, $chan_tx: expr) => { {
-			// ChannelManager : 3 (commitment tx, 2*HTLC-Timeout tx), ChannelMonitor : 2 (timeout tx) * 2 (block-rescan)
+		($node: expr, $htlc_offered: expr, $commitment_tx: expr, $chan_tx: expr, $len: expr) => { {
+			// ChannelManager : 3 (commitment tx, 2*HTLC-Timeout tx), ChannelMonitor : 2 (timeout tx)


This comment is similarly out of date (can you explain why one case has 7 and one 5) and can you just use the $htlc_offered flag to check for 7 vs 5 instead of adding a new parameter?

There is diff between node 0 and node 1 because in one case the commitment tx is local and in the other case that's remote so same explanation than comment above (local HTLC-timeout are still duplicated)

lightning/src/ln/functional_tests.rs

TheBlueMatt · 2020-02-03T03:09:18Z

lightning/src/ln/functional_tests.rs

-	check_spends!(b_txn[1], chan_1.3); // Local commitment tx, issued by ChannelManager
-	check_spends!(b_txn[2], b_txn[1]); // HTLC-Success tx, as a part of the local txn rebroadcast by ChannelManager in the force close
-	assert_eq!(b_txn[0], b_txn[3]); // HTLC-Success tx, issued by ChannelMonitor, * 2 due to block rescan
+	// ChannelMonitor: claim tx, ChannelManager: local commitment tx


This says 2, but then we check for 3?

Oops, check is false, comment right, deleted too much. Should be fix

ariard · 2020-02-11T23:52:38Z

Updated at 2668817 with better comments.

At some point this tx broadcast checking mess is going to need to be cleaned up.

Best we can do is to have a check-template for every type of transaction and pass the expected list of templates to test method on tx_broadcaster. And find a way to desecribe tx graph dependency in an easy way

lightning/src/ln/functional_tests.rs

Previously, if new ouputs were found to be watched as part of channel operations, the block was rescan which triggers again parser and generation of transactions already issued. This commit first modifies the test framework without altering further ChannelMonitor. ChannelMonitor refactoring is introduced in a latter commit.

ariard · 2020-02-12T05:37:56Z

494219e, more comments fixed

TheBlueMatt reviewed Jan 26, 2020

View reviewed changes

ariard force-pushed the 2020-remove-duplicata branch from 6c29de6 to 3d0ff09 Compare January 27, 2020 03:13

ariard force-pushed the 2020-remove-duplicata branch from 3d0ff09 to 182aa76 Compare January 27, 2020 18:13

Explicit pass index at HTLC spending transaction signing

0c595a7

ariard force-pushed the 2020-remove-duplicata branch from 182aa76 to b889804 Compare January 30, 2020 20:02

devrandom mentioned this pull request Jan 30, 2020

Add ChannelKeys to ChannelMonitor #466

Merged

TheBlueMatt reviewed Feb 3, 2020

View reviewed changes

ariard force-pushed the 2020-remove-duplicata branch from b889804 to 2668817 Compare February 11, 2020 23:44

TheBlueMatt reviewed Feb 12, 2020

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

ariard force-pushed the 2020-remove-duplicata branch from 2668817 to 494219e Compare February 12, 2020 05:37

TheBlueMatt approved these changes Feb 12, 2020

View reviewed changes

TheBlueMatt merged commit c906f28 into lightningdevkit:master Feb 12, 2020

Remove some duplicata of broadcast txn from ChannelMonitor #461

Remove some duplicata of broadcast txn from ChannelMonitor #461

Uh oh!

Conversation

ariard commented Jan 24, 2020

Uh oh!

TheBlueMatt left a comment

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!

TheBlueMatt commented Jan 27, 2020

Uh oh!

ariard commented Jan 30, 2020

Uh oh!

TheBlueMatt commented Jan 30, 2020

Uh oh!

ariard commented Jan 30, 2020

Uh oh!

TheBlueMatt left a comment

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!

ariard commented Feb 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariard commented Feb 12, 2020

Uh oh!

Uh oh!