peer_control: Make close wait for complete closure, with timeout. by ZmnSCPxj · Pull Request #1355 · ElementsProject/lightning

ZmnSCPxj · 2018-04-10T11:04:26Z

Also report tx and txid, and whether we closed unilaterally or
bilaterally, if we could close the channel.

Also make a manpage.

Fixes: #1207
Fixes: #714
Fixes: #622

rustyrussell · 2018-04-11T06:41:17Z

This closely mirrors my https://github.com/rustyrussell/lightning/tree/guilt/remove-dev-fail branch which I obviously forgot to put a PR in for! But yours is better anyway...

I avoided putting in a timeout for simplicity, but OTOH you return more information from close, which is nice but makes the timeout a necessity. You're clearly not from Australia though, because 2 seconds is grossly insufficient as a default :)

You also force a connection initiation to the peer, which is nice. And wrote the man page!

Can I ask you to cherry-pick commits or steal stuff as appropriate from my branch?

ZmnSCPxj · 2018-04-11T08:08:43Z

You're clearly not from Australia though, because 2 seconds is grossly insufficient as a default :)

I am also obviously not an embedded semi-independent agent in an AI agent network sharing computational and network resources with other competing and cooperating knowledge agents, especially since 2 seconds would also be grossly insufficient as a default in that situation.

What would be a plausible default for a human being in Australia?

Can I ask you to cherry-pick commits or steal stuff as appropriate from my branch?

Okay, I shall check. There are no useful tests for the new closing functionality in particular, so if I see any tests I will steal them.

rustyrussell · 2018-04-12T01:57:30Z

30 seconds seems to be the modern default.

ZmnSCPxj · 2018-04-14T00:54:00Z

Rebased, changed timeout to 30 seconds, added some code from @rustyrussell , please review again.

cdecker

Just some minor things that are a bit off.

cdecker · 2018-04-15T15:23:25Z

+Normally the peer needs to be live and connected in order to negotiate
+a bilateral close.
+Forcing a unilateral close can be used if you suspect you can no longer
+access the peer.


access -> contact

cdecker · 2018-04-15T15:25:07Z

+On success, an object with fields 'tx' and 'txid' containing the
+closing transaction are returned.
+It will also have a field 'type' which is either the JSON string
+'bilateral' or the JSON string 'unilateral'.


I usually use collaborative close to describe the bilateral close, which is a bit nicer. The spec talks about a "mutual close" or "cooperative close" which is also slightly nicer than bilateral (and you use cooperative in the code later).

Hmm. Grepping lightning-rfc:

cooperative: 1 time, in 00-introduction.md

bilateral: 0 times, ha

mutual: many times, in 00-introduction.md, 02-peer-protocol.md, 05-onchain.md

Perhaps we should go with 'mutual' instead, as that seems to be the preferred term in the RFC. Note that the BOLT spec uses 'mutual' and 'unilateral'.

cdecker · 2018-04-15T15:26:05Z

 #include <wire/gen_onion_wire.h>

+struct close_command {
+	/* Off struct lightningd close_commands. */


cdecker · 2018-04-15T15:29:13Z

+	cc->channel = NULL;
+	command_fail(cc->cmd, "Channel forgotten before proper close.");
+}
+/* Destroy the close command structure. */


Nit: one empty line between functions.

cdecker · 2018-04-15T15:32:01Z


        # Close the channel to forget the peer
