Make CounterpartyCommitmentSecrets public #1299

Tibo-lg · 2022-02-08T01:26:55Z

I'm working on implementation and specifications of DLC channels (not on LN for now) and I thought it'd be a good idea to reuse the secret commitment scheme, and thus would like to reuse the CounterpartyCommitmentSecrets struct in my code.

I mainly added some comments, let me know if something is off.

codecov-commenter · 2022-02-08T01:51:50Z

Codecov Report

Merging #1299 (131cb8c) into main (d29ae18) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 131cb8c differs from pull request most recent head 23b32ef. Consider uploading reports for the commit 23b32ef to get more accurate results

@@            Coverage Diff             @@
##             main    #1299      +/-   ##
==========================================
- Coverage   90.51%   90.49%   -0.02%     
==========================================
  Files          71       71              
  Lines       39012    39012              
==========================================
- Hits        35310    35304       -6     
- Misses       3702     3708       +6

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`95.51% <100.00%> (ø)`
lightning-invoice/src/de.rs	`81.06% <0.00%> (-0.21%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.20% <0.00%> (-0.08%)`	⬇️

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 d29ae18...23b32ef. Read the comment docs.

lightning/src/ln/chan_utils.rs

TheBlueMatt

LGTM aside from the wrong link.

TheBlueMatt · 2022-02-09T01:07:49Z

lightning/src/ln/chan_utils.rs

