Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

openchannel hook: add new close_to field #3280

Merged
merged 1 commit into from
Nov 22, 2019
Merged

openchannel hook: add new close_to field #3280

merged 1 commit into from
Nov 22, 2019

Conversation

niftynei
Copy link
Contributor

I wanted to add this to dual_funding, but decided that it's better to not cram too much more into that than need be, so instead did this off of master. This is more of a 'nice to have' now, but will be much more important come dual funded channels.

Rounds out the application of upfront_shutdown_script, allowing
an accepting node to specify a close_to address.

Prior to this, only the opening node could specify one.

Changelog-Added: Plugins: Allow the 'accepter' to specify an upfront_shutdown_script for
a channel via a close_to field in the result.

@niftynei niftynei added this to the 0.7.4 milestone Nov 21, 2019
@niftynei niftynei requested a review from cdecker as a code owner November 21, 2019 00:55
@niftynei niftynei force-pushed the nifty/hook-upfront-close branch from f6a9391 to 5d534f7 Compare November 21, 2019 00:56
rustyrussell
rustyrussell approved these changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bonus points of you remove DEV_OPENINGD_UPFRONT_SHUTDOWN_SCRIPT hack now we have proper support, but that's technically a separate issue.

lightningd/opening_control.c Outdated
" openchannel.close_to hook: %.*s",
t->end - t->start, buffer + t->start);
case ADDRESS_PARSE_WRONG_NETWORK:
fatal("Plugin returned invalid respnse to the"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/respnse/response/

lightningd/opening_control.c Outdated
@@ -808,7 +836,8 @@ static void opening_got_offer(struct subd *openingd,
if (peer_active_channel(uc->peer)) {
subd_send_msg(openingd,
take(towire_opening_got_offer_reply(NULL,
"Already have active channel")));
"Already have active channel",
tal_arr(tmpctx, u8, 0))));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULL works here.

@cdecker
Copy link
Member

cdecker commented Nov 21, 2019

Your Changelog entry is likely not working as expected: the script (currently) doesn't know how to handle multiple lines:

Changelog-Added: Plugins: Allow the 'accepter' to specify an upfront_shutdown_script for
a channel via a `close_to` field in the openchannel hook result

The entry would end at for. If you have a good idea on how to handle this correctly I'm open for suggestions. We could either mandate that the entries must be the last in the commit (not very portable, and would not allow for multiple entries in a single commit), or we could require continuations to be indented with some whitespace (doesn't really work for editors that wrap automatically).

cdecker
cdecker reviewed Nov 21, 2019
View reviewed changes
doc/PLUGINS.md Outdated
}
```

Note that `close_to` must be a valid address for the current chain; an invalid address will cause the node to crash.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we call it "exit with an error" instead of "crash" it is after all a safety abort :-)

@cdecker
Copy link
Member

cdecker commented Nov 21, 2019

Very nice change, yet another step towards full external on-chain wallet support 👍

@niftynei
Copy link
Contributor Author

niftynei commented Nov 21, 2019 •
edited
Loading

If you have a good idea on how to handle this correctly I'm open for suggestions.

The 'traditional' way to signal that "i'm continuing onto the next line" is to use a \ to 'escape' the newline character. This would be my suggestion.

This seems like a nice to have though; I updated this to be a single line.

@niftynei
openchannel hook: add new close_to field
d22b1fd 
Rounds out the application of `upfront_shutdown_script`, allowing
an accepting node to specify a close_to address.

Prior to this, only the opening node could specify one.

Changelog-Added: Plugins: Allow the 'accepter' to specify an upfront_shutdown_script for a channel via a `close_to` field in the openchannel hook result
@niftynei niftynei force-pushed the nifty/hook-upfront-close branch from 5d534f7 to d22b1fd Compare November 21, 2019 21:20
@rustyrussell
Copy link
Contributor

Your Changelog entry is likely not working as expected: the script (currently) doesn't know how to handle multiple lines:

Yech, stick with a single line for now. Release Captain has to neaten them anyway...

@rustyrussell
Copy link
Contributor

Ack d22b1fd

@rustyrussell rustyrussell merged commit de16d0f into ElementsProject:master Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdecker cdecker cdecker left review comments

@rustyrussell rustyrussell rustyrussell approved these changes

Assignees
No one assigned
Labels
openingd
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@niftynei @cdecker @rustyrussell