Don't include below-dust inbound HTLCs in commit tx fee calculation #762

valentinewallace · 2020-11-29T00:49:02Z

Closes #757

Does this fix look right? Seems like an oops from the original channel reserve PR. Let me know and I'll write tests.

codecov · 2020-11-29T00:58:45Z

Codecov Report

Merging #762 (fc784f4) into main (de58bcf) will increase coverage by 0.03%.
The diff coverage is 95.37%.

@@            Coverage Diff             @@
##             main     #762      +/-   ##
==========================================
+ Coverage   90.76%   90.80%   +0.03%     
==========================================
  Files          44       44              
  Lines       24282    24466     +184     
==========================================
+ Hits        22040    22216     +176     
- Misses       2242     2250       +8

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	`87.70% <94.68%> (+0.44%)`	⬆️
lightning/src/ln/functional_tests.rs	`96.99% <100.00%> (+0.06%)`	⬆️
lightning/src/ln/channelmanager.rs	`84.90% <0.00%> (-0.10%)`	⬇️

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 de58bcf...1079753. Read the comment docs.

TheBlueMatt · 2020-11-29T19:19:11Z

This looks good to me. I'll let the fuzzer spin on this + 756 for a bit and see what it thinks.

TheBlueMatt · 2020-11-30T14:48:27Z

Fuzzers didn't find anything after 15 CPU-days, so probably at least fixes that failing case. Obviously can't rely on fuzzers so should add an explicit test (yay lightning/src/ln/chanmon_update_fail_tests.rs), but otherwise looks good.

valentinewallace · 2020-12-04T21:12:07Z

Still needs a bit more testing but I pushed up one test and an additional fix (the fix being: when calculating the commit tx fee, don't include the HTLC currently being added if it's below dust).

TheBlueMatt · 2020-12-11T21:16:14Z

lightning/src/ln/channel.rs