-        l1.rpc.close(l2.info['id'])
+        self.assertRaisesRegex(ValueError,


I don't understand why now all of these closes fail in the integration tests. Aren't these supposed to be collaborative closes? The 0 timeout everywhere is a bit excessive IMHO

The tests assume that close does not wait for the close to complete. This PR makes close wait either for the timeout or the close to complete; to emulate the old behavior we set timeout to 0 and let the command fail due to timeout. Otherwise I would have to check each instance of .rpc.close to see if I can remove the wait_for_logs afterwards or if they scan for something more substantial than bilateral close. Being lazy. I shall see if they will still work properly if I make them wait.

Well, ok, we should slowly make these closes normal again though (noting this for my cleanup queue).

Yes, you should have put a FIXME above each of these!

cdecker · 2018-04-15T15:35:01Z

 	struct short_channel_id *scid;

+	/* Closing txid, for reporting to JSON */
+	struct bitcoin_txid *closing_txid;


This field will not be repopulated when restarting / loading from DB, could that lead us into an inconsistent behavior? Before restart we show the closing_txid after the restart we lose that. If we broadcast the transaction we'll have it in the transactions table btw.

ZmnSCPxj · 2018-04-16T12:28:25Z

Rebased, fixed nits. Removed closing_txid from listpeers command instead as I decided it was complicated to have to save it to the DB too, and the new close now reports the transaction that actually closes the channel. Also did some minor cleanups of some calls to the close RPC in the test. @cdecker please review again!

cdecker · 2018-04-16T13:34:54Z

Thanks @ZmnSCPxj, I oversaw the removal of dev-fail by @rustyrussell, which I think is a bit eager. Forced closes do have the side effect of potentially sending close messages to the peer, which might not be desired in the test cases, so maybe we should keep dev-fail for integration tests?

ZmnSCPxj · 2018-04-16T15:00:10Z

Okay, I will remove that commit that removes dev-fail.

Also report tx and txid, and whether we closed unilaterally or bilaterally, if we could close the channel. Also make a manpage. Fixes: ElementsProject#1207 Fixes: ElementsProject#714 Fixes: ElementsProject#622

It should, indeed, close once they reconnect. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

cdecker · 2018-04-17T10:30:33Z

Just kicked Travis, let's see if the failure was a flake.

Edit: I'm afraid it failed again, did something change with that test? It seems to get hung up on sendrawtransaction. I'm tempted to remove checking for symptoms and go to a more effect based approach, i.e., let some of the wait_for_logs fail and then check whether the desired state has been reached.

ZmnSCPxj · 2018-04-17T13:47:02Z

Well, I notice that the retry system does not seem to work well with this particular test, sigh.

ZmnSCPxj · 2018-04-18T01:46:18Z

Can you teach me how to remove the rerunfailures? I tried, to run without rerunfailures, as I suspect that an earlier run will show the root cause, and rerunfailures does not completely clear the test environment, so that reruns will fail for spurious reason once the first run fails for the real reason.

I tried removing the rerunfailures here: ZmnSCPxj@b8925ff but the CI still runs it with rerun. Hmm....

ZmnSCPxj · 2018-04-18T06:55:02Z

Increasing the timeouts for closing in the test_closing_different_fees seems to make it work even in NO_VALGRIND=0 mode. Probably just a load problem in the CI (we are running 7 lightningd in parallel and performing 6 closes in parallel).

cdecker · 2018-04-18T09:23:18Z

That's good news, any reason for it being 72? We can probably just overshoot and say 100 seconds, just to be safe.

cdecker · 2018-04-18T09:37:01Z

And to answer your question: rerunfailures is enabled automagically on Travis using the following lines in the Makefile:

lightning/Makefile

Lines 43 to 47 in 774af5f

    
           PYTEST := $(shell command -v pytest 2> /dev/null) 
        
           PYTEST_OPTS := -v -x 
        
           ifeq ($(TRAVIS),true) 
        
           PYTEST_OPTS += --reruns=3 
        
           endif

So if you set TRAVIS=false it'll not rerun.

cdecker · 2018-04-18T10:35:15Z

I also missed this comment:

Removed closing_txid from listpeers command instead as I decided it was complicated to have to save it to the DB too, and the new close now reports the transaction that actually closes the channel.

We are now storing all transactions that we broadcast or that we watched in the transactions table, so you could just add the txid into the table and then fetch it from there 😉 But we can add that later as well.

ZmnSCPxj · 2018-04-18T13:45:36Z

We already store last_tx associated with the channel; the transactions table does not have an easy association between channel and transaction. I just need to figure out when to show it as closing_tx/closing_txid. Definitely when it is in CLOSINGD_COMPLETE, possibly ONCHAIN too (but I am uncertain about the case where we trigger an error and the other side gets its commitment onchain before we do; do we track the transaction in this case?). Let us do it in another PR though, as arguably the important bit (reporting the closing transaction on return of close command) is here already and closing_txid is an enhancement.

rustyrussell

Ack fa3cff6

rustyrussell · 2018-04-23T02:22:29Z


        # Close the channel to forget the peer
-        l1.rpc.close(l2.info['id'])
+        self.assertRaisesRegex(ValueError,


Yes, you should have put a FIXME above each of these!

ZmnSCPxj mentioned this pull request Apr 11, 2018

manpages #1360

Closed

10 tasks

ZmnSCPxj force-pushed the close-wait-timeout branch from 49e5829 to 2e77181 Compare April 14, 2018 00:53

ZmnSCPxj requested review from cdecker and rustyrussell April 14, 2018 00:54

ZmnSCPxj force-pushed the close-wait-timeout branch 4 times, most recently from 451112d to 31850c3 Compare April 14, 2018 13:47

cdecker reviewed Apr 15, 2018

View reviewed changes

ZmnSCPxj force-pushed the close-wait-timeout branch 2 times, most recently from ff7c19b to 602bee5 Compare April 16, 2018 11:05

ZmnSCPxj and others added 3 commits April 16, 2018 15:00

peer_control: Make close wait for complete closure, with timeout.

48d9e33

Also report tx and txid, and whether we closed unilaterally or bilaterally, if we could close the channel. Also make a manpage. Fixes: ElementsProject#1207 Fixes: ElementsProject#714 Fixes: ElementsProject#622

json_close: test that it works while disconnected.

443fedd

It should, indeed, close once they reconnect. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

test_lightningd.py: Clean up some uses of 'close' RPC.

28868b8

ZmnSCPxj force-pushed the close-wait-timeout branch from d3b03a1 to 28868b8 Compare April 16, 2018 15:01

Loosen close timeout in test_closing_different_fees

fa3cff6

ZmnSCPxj mentioned this pull request Apr 19, 2018

Channel stuck shutting down - "log": "Funding tx announce ready, but channel state CHANNELD_SHUTTING_DOWN owned by none" #1393

Closed

rustyrussell approved these changes Apr 23, 2018

View reviewed changes

rustyrussell merged commit d6bf793 into ElementsProject:master Apr 23, 2018

ZmnSCPxj deleted the close-wait-timeout branch April 23, 2018 05:45

ZmnSCPxj mentioned this pull request Apr 24, 2018

Maintain permanent connections for active channels #1322

Merged

Uh oh!

Conversation

ZmnSCPxj commented Apr 10, 2018

Uh oh!

rustyrussell commented Apr 11, 2018

Uh oh!

ZmnSCPxj commented Apr 11, 2018

Uh oh!

rustyrussell commented Apr 12, 2018

Uh oh!

ZmnSCPxj commented Apr 14, 2018

Uh oh!

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

ZmnSCPxj Apr 15, 2018 • 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!

ZmnSCPxj commented Apr 16, 2018

Uh oh!

cdecker commented Apr 16, 2018

Uh oh!

ZmnSCPxj commented Apr 16, 2018

Uh oh!

cdecker commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZmnSCPxj commented Apr 17, 2018

Uh oh!

ZmnSCPxj commented Apr 18, 2018

Uh oh!

ZmnSCPxj commented Apr 18, 2018

Uh oh!

cdecker commented Apr 18, 2018

Uh oh!

cdecker commented Apr 18, 2018

Uh oh!

cdecker commented Apr 18, 2018

Uh oh!

ZmnSCPxj commented Apr 18, 2018

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ZmnSCPxj Apr 15, 2018 •

edited

Loading

cdecker commented Apr 17, 2018 •

edited

Loading