Add pruning of preimages no longer needed + tests #32

ariard · 2018-06-25T02:10:24Z

No description provided.

TheBlueMatt

Awesome! Thanks so much for going after this TODO!a

TheBlueMatt · 2018-06-25T14:18:44Z

src/ln/channelmonitor.rs

+					pruned_payment_preimages.insert(k, v); 
+				}
+			}
+			for (_tx, htlcs) in &self.remote_claimable_outpoints {


This seems like its gonna be a pretty large chunk of runtime, but also overkill. Can we effeciently skip remote outpoints that are already revoked somehow, cause we shouldn't need those preimages either, we can use the revocation key.

Outpoint is considered as revoked when we get its per_commitment_secret. At pruning, we need to check that its commitment number is superior at get_min_secret(), but how do we get this commitment number without input sequence and lock_time of the given outpoint, it's only place where it's stored.
Will inquiry more tomorrow to find a map between txid:commitment number, correct me if I'm wrong.

TheBlueMatt · 2018-06-25T14:19:14Z

src/ln/channelmonitor.rs

-		// that non-revoked remote commitment tx(n) do not need it, and our latest local commitment
-		// tx does not need it.
+
+		let mut pruned_payment_preimages = HashMap::with_capacity(self.payment_preimages.capacity());


Can you use self.payment_preimages.retain instead of creating a new map and drain()ing the old one? Should be a bit less overhead to doing it that way.

Should be fine

TheBlueMatt · 2018-06-25T14:19:29Z

src/ln/channelmonitor.rs

 		Ok(())
 	}
-
+	


Note you have some trailing whitespace in a few places.

Yes, configured my vim extension accordingly

TheBlueMatt · 2018-06-25T14:20:15Z

src/ln/channelmonitor.rs

+
+		let mut pruned_payment_preimages = HashMap::with_capacity(self.payment_preimages.capacity());
+		for (k, v) in self.payment_preimages.drain() {
+			for (htlc, _s1, _s2) in &self.current_local_signed_commitment_tx.as_ref().expect("Channel need at least an initial commitment tx !").htlc_outputs {


This needs to be for &(ref htlc... to make it compile on older rustcs. See travis failure on 1.22.

Compiled with 1.22, got it

TheBlueMatt

Looks good, thanks, though didn't review the tests in detail (a comment or two describing whats going on would be nice).

TheBlueMatt · 2018-06-27T21:18:58Z

src/ln/channelmonitor.rs

+				}
+				for (_tx, htlcs) in remote_claimable_outpoints {
+					for htlc in htlcs {
+						if k == htlc.payment_hash {


Still kinda worried about the potential performance issue here, especially for watchtowers.

I'd think the way to go here would be some kind of simple caching of the set of unrevoked remote outpoints. I think technically there should only ever be one, but I think channel resumption may break that so lets not assume it. Still, that means having a Vec of the txids and their commitment number (which we know when we call provide_latest_remote_commitment_tx_info, either by calculating it or just making channel pass it in, which may be simpler) should be really cheap.

Yes, I wrote that but not so simple, for a given outpoint (txid, commitment number) you have a set of hash. But how do you map hash to the outpoint without browsing its HTLCs ? You need to cache the binding somewhere, so I was thinking more to a hashmap <payment_hash,commitment number> still generate at provide_latest_remote_commitment_tx_info. That's a dup of data into ChannelMonitor, but will improve performance because you scan htlcs once. Will write that.

TheBlueMatt · 2018-06-27T21:21:19Z

src/ln/channelmonitor.rs

+		let local_signed_commitment_tx = &self.current_local_signed_commitment_tx;
+		let remote_claimable_outpoints = &self.remote_claimable_outpoints;
+		self.payment_preimages.retain(|&k, _| {
+				for &(ref htlc, _s1, _s2) in &local_signed_commitment_tx.as_ref().expect("Channel needs at least an initial commitment tx !").htlc_outputs {


Can you document this precondition in the function's documentation?

Done, also comments for tests

TheBlueMatt · 2018-06-27T21:26:38Z

src/ln/channelmonitor.rs

+				}
+				for (_tx, htlcs) in remote_claimable_outpoints {
+					for htlc in htlcs {
+						if k == htlc.payment_hash {


I'd think the way to go here would be some kind of simple caching of the set of unrevoked remote outpoints. I think technically there should only ever be one, but I think channel resumption may break that so lets not assume it. Still, that means having a Vec of the txids and their commitment number (which we know when we call provide_latest_remote_commitment_tx_info, either by calculating it or just making channel pass it in, which may be simpler) should be really cheap.

ariard · 2018-06-28T23:30:26Z

Change the logic for handling of remote_outpoints, with caching of hash:commitment number at provide_latest_remote_commitment_tx_info. At pruning, if hash belongs to tx non-revoked, keep it, else skip it. Should be more efficient at price of hodling more data.
Let me know your thoughts about it.

TheBlueMatt · 2018-06-29T20:43:54Z

Merging, but I've got a few changes I'll make afterwards myself.

…ingdevkit#32

…t#32

Some cleanups after #32

…ip-change-check Drop unused change output calculation

TheBlueMatt reviewed Jun 25, 2018

View reviewed changes

ariard force-pushed the prune_payment_preimages branch from e76bcd2 to 6e307c6 Compare June 26, 2018 23:12

TheBlueMatt reviewed Jun 27, 2018

View reviewed changes

Add pruning of preimages no longer needed + tests

ef16642

ariard force-pushed the prune_payment_preimages branch from 6e307c6 to ef16642 Compare June 28, 2018 23:23

TheBlueMatt merged commit f9ec0a7 into lightningdevkit:master Jun 29, 2018

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Jun 29, 2018

Skip some a needless temporary vec and clean indentation after lightn…

04b0acc

…ingdevkit#32

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Jun 29, 2018

A few minor test cleanups and remove new warnings from lightningdevki…

aec0fa4

…t#32

TheBlueMatt added a commit that referenced this pull request Jun 29, 2018

Merge pull request #35 from TheBlueMatt/2018-06-post-32

98e5c5e

Some cleanups after #32

ariard deleted the prune_payment_preimages branch August 27, 2018 22:28

carlaKC pushed a commit to carlaKC/rust-lightning that referenced this pull request Aug 9, 2023

Merge pull request lightningdevkit#32 from lightningdevkit/2021-08-sk…

18904ed

…ip-change-check Drop unused change output calculation

Add pruning of preimages no longer needed + tests #32

Add pruning of preimages no longer needed + tests #32

Uh oh!

Conversation

ariard commented Jun 25, 2018

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!

TheBlueMatt Jun 25, 2018 • 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!

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

ariard Jun 28, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Jun 28, 2018

Uh oh!

TheBlueMatt commented Jun 29, 2018

Uh oh!

Uh oh!

TheBlueMatt Jun 25, 2018 •

edited

Loading

ariard Jun 28, 2018 •

edited

Loading