Shutdown fixes #847

rustyrussell · 2018-01-30T09:26:50Z

Builds on #841

This is basically a rewrite of closingd to do negotiation properly. Doing so revealed a spec bug, so I've created a patch for that too, and assumed it's going to be accepted. If not, we don't really care; by default we just don't enforce sane negotiation anyway.

rustyrussell · 2018-01-30T11:14:59Z

Looks like this needs more testing...

ZmnSCPxj · 2018-01-31T04:58:56Z

Fixes: #801

cdecker · 2018-02-01T15:19:29Z

Has some valgrind errors:

------------------------------- Valgrind errors --------------------------------
Valgrind error file: valgrind-errors.5732
==5732== Invalid read of size 8
==5732==    at 0x4149FD: remove_cmd_from_hout (pay.c:292)
==5732==    by 0x468BAB: notify (tal.c:237)
==5732==    by 0x469077: del_tree (tal.c:400)
==5732==    by 0x4690C7: del_tree (tal.c:410)
==5732==    by 0x46948A: tal_free (tal.c:509)
==5732==    by 0x40F1EA: main (lightningd.c:362)
==5732==  Address 0x69df148 is 1,512 bytes inside a block of size 1,544 free'd
==5732==    at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5732==    by 0x469150: del_tree (tal.c:421)
==5732==    by 0x46948A: tal_free (tal.c:509)
==5732==    by 0x4198F2: free_htlcs (peer_control.c:1281)
==5732==    by 0x40EBA9: shutdown_subdaemons (lightningd.c:209)
==5732==    by 0x40F1DE: main (lightningd.c:360)
==5732==  Block was alloc'd at
==5732==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5732==    by 0x468C30: allocate (tal.c:250)
==5732==    by 0x4691F7: tal_alloc_ (tal.c:448)
==5732==    by 0x40A279: new_htlc_out (htlc_end.c:143)
==5732==    by 0x41FD64: send_htlc_out (peer_htlcs.c:397)
==5732==    by 0x41511C: send_payment (pay.c:388)
==5732==    by 0x41589E: json_sendpay (pay.c:513)
==5732==    by 0x40D9B1: parse_request (jsonrpc.c:600)
==5732==    by 0x40DCAC: read_json (jsonrpc.c:667)
==5732==    by 0x45C706: next_plan (io.c:59)
==5732==    by 0x45D1DD: do_plan (io.c:387)
==5732==    by 0x45D21B: io_ready (io.c:397)
==5732== 
==5732== Invalid write of size 8
==5732==    at 0x414A27: remove_cmd_from_hout (pay.c:293)
==5732==    by 0x468BAB: notify (tal.c:237)
==5732==    by 0x469077: del_tree (tal.c:400)
==5732==    by 0x4690C7: del_tree (tal.c:410)
==5732==    by 0x46948A: tal_free (tal.c:509)
==5732==    by 0x40F1EA: main (lightningd.c:362)
==5732==  Address 0x69df148 is 1,512 bytes inside a block of size 1,544 free'd
==5732==    at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5732==    by 0x469150: del_tree (tal.c:421)
==5732==    by 0x46948A: tal_free (tal.c:509)
==5732==    by 0x4198F2: free_htlcs (peer_control.c:1281)
==5732==    by 0x40EBA9: shutdown_subdaemons (lightningd.c:209)
==5732==    by 0x40F1DE: main (lightningd.c:360)
==5732==  Block was alloc'd at
==5732==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5732==    by 0x468C30: allocate (tal.c:250)
==5732==    by 0x4691F7: tal_alloc_ (tal.c:448)
==5732==    by 0x40A279: new_htlc_out (htlc_end.c:143)
==5732==    by 0x41FD64: send_htlc_out (peer_htlcs.c:397)
==5732==    by 0x41511C: send_payment (pay.c:388)
==5732==    by 0x41589E: json_sendpay (pay.c:513)
==5732==    by 0x40D9B1: parse_request (jsonrpc.c:600)
==5732==    by 0x40DCAC: read_json (jsonrpc.c:667)
==5732==    by 0x45C706: next_plan (io.c:59)
==5732==    by 0x45D1DD: do_plan (io.c:387)
==5732==    by 0x45D21B: io_ready (io.c:397)
==5732==

This is test_closing_different_fees, the failing node is l1 with port 16331.

rustyrussell · 2018-02-02T00:43:05Z

Yeah, suspect that's an unrelated issue, but I've fixed it once and for all, with the final commit.

And rebased...