-	/// Can only fail if idx is < get_min_seen_secret
-	pub(crate) fn get_secret(&self, idx: u64) -> Option<[u8; 32]> {
+	/// Returns the secret at index `idx`.
+	/// Returns `None` if idx is < [CounterpartyCommitmentSecrets::get_min_secret].


Note that CI is failing cause this link is bad - its get_min_seen_secret.

Fixed sorry, I doubled check locally it should be good now 🤞

jkczyz

Just some nits on the doc style

jkczyz · 2022-02-09T19:08:42Z

lightning/src/ln/chan_utils.rs

@@ -197,7 +200,9 @@ impl CounterpartyCommitmentSecrets {
 		res
 	}

-	pub(crate) fn provide_secret(&mut self, idx: u64, secret: [u8; 32]) -> Result<(), ()> {
+	/// Inserts the secret `secret` at index `idx`. Returns Ok(()) if the secret


I think you can leave off the leading non-ticked words "secret" and "index". Likewise elsewhere.

Done I hope I got them all

jkczyz · 2022-02-09T19:09:02Z

lightning/src/ln/chan_utils.rs

@@ -197,7 +200,9 @@ impl CounterpartyCommitmentSecrets {
 		res
 	}

-	pub(crate) fn provide_secret(&mut self, idx: u64, secret: [u8; 32]) -> Result<(), ()> {
+	/// Inserts the secret `secret` at index `idx`. Returns Ok(()) if the secret


Add ticks around Ok(())

jkczyz · 2022-02-09T19:10:11Z

lightning/src/ln/chan_utils.rs

-	/// Can only fail if idx is < get_min_seen_secret
-	pub(crate) fn get_secret(&self, idx: u64) -> Option<[u8; 32]> {
+	/// Returns the secret at index `idx`.
+	/// Returns `None` if idx is < [CounterpartyCommitmentSecrets::get_min_seen_secret].


Use ticks around identifiers.

Done (I think it was only idx on second line missing?)

Also: [`CounterpartyCommitmentSecrets::get_min_seen_secret`]

Fixed thanks, need to read up more on doc formatting.

jkczyz · 2022-02-09T19:10:26Z

lightning/src/ln/chan_utils.rs

@@ -173,7 +174,9 @@ impl CounterpartyCommitmentSecrets {
 		48
 	}

-	pub(crate) fn get_min_seen_secret(&self) -> u64 {
+	/// Return the minimum index of all stored secrets. Note that indexes start


s/Return/Returns

Tibo-lg · 2022-02-10T00:00:31Z

lightning/src/ln/chan_utils.rs

@@ -140,10 +140,10 @@ pub fn build_closing_transaction(to_holder_value_sat: u64, to_counterparty_value
 /// Implements the per-commitment secret storage scheme from
 /// [BOLT 3](https://github.com/lightningnetwork/lightning-rfc/blob/dcbf8583976df087c79c3ce0b535311212e6812d/03-transactions.md#efficient-per-commitment-secret-storage).
 ///
-/// Allows us to keep track of all of the revocation secrets of counterarties in just 50*32 bytes
+/// Allows us to keep track of all of the revocation secrets of counter parties in just 50*32 bytes


FYI fixed a typo here

counterparty is one word, not two, so I believe counterparties would be correct.

changed, my spellchecker didn't like counterparties I guess I'll have to teach him :)

TheBlueMatt

heh, one more nit, but feel free to ignore it, it doesn't matter that much.

TheBlueMatt · 2022-02-10T02:54:15Z

lightning/src/ln/chan_utils.rs

@@ -140,10 +140,10 @@ pub fn build_closing_transaction(to_holder_value_sat: u64, to_counterparty_value
 /// Implements the per-commitment secret storage scheme from
 /// [BOLT 3](https://github.com/lightningnetwork/lightning-rfc/blob/dcbf8583976df087c79c3ce0b535311212e6812d/03-transactions.md#efficient-per-commitment-secret-storage).
 ///
-/// Allows us to keep track of all of the revocation secrets of counterarties in just 50*32 bytes
+/// Allows us to keep track of all of the revocation secrets of counterparties in just 50*32 bytes


Oh, lol, this shoulnd't be plural anyway. s/counterparties/our counterparty/

TheBlueMatt reviewed Feb 8, 2022

View reviewed changes

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

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

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

Tibo-lg force-pushed the make-counterpartycommitmentsecrets-public branch from 12d5dc2 to 8fdf255 Compare February 8, 2022 23:48

TheBlueMatt reviewed Feb 9, 2022

View reviewed changes

Tibo-lg force-pushed the make-counterpartycommitmentsecrets-public branch from 8fdf255 to 76cb45c Compare February 9, 2022 02:00

TheBlueMatt previously approved these changes Feb 9, 2022

View reviewed changes

jkczyz reviewed Feb 9, 2022

View reviewed changes

Tibo-lg dismissed TheBlueMatt’s stale review via 131cb8c February 9, 2022 23:59

Tibo-lg force-pushed the make-counterpartycommitmentsecrets-public branch from 76cb45c to 131cb8c Compare February 9, 2022 23:59

Tibo-lg commented Feb 10, 2022

View reviewed changes

Tibo-lg force-pushed the make-counterpartycommitmentsecrets-public branch from 131cb8c to 23b32ef Compare February 10, 2022 01:36

TheBlueMatt previously approved these changes Feb 10, 2022

View reviewed changes

Tibo-lg dismissed TheBlueMatt’s stale review via b901df2 February 10, 2022 03:19

Tibo-lg force-pushed the make-counterpartycommitmentsecrets-public branch from 23b32ef to b901df2 Compare February 10, 2022 03:19

TheBlueMatt previously approved these changes Feb 10, 2022

View reviewed changes

Make CounterpartyCommitmentSecrets public

ba289b8

Tibo-lg dismissed TheBlueMatt’s stale review via ba289b8 February 10, 2022 06:25

Tibo-lg force-pushed the make-counterpartycommitmentsecrets-public branch from b901df2 to ba289b8 Compare February 10, 2022 06:25

jkczyz approved these changes Feb 10, 2022

View reviewed changes

TheBlueMatt approved these changes Feb 10, 2022

View reviewed changes

TheBlueMatt merged commit 9be8a66 into lightningdevkit:main Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CounterpartyCommitmentSecrets public #1299

Make CounterpartyCommitmentSecrets public #1299

Tibo-lg commented Feb 8, 2022

codecov-commenter commented Feb 8, 2022 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt Feb 9, 2022

Tibo-lg Feb 9, 2022

jkczyz left a comment

jkczyz Feb 9, 2022

Tibo-lg Feb 9, 2022

jkczyz Feb 9, 2022

Tibo-lg Feb 9, 2022

jkczyz Feb 9, 2022

Tibo-lg Feb 10, 2022

jkczyz Feb 10, 2022

Tibo-lg Feb 10, 2022

jkczyz Feb 9, 2022

Tibo-lg Feb 10, 2022

Tibo-lg Feb 10, 2022

TheBlueMatt Feb 10, 2022

Tibo-lg Feb 10, 2022

TheBlueMatt left a comment

TheBlueMatt Feb 10, 2022

Tibo-lg Feb 10, 2022

Make CounterpartyCommitmentSecrets public #1299

Make CounterpartyCommitmentSecrets public #1299

Conversation

Tibo-lg commented Feb 8, 2022

codecov-commenter commented Feb 8, 2022 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 8, 2022 •

edited

Loading