Introduce OnchainTxHandler, move bumping and tracking logic #462

ariard · 2020-01-24T21:58:30Z

Build on top of #461, unify transaction generation code in its own nodule.

Simplify parsing code (easier to implement option_simplified_commitment) and first step towards removal of in-memory private keys from ChannelMonitor.

Note: there is one intermediary failure on e2a3bd6, which is related to #459, need to rebase also on top of it when it's merged.

devrandom · 2020-01-31T03:11:14Z

lightning/src/ln/onchaintx.rs

+			tx_weight +=  match inp {
+				// number_of_witness_elements + sig_length + revocation_sig + pubkey_length + revocationpubkey + witness_script_length + witness_script
+				&InputDescriptors::RevokedOfferedHTLC => {
+					1 + 1 + 73 + 1 + 33 + 1 + 133


not for this PR, but I wonder if it will be more robust to create a sample tx and serialize it to get these numbers

Sure further dry-up of this code with some tx template what somehow in my mind

devrandom · 2020-01-31T23:24:04Z

lightning/src/ln/onchaintx.rs

+							log_trace!(self, "Can't new-estimation bump new claiming tx, amount {} is too small", $amount);
+							return None;
+						}
+					// ...else just increase the previous feerate by 25% (because that's a nice number)


(I know this is not changed in this PR)

Might want to consider escalating the fee much faster in the exponential increase case, in case the fee estimator is wrong. Even switching to 100% increase/try after some time. The game theory is that we might lose the entire channel value, so even if we end up paying 10% in fees it may be worth it.

Also, if the fee estimator increases moderately for a long time, the second branch (exponential fee increase) will never be taken, and again if the fee estimator is wrong we can lose funds.

I agree picking up a %25 bump was just magic and more aggressive heuristics could be implemented. So far IIRC we are the only implem bumping fees would like more feedback from other LN devs.

The game theory remark is a very good one.

devrandom

Initial incomplete pass.

devrandom · 2020-01-31T23:29:46Z

lightning/src/ln/channelmonitor.rs

@@ -1395,12 +1220,13 @@ impl ChannelMonitor {
 	/// HTLC-Success/HTLC-Timeout transactions.
 	/// Return updates for HTLC pending in the channel and failed automatically by the broadcast of
 	/// revoked remote commitment tx
-	fn check_spend_remote_transaction(&mut self, tx: &Transaction, height: u32, fee_estimator: &FeeEstimator) -> (Vec<Transaction>, (Sha256dHash, Vec<TxOut>), Vec<SpendableOutputDescriptor>) {
+	fn check_spend_remote_transaction(&mut self, tx: &Transaction, height: u32) -> (HashMap<Sha256dHash, Vec<(u32, bool, BitcoinOutPoint, InputMaterial)>>, (Sha256dHash, Vec<TxOut>), Option<SpendableOutputDescriptor>) {


The four-tuple should probably be a crate visible struct.

Good idea, added a ClaimRequest struct with more doc about field purpose.

devrandom · 2020-01-31T23:49:07Z

lightning/src/ln/onchaintx.rs

+	}
+
+	fn get_height_timer(current_height: u32, timelock_expiration: u32) -> u32 {
+		if timelock_expiration <= current_height || timelock_expiration - current_height <= 3 {


The second clause subsumes the first.

Thanks, corrected in its own commit as it was already there.

Reserved this one because got test failures due to unsigned int overflow..

timelock_expiration <= current_height + 3 should work.

devrandom · 2020-02-01T00:08:16Z

lightning/src/ln/channelmonitor.rs

-										inputs_info.push((payment_preimage, tx.output[transaction_output_index as usize].value, htlc.cltv_expiry));
-										total_value += tx.output[transaction_output_index as usize].value;
-									} else {
-										let mut single_htlc_tx = Transaction {


I'm probably missing something, but I don't see where this code moved.

It's L1321 as of 8603afb, aggregation now happens in OnchainTxHandler so there is no more a isolated vs multiple htlcs branch anymore.

TheBlueMatt · 2020-02-05T03:12:21Z

Was kinda hoping #461 went in first, but waiting to take a look at this for concept acks before that was unfair. Sorry about that.

At a high-level design-wise, I like the direction, though I'm not sure exactly what the end-goal is. I presume the eventual goal is to just have ChannelMonitor watch the chain with no way to sign anything, and then just call OnchainTxHandler to get the signed transactions. Thus, the monitoring would be disconnected from the signer, and OnchainTxHandler could contain only pre-signed transactions. However, that leaves me confused why the RBF bumping would be in OnchainTxHandler.

Alternatively, OnchainTxHandler could be responsible for monitoring any outputs which are "to-be-claimed", with ChannelMonitor solely responsible for detecting such outputs and notifying ChannelManager appropriately after OnchainTxHandler has done its job, though that would leave OnchainTxHandler with a further substruct which could either be presigned txn or with a ChannelKeys/Signer.

ariard · 2020-02-10T02:51:29Z

Alternatively, OnchainTxHandler could be responsible for monitoring any outputs which are "to-be-claimed", with ChannelMonitor solely responsible for detecting such outputs and notifying ChannelManager appropriately after OnchainTxHandler has done its job, though that would leave OnchainTxHandler with a further substruct which could either be presigned txn or with a ChannelKeys/Signer.

End-goals are unifying transaction generation code, encapsulate in-flight and bumping logic complexity in its own module and simplify tx parsing code (a.k.a the detection). Like you said, next step is obviously to add Signer in OnchainTxHandler. Signer implementation would determine if it's in watchtower mode or local mode (sign_justice_tx would return pre-signed txn for watchtower).

ChannelMonitor would be left with only channel tx detection and passing preimage/timeout to offchain, which are already complex tasks (specially if we have different commitment tx format like first option_simplified_commitment).

Long-term, OnchainTxHandler could be reused for dual-funding tx/splicing, where we may have to bump onchain txn too (I don't want to write another bumping logic).

However, that leaves me confused why the RBF bumping would be in OnchainTxHandler.

I still need to think about this but IMO what you can sign determine how you can bump (no key access for watchtower means CPFP for justice tx instead of RBF).

TheBlueMatt · 2020-02-10T19:03:50Z

Right, detection-vs-response is a good distinction, I just wanted to make sure that was the goal, and note that its not the only thing we need.

devrandom · 2020-02-12T18:11:50Z

My comments were addressed. (note that for some reason there are no "Resolve" buttons visible to me)

ariard · 2020-02-12T21:26:23Z

Rebased 0de8d5d, ready for new review.

TheBlueMatt

Didn't get to do a bunch more than a skim-through but the top-level API looks great. Noted a few nits that I came across but a full review may have to wait until next week.

TheBlueMatt · 2020-02-12T22:37:36Z

lightning/src/ln/channelmonitor.rs

 	Revoked {
+		/// Witness script


Note that the documentation-required checks don't apply to pub(crate) things. Two word comments probably indicate you should just change the variable name and move on :)

TheBlueMatt · 2020-02-12T22:40:22Z

lightning/src/ln/onchaintx.rs

+	// block connection we scan all inputs and if any of them is among a set of a claiming request we test for set
+	// equality between spending transaction and claim request. If true, it means transaction was one our claiming one
+	// after a security delay of 6 blocks we remove pending claim request. If false, it means transaction wasn't and
+	// we need to regenerate new claim request with reduced set of still-claimable outpoints.


If you're gonna fix a spelling mistake in a mostly-move, can you do it in a separate commit so that --color-moved doesn't barf?

I don't remember to do a spelling fix there ? What git options did you use there?

git show --color-moved highlights this line as a different color.

TheBlueMatt · 2020-02-12T22:41:27Z

lightning/src/ln/channelmonitor.rs

 	// We simply modify last_block_hash in Channel's block_connected so that serialization is
 	// consistent but hopefully the users' copy handles block_connected in a consistent way.
 	// (we do *not*, however, update them in insert_combine to ensure any local user copies keep
 	// their last_block_hash from its state and not based on updated copies that didn't run through
 	// the full block_connected).
 	pub(crate) last_block_hash: Sha256dHash,
-	secp_ctx: Secp256k1<secp256k1::All>, //TODO: dedup this a bit...
+	secp_ctx: Secp256k1<secp256k1::All>,


Why? We should still think about deduping the secp contexts especially in a ManyChannelMonitor.

Ooops, I tried at first to throw it entirely in OnchainTxHandler IIIRC. That's said, after moving local commitment txn construction there too, secp state should be the matter of the Signer

lightning/src/ln/functional_tests.rs

arik-so · 2020-02-13T23:07:45Z

lightning/src/ln/functional_tests.rs

+		assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), 71);
+		assert_eq!(node_txn[1].clone().input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+
+		timeout_tx = node_txn[2].clone();


nit: on some occasions it's tx, others it's txn. We should probably use the same abbreviated form. I'd personally opt for tx.

Right, bookmarked point, I prefer to defer this until the Great Functional Test cleanup

arik-so · 2020-02-14T00:29:35Z

lightning/src/ln/onchaintx.rs

+			}],
+		};
+
+		macro_rules! RBF_bump {


what's the reason for this being a macro?

IIRC I though at first having different code paths for different transaction cases (revoked, valid-remote broadcast, ...) but in fact was able to subsume all them on one. I think it can be functionify when we implement CPFP as a bumping mechanism or implement a better bumping strategy (cf devrandom point above)

arik-so · 2020-02-14T00:33:03Z

lightning/src/ln/onchaintx.rs

+			match per_outp_material {
+				&InputMaterial::Revoked { ref script, ref is_htlc, ref amount, .. } => {
+					inputs_witnesses_weight += Self::get_witnesses_weight(if !is_htlc { &[InputDescriptors::RevokedOutput] } else if HTLCType::scriptlen_to_htlctype(script.len()) == Some(HTLCType::OfferedHTLC) { &[InputDescriptors::RevokedOfferedHTLC] } else if HTLCType::scriptlen_to_htlctype(script.len()) == Some(HTLCType::AcceptedHTLC) { &[InputDescriptors::RevokedReceivedHTLC] } else { &[] });
+					amt += *amount;


would strongly recommend using different names from these variables. Maybe revoked_amount and total_amount?

Or current_revoked_amount

Thanks, updated name with suggestions

lightning/src/ln/onchaintx.rs

arik-so · 2020-02-14T00:34:37Z

lightning/src/ln/onchaintx.rs

+			}
+		}
+
+		let new_timer = Self::get_height_timer(height, cached_claim_datas.soonest_timelock);


some explanation of what's happening here and what the variables will be used for could bee heelpful

Yes add comment on assumptions there and also on top of get_height_timer, tell me if it's clear enough :)

lightning/src/ln/onchaintx.rs

ariard · 2020-02-18T18:15:25Z

Thanks @TheBlueMatt and @arik-so for reviewing, addressed your comment at e9dff10.

ariard · 2020-02-26T22:02:22Z

Rebased on top #489 at 5aa3816

TheBlueMatt · 2020-02-27T17:50:29Z

Travis failure is

   --> lightning/src/ln/channelmonitor.rs:811:22

    |

811 |  secp_ctx: Secp256k1<secp256k1::All>, //TODO: dedup this a bit...

    |                      ^^^^^^^^^ Use of undeclared type or module `secp256k1`

error[E0433]: failed to resolve. Use of undeclared type or module `secp256k1`

   --> lightning/src/ln/onchaintx.rs:174:22

    |

174 |  secp_ctx: Secp256k1<secp256k1::All>,

    |                      ^^^^^^^^^ Use of undeclared type or module `secp256k1`

Can you rebase on master now that 489 landed?

ariard · 2020-02-28T17:01:20Z

Rebased bce1508, with Travis 1.22 failure fixed.

TheBlueMatt

Only looked at the first two commits so far - can you squash them, as there appears to be a few awkward issues introduced in between that I dont feel like reviewing carefully :).

lightning/src/ln/onchaintx.rs

lightning/src/ln/channelmonitor.rs

TheBlueMatt · 2020-02-28T18:29:56Z

lightning/src/ln/channelmonitor.rs

 					}
 				}
 			}

-			if !inputs.is_empty() || !txn_to_broadcast.is_empty() || per_commitment_option.is_some() { // ie we're confident this is actually ours
+			// Last, track onchain revoked commitment transaction and fail backward outgoing HTLCs as payment path is broken
+			if !claimable_outpoints.is_empty() || per_commitment_option.is_some() { // ie we're confident this is actually ours


I dont think its possible for us to have inserted into claimable_outpoints by this point, only outpoints. I guess there are no tests which hit this?

Hmmm is this check accurate even using outpoints, given per_commitment_option.is_some (and we shouldn't care about tracking revoked remote commitment tx on chain if there is no htlc outputs, which may explain why we don't hit with a test)

lightning/src/ln/channelmonitor.rs

lightning/src/ln/functional_test_utils.rs

TheBlueMatt

Looks good, a few complaints but all relatively minor in the scheme of things. LoC diff stats are so nice, tho.

lightning/src/ln/onchaintx.rs

TheBlueMatt · 2020-03-04T02:25:30Z

lightning/src/ln/onchaintx.rs

+		Some((new_timer, new_feerate, bumped_tx))
+	}
+
+	pub(super) fn block_connected<B: Deref, F: Deref>(&mut self, txn_matched: &[&Transaction], claim_requests: HashMap<Sha256dHash, Vec<ClaimRequest>>, height: u32, broadcaster: B, fee_estimator: F) -> Vec<SpendableOutputDescriptor>


claim_requests seems pretty redundant as-is - its a map from previous-txid to a vec of a struct which contains the previous-txid in the outpoint. Can we drop the duplication to make it a bit easier to see correctness?

Yeah, my initial worry was someone inadvertently introducing cross-block aggregation, segmenting by txid was the reason.

Right, but why not just have it be a map from previous-txid to something that contains the vout, instead of a full OutPoint?

TheBlueMatt · 2020-03-04T02:36:24Z

lightning/src/ln/onchaintx.rs

+					new_claimable_outpoints.insert(k.clone(), (txid, height));
+				}
+				log_trace!(self, "Broadcast onchain {}", log_tx!(tx));
+				spendable_outputs.push(SpendableOutputDescriptor::StaticOutput {


We shouldn't generate this until the tx is confirmed.

Yes but that's already current behavior, pre-refactoring. Spendable output generation after ANTI_REORG_SAFE_DELAY is a todo in another PR

Grr that sucks. Alright, we should improve that, then.

lightning/src/ln/onchaintx.rs

TheBlueMatt · 2020-03-04T02:40:06Z

lightning/src/ln/onchaintx.rs

+			self.claimable_outpoints.insert(k, v);
+		}
+		for (k, v) in new_pending_claim_requests.drain() {
+			self.pending_claim_requests.insert(k, v);


Why not do this in the loop instead of having another map to track them before adding them later?

I thought borrow-checker would complain for some ownership already taken on self, but it works well, generate_claim_tx isn't mutable (it used to be in a previous verison, or at least I tried with one...)

lightning/src/ln/onchaintx.rs

TheBlueMatt · 2020-03-04T02:44:56Z

lightning/src/ln/channelmonitor.rs

 		pubkey: Option<PublicKey>,
 		key: SecretKey,
 		is_htlc: bool,
-		amount: u64,
+		revoked_amount: u64,


I really dont understand the need for this commit - amount, revoked_amount, local_amount and remote_amount are all the amount as it appears in the transaction output. I think the new names may be even more confusing as it'll make me do a double-take to make sure we don't need to calculate them from some Channel data instead of from on-chain outputs.

Okay was in reaction to #462 (comment), I'm ~0 on this, dropped the commit

Encapsulates tracking and bumping of in-flight transactions in its own component. This component may be latter abstracted to reuse tracking and RBF for new features (e.g dual-funding, splicing) Build all transactions generation in one place. Also as fees and signatures are closely tied, what keys do you have determine what bumping mode you can use.

Height timer as an important component of a more-secure, fee-sensitive claiming of time-constrained LN outputs, therefore document assumptions.

TheBlueMatt · 2020-03-04T21:36:52Z

lightning/src/ln/onchaintx.rs

+		let mut aggregated_soonest = ::std::u32::MAX;
+		let mut spendable_outputs = Vec::new();
+
+		// Try to aggregate outputs if they're 1) belong to same parent tx, 2) their


This comment is wrong now - we may aggregate an HTLC claim which spent a commitment tx with a spend of the commitment tx. This is, of course, fine, as any observer doesn't learn anything from that behavior, but the comment should be fixed.

Hmmm why comment is wrong ? Spending a HTLC output and to_local/to_remote output that still spending same commitment tx, sorry your comment is unclear to me.

TheBlueMatt · 2020-03-04T21:37:51Z

lightning/src/ln/onchaintx.rs

+		Some((new_timer, new_feerate, bumped_tx))
+	}
+
+	pub(super) fn block_connected<B: Deref, F: Deref>(&mut self, txn_matched: &[&Transaction], claimable_outpoints: Vec<Vec<ClaimRequest>>, height: u32, broadcaster: B, fee_estimator: F) -> Vec<SpendableOutputDescriptor>


Please drop the Vec<Vec<>>. That seems needlessly inefficient.

codecov · 2020-03-04T21:42:12Z

Codecov Report

Merging #462 into master will decrease coverage by 0.24%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
- Coverage   89.97%   89.72%   -0.25%     
==========================================
  Files          33       34       +1     
  Lines       19150    18997     -153     
==========================================
- Hits        17231    17046     -185     
- Misses       1919     1951      +32

Impacted Files	Coverage Δ
lightning/src/ln/channelmonitor.rs	`90.30% <0.00%> (-2.76%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.17% <0.00%> (-0.08%)`	⬇️
lightning/src/ln/onchaintx.rs	`92.46% <0.00%> (ø)`
lightning/src/ln/onion_utils.rs	`93.88% <0.00%> (+0.01%)`	⬆️
lightning/src/ln/reorg_tests.rs	`98.94% <0.00%> (+0.02%)`	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.31% <0.00%> (+0.02%)`	⬆️
lightning/src/ln/peer_handler.rs	`50.08% <0.00%> (+0.16%)`	⬆️

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 e946357...a4a5e01. Read the comment docs.

TheBlueMatt · 2020-03-04T22:23:09Z

I'm gonna have a few follow-ups anyway that are only partially-related, so I'll just take the above comments with those.

This comment was stale and referred to a previous implementation of lightningdevkit#462, which changed before it was merged.

A few minor nits on #462

ariard force-pushed the 2020-01-refactor-chan branch from f23fcb4 to 8603afb Compare January 24, 2020 22:49

devrandom reviewed Jan 31, 2020

View reviewed changes

devrandom reviewed Feb 1, 2020

View reviewed changes

ariard force-pushed the 2020-01-refactor-chan branch 2 times, most recently from 9d24658 to 5cb0880 Compare February 12, 2020 21:25

TheBlueMatt reviewed Feb 12, 2020

View reviewed changes

arik-so reviewed Feb 14, 2020

View reviewed changes

TheBlueMatt added this to the 0.0.10 milestone Feb 17, 2020

ariard force-pushed the 2020-01-refactor-chan branch from 5cb0880 to e9dff10 Compare February 18, 2020 18:14

TheBlueMatt modified the milestones: 0.0.10, 0.0.11 Feb 26, 2020

ariard force-pushed the 2020-01-refactor-chan branch from e9dff10 to 5aa3816 Compare February 26, 2020 22:01

ariard force-pushed the 2020-01-refactor-chan branch 2 times, most recently from 4167f24 to bce1508 Compare February 28, 2020 17:00

TheBlueMatt reviewed Feb 28, 2020

View reviewed changes

TheBlueMatt reviewed Mar 2, 2020

View reviewed changes

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

ariard force-pushed the 2020-01-refactor-chan branch 2 times, most recently from a063ba1 to 9888ff3 Compare March 3, 2020 00:38

TheBlueMatt reviewed Mar 4, 2020

View reviewed changes

Antoine Riard added 4 commits March 4, 2020 16:06

Structurify claim request handed between detection/reaction

1433535

Remove TestBroadcaster temporary dedup buffer

d86423c

Comment better get_height_timer logic.

e8cb076

Height timer as an important component of a more-secure, fee-sensitive claiming of time-constrained LN outputs, therefore document assumptions.

Rename InputMaterial script to witness_script

a4a5e01

ariard force-pushed the 2020-01-refactor-chan branch from 9888ff3 to a4a5e01 Compare March 4, 2020 21:08

TheBlueMatt reviewed Mar 4, 2020

View reviewed changes

TheBlueMatt merged commit 48549de into lightningdevkit:master Mar 4, 2020

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Mar 4, 2020

Correct comment in onchaintx.rs

67ab227

This comment was stale and referred to a previous implementation of lightningdevkit#462, which changed before it was merged.

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Mar 5, 2020

Correct comment in onchaintx.rs

9de9288

This comment was stale and referred to a previous implementation of lightningdevkit#462, which changed before it was merged.

TheBlueMatt added a commit that referenced this pull request Mar 5, 2020

Merge pull request #535 from TheBlueMatt/2020-03-462-nits

d850e12

A few minor nits on #462

Introduce OnchainTxHandler, move bumping and tracking logic #462

Introduce OnchainTxHandler, move bumping and tracking logic #462

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devrandom Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Feb 5, 2020

Uh oh!

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

Uh oh!

Uh oh!

TheBlueMatt commented Feb 10, 2020

Uh oh!

devrandom commented Feb 12, 2020

Uh oh!

ariard commented Feb 12, 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!

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!

ariard commented Jan 24, 2020 •

edited

Loading

devrandom Jan 31, 2020 •

edited

Loading

ariard commented Feb 10, 2020 •

edited

Loading