Move pre-signed channel transactions generation in OnchainTxHandler #540

ariard · 2020-03-10T00:00:21Z

Next step in moving private keys behind an external signer interface, moving pre-signed channel transactions (local commitment, HTLC-Success, HTLC-Timeout) in OnchainTxHandler. There is no bump logic, as without option_simplified_commitment changes we can't CPFP/BYOF them.

After this, code should be ready to move key storage inside OnchainTxHandler and some chan_utils methods in signer. We need also to think about bumping logic, if we make it an interface accessible to the signer.

TheBlueMatt · 2020-03-10T16:47:45Z

Note: failed borrowck on 1.22.

codecov · 2020-03-10T22:20:38Z

Codecov Report

Merging #540 into master will decrease coverage by 0.02%.
The diff coverage is 96.01%.

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
- Coverage   90.23%   90.21%   -0.03%     
==========================================
  Files          34       34              
  Lines       19182    19120      -62     
==========================================
- Hits        17309    17249      -60     
+ Misses       1873     1871       -2

Impacted Files	Coverage Δ
lightning/src/ln/channelmonitor.rs	`92.15% <100%> (+1.89%)`	⬆️
lightning/src/util/macro_logger.rs	`87.5% <100%> (+2.71%)`	⬆️
lightning/src/ln/onchaintx.rs	`92.99% <90.27%> (+0.17%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.26% <97.36%> (-0.08%)`	⬇️
lightning/src/ln/wire.rs	`54.54% <0%> (-11.31%)`	⬇️
lightning/src/ln/msgs.rs	`88.1% <0%> (-1.62%)`	⬇️
lightning/src/util/config.rs	`68.29% <0%> (-0.76%)`	⬇️
lightning/src/ln/features.rs	`96.62% <0%> (-0.26%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`94.44% <0%> (-0.16%)`	⬇️
... and 5 more

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 33b7c90...d66011d. Read the comment docs.

TheBlueMatt

First round.

lightning/src/ln/channelmonitor.rs

TheBlueMatt · 2020-03-17T19:57:29Z

lightning/src/ln/onchaintx.rs

@@ -514,17 +548,19 @@ impl OnchainTxHandler {
 			if let Some((new_timer, new_feerate, tx)) = self.generate_claim_tx(height, &claim_material, &*fee_estimator) {
 				claim_material.height_timer = new_timer;
 				claim_material.feerate_previous = new_feerate;
+				if claim_material.height_timer != ::std::u32::MAX {


Please use Option<> instead of a sentinal value.

Yeah, spendable outputs is reworked in next PR, so just temporary but switch to a Option<> ofc

TheBlueMatt · 2020-03-17T20:07:56Z

lightning/src/ln/channelmonitor.rs

+	},
+	Funding {
+		local_tx_remote_signed: LocalCommitmentTransaction,
+		funding_key: SecretKey,


Don't pass the SecretKeys - you can get them out of the ChannelKeys already with ChannelKeys::funding_key() or ChannelKeys::htlc_base_key().

Hmmmm what do you suggest exactly ? To introduce yet ChannelKeys in OnchainTxHandler ?

Yea, just pass in a reference to the ChannelMonitor ChannelKeys in OnchainTxHandler::block_connected() and pass it down. We can rework it later, but its so awkward to copy private keys around like this.

Previously, we would regenerate this class of txn twice due to block-rescan triggered by new watching outputs registered. This commmit doesn't change behavior, it only tweaks TestBroadcaster to ensure we modify cleanly tests anticipating next commit refactor.

Watchtower will be supported through external signer interface where a watchtower implementation may differ from a local one by the scope of key access and pre-signed datas.

ariard · 2020-03-18T06:43:21Z

@TheBlueMatt okay added 2 new commits : one to drop Watchtower mode from Storage, tower support is planned to be implemented as an external signer implem, so remove it from ChannelMonitor which is for detection. The other to move duplicate keys storage in OnchainTxHanndler, to avoid passing back and forth stuff in InputMaterial. Next PRs should keep drying up key usage in ChannelMonitor and pass Storage behind ChanSigner.

TheBlueMatt

Note this currently does not build.

TheBlueMatt · 2020-03-18T17:02:30Z

lightning/src/ln/functional_tests.rs

 		assert_eq!(node_txn[3].input.len(), 1);
+		let witness_script = node_txn[3].input[0].witness.last().unwrap();
+		assert_eq!(witness_script.len(), OFFERED_HTLC_SCRIPT_WEIGHT); //Spending an offered htlc output
+		check_spends!(node_txn[3], node_txn[2].clone());


Rebase error - you dont need the clone()s anymore. Also in a few other places.

Going further between splitting detection and transaction generation, we endow OnchainTxHandler with keys access. That way, in latter commits, we may remove keys entirely from ChannelMonitor.

Local Commitment Transaction can't be bumped without anchor outputs so their generation is one-time for now. We move them in OnchainTxHandler for simplifying ChannelMonitor and to prepare storage of keys material behind one external signer interface. Some tests break due to change in transaction broadcast order but number of transactions broadcast should stay the same.

HTLC Transaction can't be bumped without sighash changes so their gneeration is one-time for nwo. We move them in OnchainTxHandler for simplifying ChannelMonitor and to prepare storage of keys material behind one external signer interface. Some tests break due to change in transaction broadcaster order. Number of transactions may vary because of temporary anti-duplicata tweak can't dissociate between 2- broadcast from different origins (ChannelMonitor, ChannelManager) and 2-broadcast from same component.

TheBlueMatt · 2020-03-18T19:17:57Z

lightning/src/ln/functional_tests.rs

@@ -7168,11 +7176,11 @@ fn test_set_outpoints_partial_claiming() {
 	let partial_claim_tx = {
 		let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap();
 		assert_eq!(node_txn.len(), 3);
-		check_spends!(node_txn[1], node_txn[0]);
-		check_spends!(node_txn[2], node_txn[0]);
+		check_spends!(node_txn[0], node_txn[2].clone());


Rebase error - clone is not needed here.

TheBlueMatt · 2020-03-18T19:23:04Z

lightning/src/ln/channelmonitor.rs

+				// If would_broadcast_at_height determine this local tx should be broadcast, absolute_timelock is set to current_height,
+				// because it indicates that this tx confirmation is urgent. It's not going to change anything because we can't bump
+				// local_commitment before anchor_outputs, should be rethought afterwards
+				claimable_outpoints.push(ClaimRequest { absolute_timelock: height, aggregable: false, outpoint: BitcoinOutPoint { txid: self.key_storage.funding_info.as_ref().unwrap().0.txid.clone(), vout: self.key_storage.funding_info.as_ref().unwrap().0.index as u32 }, witness_data: InputMaterial::Funding { local_tx_remote_signed: cur_local_tx.tx.clone(), channel_value: self.channel_value_satoshis.unwrap() }});


clone() ing the cur_local_tx wholesale here really sucks. a) you need to drop the add_local_sig line above, since we should now be signing + broadcasting this tx in onchain wholesale, and b) can we take() the cur_local_tx.tx and pass it to OnChainTxHandler? We should never be doing anything with it aside from sign, then broadcast, so can it happen in onchain?

TheBlueMatt · 2020-03-18T19:23:05Z

lightning/src/ln/functional_tests.rs

@@ -2334,24 +2334,32 @@ fn claim_htlc_outputs_single_tx() {
 		// ChannelMonitor: local commitment + local HTLC-timeout (2)

 		// Check the pair local commitment and HTLC-timeout broadcast due to HTLC expiration
+		assert_eq!(node_txn[3].input.len(), 1);
+		check_spends!(node_txn[3], chan_1.3.clone());


Same here - can you do a pass for clone() in check_spends? Seems like you reintroduced a bunch of them.

TheBlueMatt · 2020-03-20T00:42:20Z

Needs rebase.

ariard · 2020-03-24T00:30:26Z

Replaced by #559

ariard force-pushed the 2020-02-move-local-txn branch from 15fa3be to 5927ccf Compare March 10, 2020 22:20

TheBlueMatt reviewed Mar 17, 2020

View reviewed changes

ariard force-pushed the 2020-02-move-local-txn branch from 5927ccf to f453402 Compare March 17, 2020 21:35

Remove Watchtower mode from Storage enum and make it a struct

ee5949e

Watchtower will be supported through external signer interface where a watchtower implementation may differ from a local one by the scope of key access and pre-signed datas.

ariard force-pushed the 2020-02-move-local-txn branch from f453402 to d66011d Compare March 18, 2020 06:41

ariard force-pushed the 2020-02-move-local-txn branch from d66011d to 45db495 Compare March 18, 2020 16:58

TheBlueMatt reviewed Mar 18, 2020

View reviewed changes

Antoine Riard added 5 commits March 18, 2020 13:39

Add Storage in OnchainTxHandler

e1f5e9b

Going further between splitting detection and transaction generation, we endow OnchainTxHandler with keys access. That way, in latter commits, we may remove keys entirely from ChannelMonitor.

Add logger for SpendableOutputDescriptor

d44f0b1

Remove temporary anti-duplicata logic

2fea557

ariard force-pushed the 2020-02-move-local-txn branch from 45db495 to 2fea557 Compare March 18, 2020 18:31

TheBlueMatt reviewed Mar 18, 2020

View reviewed changes

TheBlueMatt mentioned this pull request Mar 19, 2020

Generate latest local commitment transactions via monitor avoiding Channel's copy #551

Merged

ariard closed this Mar 24, 2020

Move pre-signed channel transactions generation in OnchainTxHandler #540

Move pre-signed channel transactions generation in OnchainTxHandler #540

Uh oh!

Conversation

ariard commented Mar 10, 2020

Uh oh!

TheBlueMatt commented Mar 10, 2020

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt 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!

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!

ariard commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

TheBlueMatt commented Mar 20, 2020

Uh oh!

ariard commented Mar 24, 2020

Uh oh!

Uh oh!

codecov bot commented Mar 10, 2020 •

edited

Loading

ariard commented Mar 18, 2020 •

edited

Loading