Fixes for HTLC db issues #2000

rustyrussell · 2018-10-09T05:40:20Z

Puzzled about two PR's, I wrote a stress test, which revealed a number of bugs, including those which caused the horrific issues #1960 and #1993.

It also triggers a yet-to-be-diagnosed occasional issue where we dislike the peer's option_dataloss_protect; I have temporarily disabled that check in order to resolve these more important issues, but am now chasing that one.

Fixes: #1960
Fixes: #1993

… is True This is what the callers want; generalize it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: Outstanding taken pointers: lightningd/pay.c:243:channel_update Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This means we need to check when we've altered the state, so the checks are moved to the callers of htlc_in_update_state and htlc_out_update_state. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…utgoing. Usually, we only close an incoming HTLC once the outgoing HTLC is completely resolved. However, we short-cut this in the FULFILL case: we have the preimage, so might as well use it immediately (in fact, we wait for it to be committed, but we don't need to in theory). As a side-effect of this, our assumption that every outgoing HTLC has a corresponding incoming HTLC is incorrect, and this test (xfail) tickles that corner case. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

… outgoing. We now need an explicit 'local' flag, rather than relying on the existence of the 'in' pointer. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…reed. Now we know this can happen (see previous patch), we need to handle it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Which we don't handle, due to a separate bug, so it's xfail. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…hout a preimage. We need to handle this case (old db) before the next commit, which actually fixes it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We can now wrap the 'missing preimage' hack in COMPAT_V061. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Under stress, it fails (test_restart_many_payments, the next test). I suspect a deep misunderstanding in the comparison code, will chase separately. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We don't save them to the database, so fix things up as we load them. Next patch will actually save them into the db, and this will become COMPAT code. Also: call htlc_in_check() with NULL on db load, as otherwise it aborts internally. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We don't write it in there yet, so this change currently has no effect. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Now we can finally move the fixup code under COMPAT_V061, so it's only for old nodes. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

failoutchannel tells us which channel to send an update for (specifically for temporary_channel_failure); but we don't save it into the db. It's not even clear we should, since it's a corner case and the channel might not even exist when we come back. So on db restore, change such errors to WIRE_TEMPORARY_NODE_FAILURE which doesn't need an update. We also don't memset it to 0 in the normal case (we only access if it failcode has the UPDATE bit set) so valgrind will trigger if we're wrong. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

We don't expect payment or payment->route_channels to be NULL without an old db, but putting an assert there reveals that we try to fail an HTLC which has already succeeded in 'test_onchain_unwatch'. Obviously we only want to fail an HTLC which goes onchain if we don't already have the preimage! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker

Everything looks ok from my side, just some minor nits about naming and comments. The only thing that might take some time, but I think is a nice cleanup, is to remove the preimage from hout altogether and making the state an explicit enum field, to make it easier to reason about the states that an HTLC may be in.

ACK 362fe44

cdecker · 2018-10-09T10:40:29Z

tests/utils.py

+        # Make sure everyone sees all channels: we can cheat and
+        # simply check the ends (since it's a line).
+        wait_for(lambda: both_dirs_ready(nodes[0], scids[-1]))
+        wait_for(lambda: both_dirs_ready(nodes[-1], scids[0]))


Nice simplification

cdecker · 2018-10-09T10:42:56Z

tests/utils.py


        if not announce:
            return nodes

        bitcoin.generate_block(5)
+
+        def both_dirs_ready(n, scid):


This will take a really long time without DEVELOPER=1, since we can't set the broadcast interval, so each hop adds 30 wait time this way. But it seems we mostly wait anyway (or have a 2-line-graph which does not suffer this problem).

cdecker · 2018-10-09T11:08:03Z

tests/test_connection.py

+
+    # Now, l2 should restore from DB fine, even though outgoing HTLC no longer
+    # has an incoming.
+    l2.restart()


Technically restarting does not trigger a reconnect, and it's not needed to check the HTLC restore from the database. It might be interesting nevertheless to have them reconnect and check that they clear correctly, not just load correctly from the DB. A bunch of wait_fors should suffice to see the HTLC settle correctly.

Covered in 4f42de2

cdecker · 2018-10-09T11:11:24Z

lightningd/htlc_end.h