cdecker · 2018-02-02T18:15:59Z

Had to fix a small conflict, will merge if CI is happy.

See: lightning/bolts#367 Signed-off-by: Rusty Russell <[email protected]>

Signed-off-by: Rusty Russell <[email protected]>

We shouldn't fail negotiation just because they exceeded what we thought fair: we're better off as long as it's actually <= final commitment fee. Signed-off-by: Rusty Russell <[email protected]>

This follows the proposal to make the funder send the first offer. The dual loops are because there's initially very little restriction on the amounts, then once they're first established they tighten as necessary. Signed-off-by: Rusty Russell <[email protected]>

This is a transitional patch so we can still close channels cleanly; for want of a better option, I hooked it into --deprecated-apis. Signed-off-by: Rusty Russell <[email protected]>

It was taking over 10 minutes under valgrind, causing Travis to time it out. This shrinks it to its essential tests, and also batches. Signed-off-by: Rusty Russell <[email protected]>

Maintaining it was always fraught, since the command could go away if the JSON RPC died. Most recently, it was broken again on shutdown (see below). In future we may allow pay commands to block on previous payments, so it won't even be a 1:1 mapping. Generalize it: keep commands in a simple list and do a lookup when a payment fails/succeeds. Valgrind error file: valgrind-errors.5732 ==5732== Invalid read of size 8 ==5732== at 0x4149FD: remove_cmd_from_hout (pay.c:292) ==5732== by 0x468BAB: notify (tal.c:237) ==5732== by 0x469077: del_tree (tal.c:400) ==5732== by 0x4690C7: del_tree (tal.c:410) ==5732== by 0x46948A: tal_free (tal.c:509) ==5732== by 0x40F1EA: main (lightningd.c:362) ==5732== Address 0x69df148 is 1,512 bytes inside a block of size 1,544 free'd ==5732== at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==5732== by 0x469150: del_tree (tal.c:421) ==5732== by 0x46948A: tal_free (tal.c:509) ==5732== by 0x4198F2: free_htlcs (peer_control.c:1281) ==5732== by 0x40EBA9: shutdown_subdaemons (lightningd.c:209) ==5732== by 0x40F1DE: main (lightningd.c:360) ==5732== Block was alloc'd at ==5732== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==5732== by 0x468C30: allocate (tal.c:250) ==5732== by 0x4691F7: tal_alloc_ (tal.c:448) ==5732== by 0x40A279: new_htlc_out (htlc_end.c:143) ==5732== by 0x41FD64: send_htlc_out (peer_htlcs.c:397) ==5732== by 0x41511C: send_payment (pay.c:388) ==5732== by 0x41589E: json_sendpay (pay.c:513) ==5732== by 0x40D9B1: parse_request (jsonrpc.c:600) ==5732== by 0x40DCAC: read_json (jsonrpc.c:667) ==5732== by 0x45C706: next_plan (io.c:59) ==5732== by 0x45D1DD: do_plan (io.c:387) ==5732== by 0x45D21B: io_ready (io.c:397) Signed-off-by: Rusty Russell <[email protected]>

@niftynei

Based on a commit by @niftynei, but: - Separated quickclose logic from main loop. - I made it indep of anchor_outputs, use and option instead. - Disable if they've specified how to negotiate. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: `experimental-quick-close` option to enable draft fast mutual close protocol (as per lightning-rfc ElementsProject#847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc ElementsProject#847)

@niftynei

Based on a commit by @niftynei, but: - Separated quickclose logic from main loop. - I made it indep of anchor_outputs, use and option instead. - Disable if they've specified how to negotiate. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: `experimental-quick-close` option to enable draft fast mutual close protocol (as per lightning-rfc ElementsProject#847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc ElementsProject#847)

@niftynei

Based on a commit by @niftynei, but: - Separated quickclose logic from main loop. - I made it indep of anchor_outputs, use and option instead. - Disable if they've specified how to negotiate. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: `experimental-quick-close` option to enable draft fast mutual close protocol (as per lightning-rfc ElementsProject#847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc ElementsProject#847)

@niftynei

Based on a commit by @niftynei, but: - Separated quickclose logic from main loop. - I made it indep of anchor_outputs, use and option instead. - Disable if they've specified how to negotiate. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: `experimental-quick-close` option to enable draft fast mutual close protocol (as per lightning-rfc ElementsProject#847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc ElementsProject#847)

@niftynei

