BOLT2: Check we don't send and accept 0-msat HTLC #513

ariard · 2020-02-21T00:27:05Z

Failing this requirement at sending means a strict receiver would
fail our channel while processing at HTLC routed from a third-party.

Fix by enforcing check on both sender and receiver side.

BOLT2:

A sending node MUST offer amount_msat greater than 0.

A receiving node receiving an amount_msat equal to 0, OR less than its own htlc_minimum_msat SHOULD fail the channel.

TheBlueMatt

Awesome, thanks for tackling this. Should pass travis if you rebase on master with #514.

TheBlueMatt · 2020-02-21T01:54:56Z

lightning/src/ln/channel.rs

@@ -1597,6 +1597,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		if msg.amount_msat > self.channel_value_satoshis * 1000 {
 			return Err(ChannelError::Close("Remote side tried to send more than the total value of the channel"));
 		}
+		if msg.amount_msat == 0 {


Would be nice to test this line as well. Another way to do it that would be easier to test would be to just set our_htlc_minimum_msat to at least 1 always.

Add a commit for this, without modifying default config

codecov · 2020-02-26T21:05:19Z

Codecov Report

Merging #513 into master will increase coverage by 0.16%.
The diff coverage is 98.18%.

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   90.07%   90.24%   +0.16%     
==========================================
  Files          34       34              
  Lines       19053    19197     +144     
==========================================
+ Hits        17162    17324     +162     
+ Misses       1891     1873      -18

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.35% <97.87%> (+0.05%)`	⬆️
lightning/src/ln/channel.rs	`86.95% <100.00%> (+0.04%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`94.59% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/config.rs	`69.04% <100.00%> (+0.75%)`	⬆️
lightning/src/ln/features.rs	`96.87% <0.00%> (+0.25%)`	⬆️
lightning/src/ln/msgs.rs	`89.72% <0.00%> (+1.61%)`	⬆️
lightning/src/ln/wire.rs	`65.85% <0.00%> (+11.30%)`	⬆️

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 83c9eb4...dd9c476. Read the comment docs.

ariard · 2020-02-26T21:06:27Z

Rebased 91a9321

TheBlueMatt · 2020-02-26T23:07:24Z

Looks like this needs a further rebase on top of #509.

ariard · 2020-02-28T17:02:32Z

Rebased 3d6b2d9

TheBlueMatt · 2020-02-28T20:04:43Z

lightning/src/ln/channel.rs

@@ -1652,6 +1652,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		if msg.amount_msat > self.channel_value_satoshis * 1000 {
 			return Err(ChannelError::Close("Remote side tried to send more than the total value of the channel"));
 		}
+		if msg.amount_msat == 0 {
+			return Err(ChannelError::Ignore("Remote side tried to send a 0-msat HTLC"));


I dont think this is right? We should either ::Close() or we should add the HTLC and fail it backwards as soon as they commit.

You right, should fail the channel IIRC the spec, will update

ariard · 2020-03-03T20:06:01Z

Updated at cf2918f with rebase errors fixed, ::Ignore() switched for a ::Close() and receiver-test updated to check for channel closing.

TheBlueMatt · 2020-03-06T02:25:59Z

I believe this needs further updates (and a rebase onto master to make travis pass).

Failing this requirement at sending means a strict receiver would fail our channel while processing a HTLC routed from a third-party. Fix by enforcing check on both sender and receiver side.

ariard · 2020-03-10T17:21:29Z

Rebased 16edc6d

TheBlueMatt · 2020-03-10T17:54:54Z

Doesn't build.

TheBlueMatt · 2020-03-10T20:56:40Z

As discussed, we should never be sending an htlc_minimum_msat of < 1 to any of our peers, so this needs an update to check for that.

TheBlueMatt

One doc change suggestion but looks good.

TheBlueMatt · 2020-03-11T16:38:41Z

lightning/src/util/config.rs

+	/// Announced in `open_channel`/`accept_channel` according if it's an inbound/outbound
+	/// channel. Enforced at `update_add_htlc` reception with other HTLC sanitization checks.


Suggested change

/// Announced in `open_channel`/`accept_channel` according if it's an inbound/outbound

/// channel. Enforced at `update_add_htlc` reception with other HTLC sanitization checks.

/// This value is sent to our counterparty on channel-open and we close the channel any time

/// our counterparty misbehaves by sending us an HTLC with a value smaller than this.

///

/// If the value is less than 1, it is ignored and set to 1, as is required by the protocol.

Thanks, took your suggestion

Enforce a minimum htlc_minimum_msat of 1. Instead of computing dynamically htlc_minimum_msat based on feerate, relies on user-provided configuration value. This let user compute an economical-driven channel parameter according to network dynamics.

TheBlueMatt reviewed Feb 21, 2020

View reviewed changes

ariard force-pushed the 2020-02-fix-zero-msat-htlc branch from b1cd4fc to 91a9321 Compare February 26, 2020 21:06

ariard force-pushed the 2020-02-fix-zero-msat-htlc branch from 91a9321 to 3d6b2d9 Compare February 28, 2020 17:02

TheBlueMatt reviewed Feb 28, 2020

View reviewed changes

ariard force-pushed the 2020-02-fix-zero-msat-htlc branch from 3d6b2d9 to cf2918f Compare March 3, 2020 20:04

Antoine Riard added 2 commits March 10, 2020 13:05

BOLT2: Check we don't send and accept 0-msat HTLC

d1c6f23

Failing this requirement at sending means a strict receiver would fail our channel while processing a HTLC routed from a third-party. Fix by enforcing check on both sender and receiver side.

Add test_update_add_htlc_bolt2_receiver_zero_value_msat

4abfd51

ariard force-pushed the 2020-02-fix-zero-msat-htlc branch from cf2918f to 16edc6d Compare March 10, 2020 17:21

ariard force-pushed the 2020-02-fix-zero-msat-htlc branch from 16edc6d to 43d7ceb Compare March 10, 2020 20:42

ariard force-pushed the 2020-02-fix-zero-msat-htlc branch from 43d7ceb to fe5200d Compare March 10, 2020 23:31

TheBlueMatt approved these changes Mar 11, 2020

View reviewed changes

Antoine Riard added 2 commits March 11, 2020 14:28

Make htlc_minimum_msat configurable

9e03d2b

Enforce a minimum htlc_minimum_msat of 1. Instead of computing dynamically htlc_minimum_msat based on feerate, relies on user-provided configuration value. This let user compute an economical-driven channel parameter according to network dynamics.

Add test_override_0msat_htlc_minimum

dd9c476

ariard force-pushed the 2020-02-fix-zero-msat-htlc branch from fe5200d to dd9c476 Compare March 11, 2020 18:30

TheBlueMatt approved these changes Mar 11, 2020

View reviewed changes

TheBlueMatt merged commit d6b1825 into lightningdevkit:master Mar 11, 2020

TheBlueMatt mentioned this pull request Mar 11, 2020

Add missing unwrap() in tests introduced in 4abfd515e554bef90c25dbe #543

Merged

		/// Announced in `open_channel`/`accept_channel` according if it's an inbound/outbound
		/// channel. Enforced at `update_add_htlc` reception with other HTLC sanitization checks.

-	/// Announced in `open_channel`/`accept_channel` according if it's an inbound/outbound
-	/// channel. Enforced at `update_add_htlc` reception with other HTLC sanitization checks.
+	/// This value is sent to our counterparty on channel-open and we close the channel any time
+	/// our counterparty misbehaves by sending us an HTLC with a value smaller than this.
+	///
+	/// If the value is less than 1, it is ignored and set to 1, as is required by the protocol.

BOLT2: Check we don't send and accept 0-msat HTLC #513

BOLT2: Check we don't send and accept 0-msat HTLC #513

Uh oh!

Conversation

ariard commented Feb 21, 2020

Uh oh!

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

TheBlueMatt Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

ariard Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ariard commented Feb 26, 2020

Uh oh!

TheBlueMatt commented Feb 26, 2020

Uh oh!

ariard commented Feb 28, 2020

Uh oh!

TheBlueMatt Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

ariard Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

ariard commented Mar 3, 2020

Uh oh!

TheBlueMatt commented Mar 6, 2020

Uh oh!

ariard commented Mar 10, 2020

Uh oh!

TheBlueMatt commented Mar 10, 2020

Uh oh!

TheBlueMatt commented Mar 10, 2020

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

ariard Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt left a comment •

edited

Loading

codecov bot commented Feb 26, 2020 •

edited

Loading