@@ -73,6 +73,9 @@ struct htlc_out {
 	/* If we fulfilled, here's the preimage. */
 	struct preimage *preimage;

+	/* Is this local?  Implies ->in is NULL. */
+	bool local;


We're already using local in the context of a single channel quite extensively, could we name this origin or sender instead? I'd like to separate the nomenclatures of channel context from the multi-channel context a bit more, to avoid confusion.

cdecker · 2018-10-09T11:27:50Z

lightningd/peer_htlcs.c

+			/* FIXME: COMPAT_V061 only */
+			log_broken(ld->log,
+				   "Missing preimage for orphaned HTLC; replacing with zeros");
+			hout->preimage = talz(hout, struct preimage);


It appears we are using the preimage solely to test for success and to pass it to the payment. So it is never used on the downstream side, only on the upstream side (towards the sender), so why are we keeping this at all?

I might have been a bit overzealous in saving it on both sides, when it could just have been part of the state. This might be part of a bigger PR that drops the preimage from hout, but it'd allow us to avoid a compat flag here (and subsequent forced global reload).

I'll add a FIXME.

cdecker · 2018-10-09T11:34:58Z

tests/test_connection.py

+    # Manually create routes, get invoices
+    route = []
+    payment_hash = []
+    for i in range(len(innodes)):


Having a tuple of (route, hash) seems to be preferable here. it'd make the unpacking for loop below easier.

Also since we are iterating over innodes we could extend this to be (innode, route, hash) making the loop really trivial below.

in the spirit of pythonicity, this is a bit nicer:

import itertools for inode, outnode, outchan, inchan in itertools.izip(innodes,outnodes,outchans,inchans):

via https://stackoverflow.com/a/6498180

cdecker · 2018-10-09T11:37:55Z

tests/test_connection.py

+
+    # sendpay is async.
+    for i in range(len(payment_hash)):
+        innodes[i // 2].rpc.sendpay(route[i], payment_hash[i])


The [i // 2] seems strange here, shouldn't we iterate over all innodes not just the first half?

There are two payments per innode...

cdecker · 2018-10-09T11:52:43Z

wallet/wallet.c

@@ -1346,6 +1346,34 @@ static bool wallet_stmt2htlc_out(struct channel *channel,
 	return ok;
 }

+/* We didn't used to save failcore, failuremsg... */


nit: "didn't used to"

cdecker · 2018-10-09T12:02:22Z

wallet/wallet.c

@@ -1378,19 +1385,20 @@ static void fixup_hin(struct wallet *wallet, struct htlc_in *hin)
 	if (hin->failcode || hin->failuremsg)
 		return;

-	hin->failcode = WIRE_TEMPORARY_CHANNEL_FAILURE;
+	hin->failcode = WIRE_TEMPORARY_NODE_FAILURE;


nit: This and the next change could be added to the commit that added this fix in the first place.

Yes, agreed. I only realized it when I found that issue though...

cdecker · 2018-10-09T12:02:44Z

wallet/wallet.c

 #endif
+}
+


nit: extra newline

I'll kill that, too.

@cdecker

Noted by @cdecker, the term 'local' is grossly overused, and the hout preimage is basically only used as a sanity check (though I've just put a FIXME there for now). Also eliminated spurious blank line which crept into wallet.c. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2018-10-09T20:41:55Z

Awesome, thanks for the fixes @rustyrussell

ACK 14e2798

The merge request should disappear when doing a Rebase and merge, hopefully 🤞

niftynei · 2018-10-09T21:10:31Z

tests/test_connection.py

+    # We agree on fee change first, then add HTLC, then remove; stop after remove.
+    disconnects = ['+WIRE_COMMITMENT_SIGNED*3']
+    # We manually reconnect l2 & l3, after 100 blocks; hence allowing manual
+    # reconnect, but disabling auto coneect, and massive cltv so 2/3 doesn't


nit: coneect -> connect

I had to rebase anyway due to CHANGELOG conflict, so I snuck this in too... thanks!

niftynei · 2018-10-09T21:58:16Z

lightningd/htlc_end.h

 	struct preimage *preimage;

-	/* Is this local?  Implies ->in is NULL. */
-	bool local;
+	/* Is this a locally-generated payment?  Implies ->in is NULL. */


This is much helpful 👍

niftynei · 2018-10-09T22:09:09Z

tests/test_connection.py

+    # Manually create routes, get invoices
+    route = []
+    payment_hash = []
+    for i in range(len(innodes)):


in the spirit of pythonicity, this is a bit nicer:

import itertools for inode, outnode, outchan, inchan in itertools.izip(innodes,outnodes,outchans,inchans):

via https://stackoverflow.com/a/6498180

@cdecker

IIUC, namedtuple is like tuple for grown-ups: Pythonify! Suggested-by: @cdecker Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-1960 branch from 2e629ae to 362fe44 Compare October 9, 2018 06:30

pytest: make line_graph wait for gossip propagation if announce param…

9160ef1

… is True This is what the callers want; generalize it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell added bug testing forwarding labels Oct 9, 2018

rustyrussell added 17 commits October 9, 2018 19:12

Makefile: our first db-from-0.6.1 bugfix is coming.

3b7b61d

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: fix outstanding taken pointer.

6306021

lightningd: Outstanding taken pointers: lightningd/pay.c:243:channel_update Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: beef up HTLC consistency checking.

ba8c0c7

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: even more HTLC consistency checking: check states.

ef10bae

This means we need to check when we've altered the state, so the checks are moved to the callers of htlc_in_update_state and htlc_out_update_state. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: handle case where incoming HTLC vanished before fulfilled…

ecbbdd9

… outgoing. We now need an explicit 'local' flag, rather than relying on the existence of the 'in' pointer. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: don't leave htlc_out's in pointer dangling when htlc_in f…

8188e88

…reed. Now we know this can happen (see previous patch), we need to handle it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

pytest: extend the test_fulfill_incoming_first case to cover reconnect.

dd87cf2

Which we don't handle, due to a separate bug, so it's xfail. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: handle the case where the db contains a resolved HTLC wit…

553f90f

…hout a preimage. We need to handle this case (old db) before the next commit, which actually fixes it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd: save outgoing HTLC's preimage to db.

f8b7ecc

We can now wrap the 'missing preimage' hack in COMPAT_V061. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

channeld: disable check for my_current_per_commitment_point.

e616a41

Under stress, it fails (test_restart_many_payments, the next test). I suspect a deep misunderstanding in the comparison code, will chase separately. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

pytest: add restart-during-n-way payment test.

6f6b36c

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

db: restore failuremsg/failcode from db.

9d5072f

We don't write it in there yet, so this change currently has no effect. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

db: save the failcode / failuremsg into db.

f1b213e

Now we can finally move the fixup code under COMPAT_V061, so it's only for old nodes. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker approved these changes Oct 9, 2018

View reviewed changes

niftynei approved these changes Oct 9, 2018

View reviewed changes

rustyrussell added 2 commits October 10, 2018 09:09

pytest: make test_restart_many_payments more Pythony!

8ad6142

IIUC, namedtuple is like tuple for grown-ups: Pythonify! Suggested-by: @cdecker Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

CHANGELOG.md: update for flurry of HTLC fixes.

18b6c63

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/pr-1960 branch from 14e2798 to 18b6c63 Compare October 9, 2018 22:39

rustyrussell merged commit a3e87af into ElementsProject:master Oct 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for HTLC db issues #2000

Fixes for HTLC db issues #2000

rustyrussell commented Oct 9, 2018

cdecker left a comment •

edited

cdecker Oct 9, 2018

cdecker Oct 9, 2018

cdecker Oct 9, 2018

cdecker Oct 9, 2018

rustyrussell Oct 9, 2018

cdecker Oct 9, 2018

rustyrussell Oct 9, 2018

cdecker Oct 9, 2018

cdecker Oct 9, 2018

niftynei Oct 9, 2018

cdecker Oct 9, 2018

rustyrussell Oct 9, 2018

cdecker Oct 9, 2018

cdecker Oct 9, 2018

rustyrussell Oct 9, 2018

cdecker Oct 9, 2018

rustyrussell Oct 9, 2018

cdecker commented Oct 9, 2018

niftynei Oct 9, 2018

rustyrussell Oct 9, 2018

niftynei Oct 9, 2018

niftynei Oct 9, 2018

Fixes for HTLC db issues #2000

Fixes for HTLC db issues #2000

Conversation

rustyrussell commented Oct 9, 2018

cdecker left a comment • edited

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

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

cdecker commented Oct 9, 2018

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

cdecker left a comment •

edited