Check for timing-out HTLCs in remote unrevoked commitments #284

TheBlueMatt · 2019-01-07T17:25:56Z

This is based on #282 and #283 as they all work on similar bits of code.

This resolves a TODO/issue in would_broadcast_at_height where we
will not fail a channel with HTLCs which time out in remote
broadcastable transactions.

Note that it requires a good bit of refactoring, sadly, undoing most of the refactoring I did to get #269 through. Luckily it cleans up state tracking in the process, so that's OK.

src/ln/channel.rs

ariard

Reviewed for now until 92d8e53

TheBlueMatt · 2019-01-09T04:48:32Z

Rebased after dependencies got merged.

ariard

Just review this commit, still ongoing work

ariard · 2019-01-10T01:57:09Z

src/ln/functional_tests.rs

@@ -50,11 +50,12 @@ use std::sync::atomic::Ordering;
 use std::time::Instant;
 use std::mem;

+const CHAN_CONFIRM_DEPTH: u32 = 100;


Maybe you could even parameterize it, for coming tracking-claims-confirmation, I need to connect only HTLC_FAIL_ANTI__REORG_DELAY blocks, and more will slow down tests

This is just the "how many blocks confirm_transaction connects" which is "how many blocks do we need to get a funding_locked". Later connects dont need to connect this many blocks (and dont).

My point was we may want to use confirm_transaction for other uses than getting channel confirmation but a macro would be better if I'm lazy enough to write each time the loop confirmation

Ohoh, right, yea, would love to see that, but I'll leave that for when something is ready to start using it.

ariard · 2019-01-10T02:10:51Z

src/ln/functional_tests.rs

+		nodes[1].chain_monitor.block_connected_checked(&header, i, &Vec::new(), &Vec::new());
+		header.prev_blockhash = header.bitcoin_hash();
+	}
+	test_txn_broadcast(&nodes[1], &chan, None, if use_dust { HTLCType::NONE } else { HTLCType::SUCCESS });


Oh interesting ! This means that receiving node isn't incentivized to unilateral close channel as it would lost capability to claim dust HTLCs even being in possession of preimage for them.

Indeed, we currently dont do the optimal thing and check inbound+outbound+dust+non-dust all the same way, which could be tweaked, but its simpler this way and should be fine at least for now.

ariard

Okay, apart of the issue raised elsewhere, seems good to me, it may help further stuff to have recombined HTLCOutputInCommitment and HTLCSource.

Side-notes: it could be nice to have warning comments somewhere on distinction between "virtual offchain HTLC" (dust + non-dust) and "physical onchain HTLC" (non-dust) to ease understanding of newcomers of some code parts. And also maybe have a TODO to write advanced tests of gaming of all different delays.

ariard · 2019-01-10T23:57:18Z

src/ln/channelmonitor.rs

+					//      HTLC_FAIL_TIMEOUT_BLOCKS + 2*CLTV_CLAIM_BUFER <= inbound_cltv - outbound_cltv
+					//      HTLC_FAIL_TIMEOUT_BLOCKS + 2*CLTV_CLAIM_BUFER <= CLTV_EXPIRY_DELTA
+					let htlc_outbound = $local_tx == htlc.offered;
+					if ( htlc_outbound && htlc.cltv_expiry + HTLC_FAIL_TIMEOUT_BLOCKS <= height) ||


nit: space after opening bracket

That was on purpose to match the next line which has a "!" there, but doesnt really matter.

ariard · 2019-01-11T00:20:08Z

src/ln/channelmonitor.rs

+					//      inbound_cltv == height + CLTV_CLAIM_BUFFER
+					//      outbound_cltv + HTLC_FAIL_TIMEOUT_BLOCKS + CLTV_CLAIM_BUFER <= inbound_cltv - CLTV_CLAIM_BUFFER
+					//      HTLC_FAIL_TIMEOUT_BLOCKS + 2*CLTV_CLAIM_BUFER <= inbound_cltv - outbound_cltv
+					//      HTLC_FAIL_TIMEOUT_BLOCKS + 2*CLTV_CLAIM_BUFER <= CLTV_EXPIRY_DELTA


*nit: CLTV_CLAIM_BUFER

oh, you mean the missing F? fixed.

ariard · 2019-01-11T02:17:50Z

src/ln/channelmonitor.rs

+					//  aka outbound_cltv + HTLC_FAIL_TIMEOUT_BLOCKS == height - CLTV_CLAIM_BUFFER
+					//      inbound_cltv == height + CLTV_CLAIM_BUFFER
+					//      outbound_cltv + HTLC_FAIL_TIMEOUT_BLOCKS + CLTV_CLAIM_BUFER <= inbound_cltv - CLTV_CLAIM_BUFFER
+					//      HTLC_FAIL_TIMEOUT_BLOCKS + 2*CLTV_CLAIM_BUFER <= inbound_cltv - outbound_cltv


Okay to be a little bit clearer, maybe you could have a line there : "Or we want outbound_cltv inferior or equal to inbound_cltv" and at the end we have "inbound_ctlv - outbound_cltv " that we set at 72 so have a comment "IMPRESCRIPTIBLE_SAFE_DELAY == 57". Maybe also update stuff in channel_manager.rs L338 CLTV_EXPIRY_DELTA = IMPRESCRIPTIBLE_SAFE_DELAY + HTLC_FAIL_TIMEOUT_BLOCKS + 2 * CLTV_CLAIM_BUFFER or at least its comment.
(Hmm in fact worth have a clean commit on const delay to clarify relations between all of them when things stabilize there)