Based on a commit by @niftynei, but: - Separated quickclose logic from main loop. - I made it indep of anchor_outputs, use and option instead. - Disable if they've specified how to negotiate. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: `experimental-quick-close` option to enable draft fast mutual close protocol (as per lightning-rfc ElementsProject#847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc ElementsProject#847)

@niftynei

Based on a commit by @niftynei, but: - Separated quickclose logic from main loop. - I made it indep of anchor_outputs, use and option instead. - Disable if they've specified how to negotiate. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: `experimental-quick-close` option to enable draft fast mutual close protocol (as per lightning-rfc ElementsProject#847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc ElementsProject#847)

@niftynei

Based on a commit by @niftynei, but: - Separated quickclose logic from main loop. - I made it indep of anchor_outputs, use and option instead. - Disable if they've specified how to negotiate. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: `experimental-quick-close` option to enable draft fast mutual close protocol (as per lightning-rfc ElementsProject#847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc ElementsProject#847)

@niftynei

Based on a commit by @niftynei, but: - Separated quickclose logic from main loop. - I made it indep of anchor_outputs, use and option instead. - Disable if they've specified how to negotiate. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: `experimental-quick-close` option to enable draft fast mutual close protocol (as per lightning-rfc ElementsProject#847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc ElementsProject#847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc #847)

This follows lightning/bolts#847. For anchor_outputs, we pass down a max_feerate to closingd, and set the fee ceiling to MAX. It uses that to estimate the desired closing fee. Signed-off-by: Rusty Russell <[email protected]> Changelog-EXPERIMENTAL: Anchor output mutual close allow a fee higher than the final commitment transaction (as per lightning-rfc ElementsProject#847)

rustyrussell requested a review from cdecker January 30, 2018 09:26

rustyrussell force-pushed the shutdown-fixes branch from bb6459a to d87d902 Compare February 1, 2018 03:11

rustyrussell force-pushed the shutdown-fixes branch 2 times, most recently from d6bb842 to 3288334 Compare February 2, 2018 01:52

rustyrussell added 9 commits February 2, 2018 19:16

channeld: don't send update_fee after shutdown.

956ff8f

See: lightning/bolts#367 Signed-off-by: Rusty Russell <[email protected]>

lightningd: add more debugging info for closing.

901cd93

Signed-off-by: Rusty Russell <[email protected]>

test_lightning: add tests for closing with differetn feerates.

6dd10fc

Signed-off-by: Rusty Russell <[email protected]>

lightningd: prefer mutual close over unilateral, even if fee identical.

0739278

Signed-off-by: Rusty Russell <[email protected]>

closingd: use the final commitment tx fee as the maximum.

a0b7b4a

We shouldn't fail negotiation just because they exceeded what we thought fair: we're better off as long as it's actually <= final commitment fee. Signed-off-by: Rusty Russell <[email protected]>

closingd: don't punish peers who can't negotiate properly.

6b99dd0

This is a transitional patch so we can still close channels cleanly; for want of a better option, I hooked it into --deprecated-apis. Signed-off-by: Rusty Russell <[email protected]>

test_lightning.py: speed up test_closing_different_fees

53a9e4f

It was taking over 10 minutes under valgrind, causing Travis to time it out. This shrinks it to its essential tests, and also batches. Signed-off-by: Rusty Russell <[email protected]>

cdecker force-pushed the shutdown-fixes branch from 87a0ef5 to 89fb750 Compare February 2, 2018 18:18

cdecker merged commit 9b8fe61 into ElementsProject:master Feb 2, 2018

cdecker mentioned this pull request Feb 2, 2018

Fee jumped ""channel ALL: lightning_closingd: Their fee jumped 14661 to 13410 when ours was 14035"" #801

Closed

ZmnSCPxj mentioned this pull request Feb 3, 2018

Closing negotiations too restrictive #435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shutdown fixes #847

Shutdown fixes #847

rustyrussell commented Jan 30, 2018

rustyrussell commented Jan 30, 2018

ZmnSCPxj commented Jan 31, 2018

cdecker commented Feb 1, 2018

rustyrussell commented Feb 2, 2018 •

edited

Loading

cdecker commented Feb 2, 2018

Shutdown fixes #847

Shutdown fixes #847

Conversation

rustyrussell commented Jan 30, 2018

rustyrussell commented Jan 30, 2018

ZmnSCPxj commented Jan 31, 2018

cdecker commented Feb 1, 2018

rustyrussell commented Feb 2, 2018 • edited Loading

cdecker commented Feb 2, 2018

rustyrussell commented Feb 2, 2018 •

edited

Loading