Cleanup comments and docs in channel fee update logic by TheBlueMatt · Pull Request #1047 · lightningdevkit/rust-lightning

TheBlueMatt · 2021-08-16T18:04:06Z

Following up on @ariard's review at #985 (review)

codecov · 2021-08-16T18:23:45Z

Codecov Report

Merging #1047 (623da4d) into main (a369f9e) will increase coverage by 1.37%.
The diff coverage is 50.00%.

@@ Coverage Diff @@## main #1047 +/- ##
==========================================
+ Coverage 90.96% 92.33% +1.37% 
==========================================
Files 64 65 +1 Lines 32393 43186 +10793 ==========================================
+ Hits 29467 39877 +10410 - Misses 2926 3309 +383

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`92.66% <50.00%> (+3.29%)`	⬆️
lightning/src/util/ser_macros.rs	`90.52% <0.00%> (-5.79%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.54% <0.00%> (-0.85%)`	⬇️
lightning/src/ln/script.rs	`93.57% <0.00%> (-0.18%)`	⬇️
lightning-invoice/src/de.rs	`80.62% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.58% <0.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.90% <0.00%> (+0.03%)`	⬆️
lightning/src/ln/features.rs	`99.60% <0.00%> (+0.16%)`	⬆️
lightning-background-processor/src/lib.rs	`94.17% <0.00%> (+0.23%)`	⬆️
lightning/src/ln/chan_utils.rs	`98.37% <0.00%> (+0.94%)`	⬆️
... and 19 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 a369f9e...623da4d. Read the comment docs.

ariard

ACK dd3097c-modulo-if-you-feel-it : #985 (comment)

ariard · 2021-08-17T00:26:14Z

ACK ce4e20d

TheBlueMatt · 2021-08-17T00:32:22Z

Reverted the additional debug_assert as it doesn't pass borrowck, which presumably is why it wasnt in the first pr either :)

ariard · 2021-08-21T00:09:22Z

re-ACK dd3097c

valentinewallace

Otherwise ACK

The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt · 2021-09-09T08:38:45Z

Only change from ariard's ack is comment fixups suggested by val, will take after CI:

$ git diff-tree -U1 dd3097c 623da4da
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b0f733454..f8cbc26a2 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -433,7 +433,7 @@ pub(super) struct Channel<Signer: Sign> {
pending_update_fee: Option<(u32, FeeUpdateState)>,
-	// If an update_fee() call is made with ChannelState::AwaitingRemoteRevoke set, we place it
-	// here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
+	// If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place
+	// it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
// `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into
// `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by
-	// further `update_fee` calls, dropping the previous holding cell update entirely.
+	// further `send_update_fee` calls, dropping the previous holding cell update entirely.
holding_cell_update_fee: Option<u32>,
$

TheBlueMatt · 2021-08-16T18:04:06Z

Following up on @ariard's review at #985 (review)

codecov · 2021-08-16T18:23:45Z

Codecov Report

Merging #1047 (623da4d) into main (a369f9e) will increase coverage by 1.37%.
The diff coverage is 50.00%.

@@ Coverage Diff @@## main #1047 +/- ##
==========================================
+ Coverage 90.96% 92.33% +1.37% 
==========================================
Files 64 65 +1 Lines 32393 43186 +10793 ==========================================
+ Hits 29467 39877 +10410 - Misses 2926 3309 +383

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`92.66% <50.00%> (+3.29%)`	⬆️
lightning/src/util/ser_macros.rs	`90.52% <0.00%> (-5.79%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.54% <0.00%> (-0.85%)`	⬇️
lightning/src/ln/script.rs	`93.57% <0.00%> (-0.18%)`	⬇️
lightning-invoice/src/de.rs	`80.62% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.58% <0.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.90% <0.00%> (+0.03%)`	⬆️
lightning/src/ln/features.rs	`99.60% <0.00%> (+0.16%)`	⬆️
lightning-background-processor/src/lib.rs	`94.17% <0.00%> (+0.23%)`	⬆️
lightning/src/ln/chan_utils.rs	`98.37% <0.00%> (+0.94%)`	⬆️
... and 19 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 a369f9e...623da4d. Read the comment docs.

ariard

ACK dd3097c-modulo-if-you-feel-it : #985 (comment)

ariard · 2021-08-17T00:26:14Z

ACK ce4e20d

TheBlueMatt · 2021-08-17T00:32:22Z

Reverted the additional debug_assert as it doesn't pass borrowck, which presumably is why it wasnt in the first pr either :)

ariard · 2021-08-21T00:09:22Z

re-ACK dd3097c

valentinewallace

Otherwise ACK

The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt · 2021-09-09T08:38:45Z

Only change from ariard's ack is comment fixups suggested by val, will take after CI:

$ git diff-tree -U1 dd3097c 623da4da
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b0f733454..f8cbc26a2 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -433,7 +433,7 @@ pub(super) struct Channel<Signer: Sign> {
pending_update_fee: Option<(u32, FeeUpdateState)>,
-	// If an update_fee() call is made with ChannelState::AwaitingRemoteRevoke set, we place it
-	// here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
+	// If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place
+	// it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
// `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into
// `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by
-	// further `update_fee` calls, dropping the previous holding cell update entirely.
+	// further `send_update_fee` calls, dropping the previous holding cell update entirely.
holding_cell_update_fee: Option<u32>,
$

TheBlueMatt · 2021-08-16T18:04:06Z

Following up on @ariard's review at #985 (review)

codecov · 2021-08-16T18:23:45Z

Codecov Report

Merging #1047 (623da4d) into main (a369f9e) will increase coverage by 1.37%.
The diff coverage is 50.00%.

@@ Coverage Diff @@## main #1047 +/- ##
==========================================
+ Coverage 90.96% 92.33% +1.37% 
==========================================
Files 64 65 +1 Lines 32393 43186 +10793 ==========================================
+ Hits 29467 39877 +10410 - Misses 2926 3309 +383

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`92.66% <50.00%> (+3.29%)`	⬆️
lightning/src/util/ser_macros.rs	`90.52% <0.00%> (-5.79%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.54% <0.00%> (-0.85%)`	⬇️
lightning/src/ln/script.rs	`93.57% <0.00%> (-0.18%)`	⬇️
lightning-invoice/src/de.rs	`80.62% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.58% <0.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.90% <0.00%> (+0.03%)`	⬆️
lightning/src/ln/features.rs	`99.60% <0.00%> (+0.16%)`	⬆️
lightning-background-processor/src/lib.rs	`94.17% <0.00%> (+0.23%)`	⬆️
lightning/src/ln/chan_utils.rs	`98.37% <0.00%> (+0.94%)`	⬆️
... and 19 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 a369f9e...623da4d. Read the comment docs.

ariard

ACK dd3097c-modulo-if-you-feel-it : #985 (comment)

ariard · 2021-08-17T00:26:14Z

ACK ce4e20d

TheBlueMatt · 2021-08-17T00:32:22Z

Reverted the additional debug_assert as it doesn't pass borrowck, which presumably is why it wasnt in the first pr either :)

ariard · 2021-08-21T00:09:22Z

re-ACK dd3097c

valentinewallace

Otherwise ACK

The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt · 2021-09-09T08:38:45Z

Only change from ariard's ack is comment fixups suggested by val, will take after CI:

$ git diff-tree -U1 dd3097c 623da4da
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b0f733454..f8cbc26a2 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -433,7 +433,7 @@ pub(super) struct Channel<Signer: Sign> {
pending_update_fee: Option<(u32, FeeUpdateState)>,
-	// If an update_fee() call is made with ChannelState::AwaitingRemoteRevoke set, we place it
-	// here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
+	// If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place
+	// it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
// `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into
// `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by
-	// further `update_fee` calls, dropping the previous holding cell update entirely.
+	// further `send_update_fee` calls, dropping the previous holding cell update entirely.
holding_cell_update_fee: Option<u32>,
$

TheBlueMatt · 2021-08-16T18:04:06Z

Following up on @ariard's review at #985 (review)

codecov · 2021-08-16T18:23:45Z

Codecov Report

Merging #1047 (623da4d) into main (a369f9e) will increase coverage by 1.37%.
The diff coverage is 50.00%.

@@ Coverage Diff @@## main #1047 +/- ##
==========================================
+ Coverage 90.96% 92.33% +1.37% 
==========================================
Files 64 65 +1 Lines 32393 43186 +10793 ==========================================
+ Hits 29467 39877 +10410 - Misses 2926 3309 +383

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`92.66% <50.00%> (+3.29%)`	⬆️
lightning/src/util/ser_macros.rs	`90.52% <0.00%> (-5.79%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.54% <0.00%> (-0.85%)`	⬇️
lightning/src/ln/script.rs	`93.57% <0.00%> (-0.18%)`	⬇️
lightning-invoice/src/de.rs	`80.62% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.58% <0.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.90% <0.00%> (+0.03%)`	⬆️
lightning/src/ln/features.rs	`99.60% <0.00%> (+0.16%)`	⬆️
lightning-background-processor/src/lib.rs	`94.17% <0.00%> (+0.23%)`	⬆️
lightning/src/ln/chan_utils.rs	`98.37% <0.00%> (+0.94%)`	⬆️
... and 19 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 a369f9e...623da4d. Read the comment docs.

ariard

ACK dd3097c-modulo-if-you-feel-it : #985 (comment)

ariard · 2021-08-17T00:26:14Z

ACK ce4e20d

TheBlueMatt · 2021-08-17T00:32:22Z

Reverted the additional debug_assert as it doesn't pass borrowck, which presumably is why it wasnt in the first pr either :)

ariard · 2021-08-21T00:09:22Z

re-ACK dd3097c

valentinewallace

Otherwise ACK

The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt · 2021-09-09T08:38:45Z

Only change from ariard's ack is comment fixups suggested by val, will take after CI:

$ git diff-tree -U1 dd3097c 623da4da
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b0f733454..f8cbc26a2 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -433,7 +433,7 @@ pub(super) struct Channel<Signer: Sign> {
pending_update_fee: Option<(u32, FeeUpdateState)>,
-	// If an update_fee() call is made with ChannelState::AwaitingRemoteRevoke set, we place it
-	// here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
+	// If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place
+	// it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
// `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into
// `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by
-	// further `update_fee` calls, dropping the previous holding cell update entirely.
+	// further `send_update_fee` calls, dropping the previous holding cell update entirely.
holding_cell_update_fee: Option<u32>,
$

TheBlueMatt · 2021-08-16T18:04:06Z

Following up on @ariard's review at #985 (review)

codecov · 2021-08-16T18:23:45Z

Codecov Report

Merging #1047 (623da4d) into main (a369f9e) will increase coverage by 1.37%.
The diff coverage is 50.00%.

@@ Coverage Diff @@## main #1047 +/- ##
==========================================
+ Coverage 90.96% 92.33% +1.37% 
==========================================
Files 64 65 +1 Lines 32393 43186 +10793 ==========================================
+ Hits 29467 39877 +10410 - Misses 2926 3309 +383

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`92.66% <50.00%> (+3.29%)`	⬆️
lightning/src/util/ser_macros.rs	`90.52% <0.00%> (-5.79%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.54% <0.00%> (-0.85%)`	⬇️
lightning/src/ln/script.rs	`93.57% <0.00%> (-0.18%)`	⬇️
lightning-invoice/src/de.rs	`80.62% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.58% <0.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.90% <0.00%> (+0.03%)`	⬆️
lightning/src/ln/features.rs	`99.60% <0.00%> (+0.16%)`	⬆️
lightning-background-processor/src/lib.rs	`94.17% <0.00%> (+0.23%)`	⬆️
lightning/src/ln/chan_utils.rs	`98.37% <0.00%> (+0.94%)`	⬆️
... and 19 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 a369f9e...623da4d. Read the comment docs.

ariard

ACK dd3097c-modulo-if-you-feel-it : #985 (comment)

ariard · 2021-08-17T00:26:14Z

ACK ce4e20d

TheBlueMatt · 2021-08-17T00:32:22Z

Reverted the additional debug_assert as it doesn't pass borrowck, which presumably is why it wasnt in the first pr either :)

ariard · 2021-08-21T00:09:22Z

re-ACK dd3097c

valentinewallace

Otherwise ACK

The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt · 2021-09-09T08:38:45Z

Only change from ariard's ack is comment fixups suggested by val, will take after CI:

$ git diff-tree -U1 dd3097c 623da4da
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b0f733454..f8cbc26a2 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -433,7 +433,7 @@ pub(super) struct Channel<Signer: Sign> {
pending_update_fee: Option<(u32, FeeUpdateState)>,
-	// If an update_fee() call is made with ChannelState::AwaitingRemoteRevoke set, we place it
-	// here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
+	// If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place
+	// it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
// `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into
// `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by
-	// further `update_fee` calls, dropping the previous holding cell update entirely.
+	// further `send_update_fee` calls, dropping the previous holding cell update entirely.
holding_cell_update_fee: Option<u32>,
$

TheBlueMatt · 2021-08-16T18:04:06Z

Following up on @ariard's review at #985 (review)

codecov · 2021-08-16T18:23:45Z

Codecov Report

Merging #1047 (623da4d) into main (a369f9e) will increase coverage by 1.37%.
The diff coverage is 50.00%.

@@ Coverage Diff @@## main #1047 +/- ##
==========================================
+ Coverage 90.96% 92.33% +1.37% 
==========================================
Files 64 65 +1 Lines 32393 43186 +10793 ==========================================
+ Hits 29467 39877 +10410 - Misses 2926 3309 +383

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`92.66% <50.00%> (+3.29%)`	⬆️
lightning/src/util/ser_macros.rs	`90.52% <0.00%> (-5.79%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.54% <0.00%> (-0.85%)`	⬇️
lightning/src/ln/script.rs	`93.57% <0.00%> (-0.18%)`	⬇️
lightning-invoice/src/de.rs	`80.62% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.58% <0.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.90% <0.00%> (+0.03%)`	⬆️
lightning/src/ln/features.rs	`99.60% <0.00%> (+0.16%)`	⬆️
lightning-background-processor/src/lib.rs	`94.17% <0.00%> (+0.23%)`	⬆️
lightning/src/ln/chan_utils.rs	`98.37% <0.00%> (+0.94%)`	⬆️
... and 19 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 a369f9e...623da4d. Read the comment docs.

ariard

ACK dd3097c-modulo-if-you-feel-it : #985 (comment)

ariard · 2021-08-17T00:26:14Z

ACK ce4e20d

TheBlueMatt · 2021-08-17T00:32:22Z

Reverted the additional debug_assert as it doesn't pass borrowck, which presumably is why it wasnt in the first pr either :)

ariard · 2021-08-21T00:09:22Z

re-ACK dd3097c

valentinewallace

Otherwise ACK

The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt · 2021-09-09T08:38:45Z

Only change from ariard's ack is comment fixups suggested by val, will take after CI:

$ git diff-tree -U1 dd3097c 623da4da
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b0f733454..f8cbc26a2 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -433,7 +433,7 @@ pub(super) struct Channel<Signer: Sign> {
pending_update_fee: Option<(u32, FeeUpdateState)>,
-	// If an update_fee() call is made with ChannelState::AwaitingRemoteRevoke set, we place it
-	// here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
+	// If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place
+	// it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
// `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into
// `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by
-	// further `update_fee` calls, dropping the previous holding cell update entirely.
+	// further `send_update_fee` calls, dropping the previous holding cell update entirely.
holding_cell_update_fee: Option<u32>,
$

TheBlueMatt · 2021-08-16T18:04:06Z

Following up on @ariard's review at #985 (review)

codecov · 2021-08-16T18:23:45Z

Codecov Report

Merging #1047 (623da4d) into main (a369f9e) will increase coverage by 1.37%.
The diff coverage is 50.00%.

@@ Coverage Diff @@## main #1047 +/- ##
==========================================
+ Coverage 90.96% 92.33% +1.37% 
==========================================
Files 64 65 +1 Lines 32393 43186 +10793 ==========================================
+ Hits 29467 39877 +10410 - Misses 2926 3309 +383

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`92.66% <50.00%> (+3.29%)`	⬆️
lightning/src/util/ser_macros.rs	`90.52% <0.00%> (-5.79%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.54% <0.00%> (-0.85%)`	⬇️
lightning/src/ln/script.rs	`93.57% <0.00%> (-0.18%)`	⬇️
lightning-invoice/src/de.rs	`80.62% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.58% <0.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.90% <0.00%> (+0.03%)`	⬆️
lightning/src/ln/features.rs	`99.60% <0.00%> (+0.16%)`	⬆️
lightning-background-processor/src/lib.rs	`94.17% <0.00%> (+0.23%)`	⬆️
lightning/src/ln/chan_utils.rs	`98.37% <0.00%> (+0.94%)`	⬆️
... and 19 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 a369f9e...623da4d. Read the comment docs.

ariard

ACK dd3097c-modulo-if-you-feel-it : #985 (comment)

ariard · 2021-08-17T00:26:14Z

ACK ce4e20d

TheBlueMatt · 2021-08-17T00:32:22Z

Reverted the additional debug_assert as it doesn't pass borrowck, which presumably is why it wasnt in the first pr either :)

ariard · 2021-08-21T00:09:22Z

re-ACK dd3097c

valentinewallace

Otherwise ACK

The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt · 2021-09-09T08:38:45Z

Only change from ariard's ack is comment fixups suggested by val, will take after CI:

$ git diff-tree -U1 dd3097c 623da4da
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b0f733454..f8cbc26a2 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -433,7 +433,7 @@ pub(super) struct Channel<Signer: Sign> {
pending_update_fee: Option<(u32, FeeUpdateState)>,
-	// If an update_fee() call is made with ChannelState::AwaitingRemoteRevoke set, we place it
-	// here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
+	// If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place
+	// it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
// `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into
// `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by
-	// further `update_fee` calls, dropping the previous holding cell update entirely.
+	// further `send_update_fee` calls, dropping the previous holding cell update entirely.
holding_cell_update_fee: Option<u32>,
$

TheBlueMatt · 2021-08-16T18:04:06Z

Following up on @ariard's review at #985 (review)

codecov · 2021-08-16T18:23:45Z

Codecov Report

Merging #1047 (623da4d) into main (a369f9e) will increase coverage by 1.37%.
The diff coverage is 50.00%.

@@ Coverage Diff @@## main #1047 +/- ##
==========================================
+ Coverage 90.96% 92.33% +1.37% 
==========================================
Files 64 65 +1 Lines 32393 43186 +10793 ==========================================
+ Hits 29467 39877 +10410 - Misses 2926 3309 +383

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`92.66% <50.00%> (+3.29%)`	⬆️
lightning/src/util/ser_macros.rs	`90.52% <0.00%> (-5.79%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.54% <0.00%> (-0.85%)`	⬇️
lightning/src/ln/script.rs	`93.57% <0.00%> (-0.18%)`	⬇️
lightning-invoice/src/de.rs	`80.62% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.58% <0.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.90% <0.00%> (+0.03%)`	⬆️
lightning/src/ln/features.rs	`99.60% <0.00%> (+0.16%)`	⬆️
lightning-background-processor/src/lib.rs	`94.17% <0.00%> (+0.23%)`	⬆️
lightning/src/ln/chan_utils.rs	`98.37% <0.00%> (+0.94%)`	⬆️
... and 19 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 a369f9e...623da4d. Read the comment docs.

ariard

ACK dd3097c-modulo-if-you-feel-it : #985 (comment)

ariard · 2021-08-17T00:26:14Z

ACK ce4e20d

TheBlueMatt · 2021-08-17T00:32:22Z

Reverted the additional debug_assert as it doesn't pass borrowck, which presumably is why it wasnt in the first pr either :)

ariard · 2021-08-21T00:09:22Z

re-ACK dd3097c

valentinewallace

Otherwise ACK

The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt · 2021-09-09T08:38:45Z

Only change from ariard's ack is comment fixups suggested by val, will take after CI:

$ git diff-tree -U1 dd3097c 623da4da
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b0f733454..f8cbc26a2 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -433,7 +433,7 @@ pub(super) struct Channel<Signer: Sign> {
pending_update_fee: Option<(u32, FeeUpdateState)>,
-	// If an update_fee() call is made with ChannelState::AwaitingRemoteRevoke set, we place it
-	// here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
+	// If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place
+	// it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in
// `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into
// `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by
-	// further `update_fee` calls, dropping the previous holding cell update entirely.
+	// further `send_update_fee` calls, dropping the previous holding cell update entirely.
holding_cell_update_fee: Option<u32>,
$

ariard reviewed Aug 16, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-08-985-followups branch from ce4e20d to dd3097cCompare August 17, 2021 00:31

valentinewallace reviewed Sep 8, 2021
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

TheBlueMatt force-pushed the 2021-08-985-followups branch from dd3097c to 763fc5fCompare September 9, 2021 08:37

TheBlueMatt added 2 commits September 9, 2021 08:37

Update docs for pending_update_fee and holding_cell_update_fee
fc35aa7
The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

Add further comments around fee update handling in channel
623da4d
These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt force-pushed the 2021-08-985-followups branch from 763fc5f to 623da4dCompare September 9, 2021 08:38

TheBlueMatt merged commit b3be420 into lightningdevkit:mainSep 9, 2021

ariard reviewed Aug 16, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-08-985-followups branch from ce4e20d to dd3097cCompare August 17, 2021 00:31

valentinewallace reviewed Sep 8, 2021
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

TheBlueMatt force-pushed the 2021-08-985-followups branch from dd3097c to 763fc5fCompare September 9, 2021 08:37

TheBlueMatt added 2 commits September 9, 2021 08:37

Update docs for pending_update_fee and holding_cell_update_fee
fc35aa7
The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

Add further comments around fee update handling in channel
623da4d
These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt force-pushed the 2021-08-985-followups branch from 763fc5f to 623da4dCompare September 9, 2021 08:38

TheBlueMatt merged commit b3be420 into lightningdevkit:mainSep 9, 2021

ariard reviewed Aug 16, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-08-985-followups branch from ce4e20d to dd3097cCompare August 17, 2021 00:31

valentinewallace reviewed Sep 8, 2021
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

TheBlueMatt force-pushed the 2021-08-985-followups branch from dd3097c to 763fc5fCompare September 9, 2021 08:37

TheBlueMatt added 2 commits September 9, 2021 08:37

Update docs for pending_update_fee and holding_cell_update_fee
fc35aa7
The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

Add further comments around fee update handling in channel
623da4d
These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt force-pushed the 2021-08-985-followups branch from 763fc5f to 623da4dCompare September 9, 2021 08:38

TheBlueMatt merged commit b3be420 into lightningdevkit:mainSep 9, 2021

ariard reviewed Aug 16, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-08-985-followups branch from ce4e20d to dd3097cCompare August 17, 2021 00:31

valentinewallace reviewed Sep 8, 2021
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

TheBlueMatt force-pushed the 2021-08-985-followups branch from dd3097c to 763fc5fCompare September 9, 2021 08:37

TheBlueMatt added 2 commits September 9, 2021 08:37

Update docs for pending_update_fee and holding_cell_update_fee
fc35aa7
The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

Add further comments around fee update handling in channel
623da4d
These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt force-pushed the 2021-08-985-followups branch from 763fc5f to 623da4dCompare September 9, 2021 08:38

TheBlueMatt merged commit b3be420 into lightningdevkit:mainSep 9, 2021

ariard reviewed Aug 16, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-08-985-followups branch from ce4e20d to dd3097cCompare August 17, 2021 00:31

valentinewallace reviewed Sep 8, 2021
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

TheBlueMatt force-pushed the 2021-08-985-followups branch from dd3097c to 763fc5fCompare September 9, 2021 08:37

TheBlueMatt added 2 commits September 9, 2021 08:37

Update docs for pending_update_fee and holding_cell_update_fee
fc35aa7
The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

Add further comments around fee update handling in channel
623da4d
These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt force-pushed the 2021-08-985-followups branch from 763fc5f to 623da4dCompare September 9, 2021 08:38

TheBlueMatt merged commit b3be420 into lightningdevkit:mainSep 9, 2021

ariard reviewed Aug 16, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-08-985-followups branch from ce4e20d to dd3097cCompare August 17, 2021 00:31

valentinewallace reviewed Sep 8, 2021
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

TheBlueMatt force-pushed the 2021-08-985-followups branch from dd3097c to 763fc5fCompare September 9, 2021 08:37

TheBlueMatt added 2 commits September 9, 2021 08:37

Update docs for pending_update_fee and holding_cell_update_fee
fc35aa7
The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

Add further comments around fee update handling in channel
623da4d
These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt force-pushed the 2021-08-985-followups branch from 763fc5f to 623da4dCompare September 9, 2021 08:38

TheBlueMatt merged commit b3be420 into lightningdevkit:mainSep 9, 2021

ariard reviewed Aug 16, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-08-985-followups branch from ce4e20d to dd3097cCompare August 17, 2021 00:31

valentinewallace reviewed Sep 8, 2021
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

TheBlueMatt force-pushed the 2021-08-985-followups branch from dd3097c to 763fc5fCompare September 9, 2021 08:37

TheBlueMatt added 2 commits September 9, 2021 08:37

Update docs for pending_update_fee and holding_cell_update_fee
fc35aa7
The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

Add further comments around fee update handling in channel
623da4d
These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt force-pushed the 2021-08-985-followups branch from 763fc5f to 623da4dCompare September 9, 2021 08:38

TheBlueMatt merged commit b3be420 into lightningdevkit:mainSep 9, 2021

ariard reviewed Aug 16, 2021
View reviewed changes

TheBlueMatt force-pushed the 2021-08-985-followups branch from ce4e20d to dd3097cCompare August 17, 2021 00:31

valentinewallace reviewed Sep 8, 2021
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

TheBlueMatt force-pushed the 2021-08-985-followups branch from dd3097c to 763fc5fCompare September 9, 2021 08:37

TheBlueMatt added 2 commits September 9, 2021 08:37

Update docs for pending_update_fee and holding_cell_update_fee
fc35aa7
The docs were left stale after the logic was updated in lightningdevkit#985 as pointed out in post-merge review.

Add further comments around fee update handling in channel
623da4d
These were suggested to clarify behavior in post-merge review of lightningdevkit#985.

TheBlueMatt force-pushed the 2021-08-985-followups branch from 763fc5f to 623da4dCompare September 9, 2021 08:38

TheBlueMatt merged commit b3be420 into lightningdevkit:mainSep 9, 2021

Uh oh!

Conversation

TheBlueMatt commented Aug 16, 2021

Uh oh!

codecovBot commented Aug 16, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Aug 17, 2021

Uh oh!

TheBlueMatt commented Aug 17, 2021

Uh oh!

ariard commented Aug 21, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TheBlueMatt commented Aug 16, 2021

Uh oh!

codecovBot commented Aug 16, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Aug 17, 2021

Uh oh!

TheBlueMatt commented Aug 17, 2021

Uh oh!

ariard commented Aug 21, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

TheBlueMatt commented Aug 16, 2021

Uh oh!

codecovBot commented Aug 16, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Aug 17, 2021

Uh oh!

TheBlueMatt commented Aug 17, 2021

Uh oh!

ariard commented Aug 21, 2021

Uh oh!

valentinewallace left a comment

codecovBot commented Aug 16, 2021•
edited
Loading

codecovBot commented Aug 16, 2021•
edited
Loading

codecovBot commented Aug 16, 2021•
edited
Loading

codecovBot commented Aug 16, 2021•
edited
Loading