I'm really confused, where did you get IMPRESCRIPTIBLE_SAFE_DELAY/57? I added another line to the algebra noting the check in ChannelManager is inbound_cltv - outbound_cltv >= CLTV_EXPIRY_DELTA.

CLTV_EXPIRY_DELTA = 6 * 12 (ChannelManager L338)
HTLC_FAIL_TIMEOUT_BLOCKS = 3 (ChannelMonitor L298)
CLTV_CLAIM_BUFFER = 6 (ChannelMonitor L294)
IMPRESCRIPTIBLE_SAFE_DELAY = CLTV_EXPIRY_DELTA - HTLC_FAIL_TIMEOUT_BLOCKS - 2 * CLTV_CLAIM_BUFFER
So with current config values we have IMPRESCRIPTIBLE_SAFE_DELAY == 57 ?

Hmm, I'm still not entire sure what "IMPRESCRIPTABLE" means in this context, and not entirely sure this makes sense as a separate constant? Its just an implied requirement that we have to make sure is true even as we change constants.

From IRC:

<BlueMatt> ariard: I have no idea what "IMPRESCRITABLE" means <ariard> BlueMatt: oh that a term I get from civil law, which means that a right can't be trespass, encroach, compress <ariard> in context, I would to mean that we should be sure that we have a minimal safe delay we can't fuckup we substraction of HTLC_FAIL_TIMEOUT and CLTV_CLAIM_BUFFER <BlueMatt> I mean so why not just leave it as "CHECK_CLTV_EXPIRY_SANITY_2" <BlueMatt> in channelmanager <BlueMatt> we'll already fail compile if that fails <BlueMatt> (cause we'll be trying to set a negative number into a u32 const) <ariard> Ah yes that what CHECK_CLTV_EXPIRY_SANITY is doing, same idea,, well don't have it in mind when I reviewed, maybe add a comment in would_broacast_height which references it too <BlueMatt> k <BlueMatt> is that sufficient or should I do something else before merge? <ariard> All fine, just looked on your last comments, nothing to add

ariard · 2019-01-11T02:46:11Z

src/ln/channel.rs

 			}
 		}
+		let non_dust_htlc_count = htlcs_included.len();
+		htlcs_included.append(&mut unincluded_htlc_sources);


nit: hey if you append unincluded_htlc does htlcs_included is still meaningful?

Yea, that was unclear. I guess my change to call "unincluded_htlc_sources"->"included_dust_htlcs" makes it more clear? I consider dust HTLCs "included" since they effect the to_self/remote outputs and are distinct from HTLCs which were not included due to being in a state where we dont care about them.

Ah okay if you consider them included because of them being part of miner fee maybe add a hint about it in function comment.

Added a comment describing return value to build_commitment_transaction, which was completely absent previously.

We really shouldn't have split out the with-source HTLCs from the in-transaction HTLCs when we added back-failing, and will need almost all of the info in HTLCOutputInCommitment for each HTLC to fix would_broadcast_at_height, so this is a first step at recombining them.

This simplifies a few things, deduplicates a some small memory overhead, and, most importantly, is a first step to fixing would_broadcast_at_height.

This resolves a TODO/issue in would_broadcast_at_height where we will not fail a channel with HTLCs which time out in remote broadcastable transactions.

TheBlueMatt force-pushed the 2019-01-remote-htlc-timeout-broadcast branch 5 times, most recently from 898433c to ad238e2 Compare January 7, 2019 19:02

ariard reviewed Jan 9, 2019

View reviewed changes

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

ariard reviewed Jan 9, 2019

View reviewed changes

TheBlueMatt force-pushed the 2019-01-remote-htlc-timeout-broadcast branch from 188a3fa to 70499da Compare January 9, 2019 04:48

ariard reviewed Jan 10, 2019

View reviewed changes

ariard approved these changes Jan 11, 2019

View reviewed changes

TheBlueMatt force-pushed the 2019-01-remote-htlc-timeout-broadcast branch from 70499da to 4156afd Compare January 11, 2019 21:40

TheBlueMatt added 3 commits January 13, 2019 12:59

Merge tracking of HTLCs-in-commitment with outbound-HTLCs

92424eb

This simplifies a few things, deduplicates a some small memory overhead, and, most importantly, is a first step to fixing would_broadcast_at_height.

Test for failing channel after local commitment dust HTLC timeout

78939a7

Check for timing-out HTLCs in remote unrevoked commitments

d8707c2

This resolves a TODO/issue in would_broadcast_at_height where we will not fail a channel with HTLCs which time out in remote broadcastable transactions.

TheBlueMatt force-pushed the 2019-01-remote-htlc-timeout-broadcast branch from 4156afd to 54f112c Compare January 13, 2019 17:59

TheBlueMatt mentioned this pull request Jan 13, 2019

Fix several more monitor-update-failed cases #288

Merged

Fix comment spelling and clarify algebra a bit.

f065a62

TheBlueMatt force-pushed the 2019-01-remote-htlc-timeout-broadcast branch from 54f112c to f065a62 Compare January 13, 2019 18:12

TheBlueMatt merged commit 301f91e into lightningdevkit:master Jan 13, 2019

Check for timing-out HTLCs in remote unrevoked commitments #284

Check for timing-out HTLCs in remote unrevoked commitments #284

Uh oh!

Conversation

TheBlueMatt commented Jan 7, 2019

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jan 9, 2019

Uh oh!

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

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

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!

Uh oh!

ariard left a comment •

edited

Loading