-		let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
+		let mut addl_htlcs = 1;
+		if fee_spike_buffer_htlc.is_some() { addl_htlcs += 1; }
+		if new_htlc_amount / 1000 <= self.counterparty_dust_limit_satoshis {


Should the counterparty/holder dust limit references in this PR be swapped? If its easy, would be nice to get a test which would catch such an inversion.

valentinewallace · 2020-12-16T19:55:18Z

Found some more missing pieces so I'm restructuring to reuse some of the logic in build_commitment_transaction. Will update when this is ready for review again.

TheBlueMatt · 2020-12-16T21:33:42Z

If you're refactoring bits of build_commitment_transaction you may want to build on/anticipate #742.

TheBlueMatt · 2020-12-17T20:11:24Z

Is there a way we can do a more thorough test of this? eg in cfg(test) store the latest value here and when we call build_commitment_transaction check that the latest value matches? It may require a bunch of "do we need to check here" cases, but if its not too bad it may be worth it.

valentinewallace · 2020-12-21T18:15:11Z

Is there a way we can do a more thorough test of this? eg in cfg(test) store the latest value here and when we call build_commitment_transaction check that the latest value matches? It may require a bunch of "do we need to check here" cases, but if its not too bad it may be worth it.

That makes sense, currently trying to implement this!

valentinewallace · 2020-12-22T21:33:38Z

Is there a way we can do a more thorough test of this? eg in cfg(test) store the latest value here and when we call build_commitment_transaction check that the latest value matches? It may require a bunch of "do we need to check here" cases, but if its not too bad it may be worth it.

Ok, I believe I've approximately added this testing.

TheBlueMatt · 2020-12-28T18:38:58Z

We should probably enable the new fee caching stuff in fuzztarget, too.

TheBlueMatt · 2020-12-28T18:44:28Z

Note that the new tests, with just the cfg changed to run in fuzztarget, fail on at least 2c090d2c2c2c340c13fffe039f42f3e7.

TheBlueMatt · 2021-01-27T16:04:38Z

Ah, I take that back, the full_stack_target failure definitely looks like a bug either in the new tests or in the new logic.

valentinewallace · 2021-01-28T17:08:11Z

Note that the new tests, with just the cfg changed to run in fuzztarget, fail on at least 2c090d2c2c2c340c13fffe039f42f3e7.

OK I believe I've fixed this and full_stack_target

TheBlueMatt · 2021-02-03T16:59:29Z

It looks like 2a441414192a441414190a0a2a441414190d61c9de5c021d is failing chanmon_consistency.

valentinewallace · 2021-02-04T19:22:09Z

It looks like 2a441414192a441414190a0a2a441414190d61c9de5c021d is failing chanmon_consistency.

I think my current approach in 076137d causes too many bespoke exceptions to be necessary. Gonna try to rework it.

TheBlueMatt · 2021-02-04T19:28:22Z

Right, I was kinda afraid of that. I'm mostly OK with a ton of exceptions in #[cfg(test)] blocks, but if you can find a simpler way that would be cool too.

lightning/src/ln/channel.rs

valentinewallace · 2021-02-05T20:01:34Z

Right, I was kinda afraid of that. I'm mostly OK with a ton of exceptions in #[cfg(test)] blocks, but if you can find a simpler way that would be cool too.

Lmk what you think of the new approach.

TheBlueMatt · 2021-02-06T00:21:22Z

Lmk what you think of the new approach.

Right, much better, thanks! I am a little worried about the "info.num_htlcs == counterparty_commitment_tx.1" checks neutering some of the value - isn't part of the reason for the checks to make sure we have the same number of HTLCs as we thought we would? Is there a way to take a bit of both approaches and maybe just drop that check and then reset the states on RAA or commitment_signed messages?

valentinewallace · 2021-02-08T22:01:50Z

Lmk what you think of the new approach.

Right, much better, thanks! I am a little worried about the "info.num_htlcs == counterparty_commitment_tx.1" checks neutering some of the value - isn't part of the reason for the checks to make sure we have the same number of HTLCs as we thought we would? Is there a way to take a bit of both approaches and maybe just drop that check and then reset the states on RAA or commitment_signed messages?

Adding those specific checks^ still had test failures. I agree the info.num_htlcs == counterparty_commitment_tx.1 did neuter some of the value though, so I've updated it to instead make sure the total number of pending HTLCs is the same. I think this should capture the full value of the check, but lmk if I'm off on that.

TheBlueMatt

Right. Looks good to me! I added one note about some manual mutation testing that didn't get caught, if its easy would be nice, but if its a pain I'm ok without.

lightning/src/ln/channel.rs

jkczyz · 2021-02-17T02:37:40Z

lightning/src/ln/channel.rs

+						&& info.next_holder_htlc_id == self.next_holder_htlc_id
+						&& info.next_counterparty_htlc_id == self.next_counterparty_htlc_id
+						&& info.feerate == self.feerate_per_kw {
+							assert_eq!(total_fee, info.fee / 1000);


Is there any way to write a test that checks that these agree instead of adding to the production code (albeit, only conditionally for tests). Generally, writing a test would be preferable if it is possible.

I think the point of this block is more to check it in fuzz/functional tests than one-off. Would be great to get the test code our of channel.rs, I agree, but not sure how to do that in the fuzzer/functional tests.

lightning/src/ln/channel.rs

Also remove part of the holding cell channel reserve test that's newly failing but a bit of a redundant test anyway.

ariard

Post-merge Code Review ACK 1079753

Overall, a call for better comments given how much off-chain computation is tricky. Accounting a HTLC for fee computation is function of its state, and even if we follow the most sound reasoning IMO, I wouldn't be surprised that other implementation have weird recipes ("What if they have another definition of AwaitingRemoteRevokeToRemove ?"), thus leading to unexpected closure in case of edge-cases. Good documentation would help track down that kind of cases if they show up.

I think I've also found few test coverage holes.

ariard · 2021-02-22T20:00:58Z

lightning/src/ln/channel.rs

 		assert!(self.is_outbound());

-		let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
+		let real_dust_limit_success_sat = (self.feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;


I think it would be great to document what does real dust_limit means, it's quite buried in BOLT3. I even think the spec should scope the HTLC output fee contribution to the commitment one, but it doesn't seem to be recommended anywhere...

ariard · 2021-02-22T20:23:09Z

lightning/src/ln/channel.rs

+				OutboundHTLCState::RemoteRemoved {..} => included_htlcs += 1,
+				// We don't include AwaitingRemoteRevokeToRemove HTLCs because our next commitment
+				// transaction won't be generated until they send us their next RAA, which will mean
+				// dropping any HTLCs in this state.


I think this is correct, sequence where you might have HTLC in AwaitingRemoteRevokeToRemote are either us sending update_fail/success_htlc, commitment_signed and yet trying to send one more HTLC or receive an update from remote.

Note, "dropping any HTLCs in this state" doesn't mean this HTLC won't materialize onchain. Receiving counterparty RAA doesn't guarantee then to receive a commitment_signedwith such HTLC effectively removed for holder commitment tx.

ariard · 2021-02-22T20:40:24Z

lightning/src/ln/channel.rs

+	// Get the commitment tx fee for the remote's next commitment transaction based on the number of
+	// pending HTLCs that are on track to be in their next commitment tx, plus an additional HTLC if
+	// `fee_spike_buffer_htlc` is Some, plus a new HTLC given by `new_htlc_amount`. Dust HTLCs are
+	// excluded.


May we prefix mention of dust with "economically" or something like this to mark the difference with Core' s notion of IsDust to evaluate a transaction standardness ?

ariard · 2021-02-22T20:48:45Z

lightning/src/ln/channel.rs

-			// +1 for this HTLC.
-			self.next_remote_commit_tx_fee_msat(1)
+			let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
+			self.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations


No test coverage, mutating here

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index d3a372e4..b7f5d6a2 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1969,7 +1969,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> { // feerate_per_kw, while maintaining their channel reserve (as required by the spec). let remote_commit_tx_fee_msat = if self.is_outbound() { 0 } else { let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered); - self.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations + self.next_remote_commit_tx_fee_msat(htlc_candidate, Some(())) // Don't include the extra fee spike buffer HTLC in calculations }; if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat { return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));

ariard · 2021-02-22T20:56:51Z

lightning/src/ln/channel.rs

-				OutboundHTLCState::RemoteRemoved {..} => their_acked_htlcs += 1,
+				OutboundHTLCState::Committed => included_htlcs += 1,
+				OutboundHTLCState::RemoteRemoved {..} => included_htlcs += 1,
+				OutboundHTLCState::LocalAnnounced { .. } => included_htlcs += 1,


No test coverage, mutating here

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index d3a372e4..6751b973 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1871,7 +1871,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> { match htlc.state { OutboundHTLCState::Committed => included_htlcs += 1, OutboundHTLCState::RemoteRemoved {..} => included_htlcs += 1, - OutboundHTLCState::LocalAnnounced { .. } => included_htlcs += 1, + OutboundHTLCState::LocalAnnounced { .. } => {}, //included_htlcs += 1, _ => {}, } }

ariard · 2021-02-22T21:08:25Z

lightning/src/ln/channel.rs

 		for ref htlc in self.pending_outbound_htlcs.iter() {
-			if htlc.amount_msat / 1000 <= self.counterparty_dust_limit_satoshis {
+			if htlc.amount_msat / 1000 <= real_dust_limit_success_sat {


I think it would be great to comment this subtlety around which dust limit apply in function of which viewpoint the fee is computed for.

"When computing the fee as paid by counterparty, if they have to claim onchain HTLC by broadcasting their commitment, they will have to spend inbound through a HTLC-timeout, outbound through a HTLC-Success.
When computing the fee as paid by holder, if we have to claim onchain HTLC by broadcasting our commitment, we will have to spend inbound through a HTLC-success, outbound through a HTLC-timeout"

Well, that's an attempt, but we should make it clear that even if our channel variables represent the holder direction-side (pending_inbound_htlcs/pending_outbound_htlcs) the actual fee cost is function from which commitment they will be claim.

valentinewallace · 2021-02-24T17:50:41Z

Thanks for the post-merge review @ariard, will attempt to address soon^tm

TheBlueMatt added this to the 0.0.13 milestone Dec 3, 2020

valentinewallace force-pushed the chan-reserve-fuzz-failure branch 4 times, most recently from fa57462 to 84149fd Compare December 4, 2020 21:09

valentinewallace force-pushed the chan-reserve-fuzz-failure branch from 84149fd to 1faf0da Compare December 5, 2020 20:53

valentinewallace marked this pull request as ready for review December 7, 2020 20:08

TheBlueMatt reviewed Dec 11, 2020

View reviewed changes

valentinewallace force-pushed the chan-reserve-fuzz-failure branch from 1faf0da to 15a92be Compare December 16, 2020 16:53

valentinewallace force-pushed the chan-reserve-fuzz-failure branch from 15a92be to f165fca Compare December 16, 2020 23:51

valentinewallace force-pushed the chan-reserve-fuzz-failure branch from f165fca to 068543e Compare December 22, 2020 21:30

devrandom mentioned this pull request Dec 27, 2020

Meeting Minutes #717

Closed

valentinewallace force-pushed the chan-reserve-fuzz-failure branch 2 times, most recently from ba5ea8c to 5754f42 Compare January 27, 2021 01:58

valentinewallace force-pushed the chan-reserve-fuzz-failure branch 2 times, most recently from 3b02a56 to 076137d Compare January 27, 2021 23:39

valentinewallace force-pushed the chan-reserve-fuzz-failure branch from 076137d to e6a3fe3 Compare February 5, 2021 18:09

valentinewallace commented Feb 5, 2021

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

valentinewallace force-pushed the chan-reserve-fuzz-failure branch from e6a3fe3 to 3b6000d Compare February 8, 2021 21:57

TheBlueMatt approved these changes Feb 9, 2021

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

valentinewallace force-pushed the chan-reserve-fuzz-failure branch from 137346c to ebf5a06 Compare February 9, 2021 20:16

jkczyz reviewed Feb 17, 2021

View reviewed changes

valentinewallace force-pushed the chan-reserve-fuzz-failure branch from ebf5a06 to d6860ec Compare February 17, 2021 21:28

jkczyz approved these changes Feb 18, 2021

View reviewed changes

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

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

valentinewallace force-pushed the chan-reserve-fuzz-failure branch 2 times, most recently from 2e4297b to 2cc8aa7 Compare February 18, 2021 18:09

valentinewallace added 2 commits February 18, 2021 13:09

Don't include below-dust inbound HTLCs in commit tx fee calculation

3d4735c

Also remove part of the holding cell channel reserve test that's newly failing but a bit of a redundant test anyway.

Ensure build_commitment_tx and next_local/remote_tx_fee agree on the fee

1079753

valentinewallace force-pushed the chan-reserve-fuzz-failure branch from 2cc8aa7 to 1079753 Compare February 18, 2021 18:09

TheBlueMatt merged commit 13e990e into lightningdevkit:main Feb 19, 2021

ariard reviewed Feb 22, 2021

View reviewed changes

TheBlueMatt mentioned this pull request Feb 26, 2021

HTLC Commitment Tx Fee Prediction Tests Could use more coverage #809

Open

Don't include below-dust inbound HTLCs in commit tx fee calculation #762

Don't include below-dust inbound HTLCs in commit tx fee calculation #762

Uh oh!

Conversation

valentinewallace commented Nov 29, 2020

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Nov 29, 2020

Uh oh!

TheBlueMatt commented Nov 30, 2020

Uh oh!

valentinewallace commented Dec 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Dec 16, 2020

Uh oh!

TheBlueMatt commented Dec 16, 2020

Uh oh!

TheBlueMatt commented Dec 17, 2020

Uh oh!

valentinewallace commented Dec 21, 2020

Uh oh!

valentinewallace commented Dec 22, 2020

Uh oh!

TheBlueMatt commented Dec 28, 2020

Uh oh!

TheBlueMatt commented Dec 28, 2020

Uh oh!

TheBlueMatt commented Jan 27, 2021

Uh oh!

valentinewallace commented Jan 28, 2021

Uh oh!

TheBlueMatt commented Feb 3, 2021

Uh oh!

valentinewallace commented Feb 4, 2021

Uh oh!

TheBlueMatt commented Feb 4, 2021

Uh oh!

Uh oh!

valentinewallace commented Feb 5, 2021

Uh oh!

TheBlueMatt commented Feb 6, 2021

Uh oh!

valentinewallace commented Feb 8, 2021 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ariard left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Feb 24, 2021

Uh oh!

Uh oh!

codecov bot commented Nov 29, 2020 •

edited

Loading

valentinewallace commented Feb 8, 2021 •

edited

Loading

ariard left a comment •

edited

Loading