Conversation
|
Need to debug the CI failure tomorrow, hence the WIP tag 😉 |
cdecker
force-pushed
the
connected_hook
branch
from
c291445 to
2b0a11a
Compare
|
Should be ready to be reciewed, turns out it was just the switch from Ping @rustyrussell |
rustyrussell
left a comment
rustyrussell
left a comment
There was a problem hiding this comment.
This is really nice; just a couple of minor quibbles which should be easy to change/disagree with.
| enum peer_connected_hook_result { | ||
| peer_connected_continue, | ||
| peer_connected_disconnect, | ||
| }; |
There was a problem hiding this comment.
enum constants should be CAPS... I know it seems shouty but there it is
| */ | ||
| void derive_channel_id(struct channel_id *channel_id, | ||
| struct bitcoin_txid *txid, u16 txout) | ||
| const struct bitcoin_txid *txid, u16 txout) |
| } else { | ||
| json_add_null(stream, "channel"); | ||
| } | ||
| json_object_end(stream); /* .peer */ |
There was a problem hiding this comment.
I think this hook may be far too helpful. Just hand it the peer id, address and the global and local features from this connection message; the rest it can get via an RPC call if it wants?
| void json_add_null(struct json_stream *stream, const char *fieldname) | ||
| { | ||
| json_add_member(stream, fieldname, "null"); | ||
| } |
There was a problem hiding this comment.
This commit should precede the prior one, but moreover: I think the usage here is wrong. If a field doesn't exist it shouldn't exist, not be set to 'null'. That's how we handle every other optional field.
lightningd/peer_control.c
Outdated
| if (json_tok_streq(buffer, resulttok, "continue")) | ||
| resp->result = peer_connected_continue; | ||
| else | ||
| resp->result = peer_connected_disconnect; |
There was a problem hiding this comment.
Probably best to be explicit about the alternative to continue, eg "disconnect"? Otherwise typos can have bad effects, and its hard to add other return types in the future (eg. an error message to send or something?).
cdecker
force-pushed
the
connected_hook
branch
from
2b0a11a to
a09dc49
Compare
|
Thanks @rustyrussell for the feedback, I rebased and amended as follows:
I totally agree that the hook call was too heavy 👍 |
This hook is used to let a plugin decide whether we should continue with the connection normally, or whether we should be dropping the connection. Possible use-cases include policy enforcement (dropping connections based on previous interactions), draining a node by allowing only peers with an active channel to reconnect, or temporarily preventing a channel from making progress. Signed-off-by: Christian Decker <[email protected]>
It's not modifying anything in the txid itself, just mashing it up with the txout index. Signed-off-by: Christian Decker <[email protected]>
This used to be inline, but we want to pass channels to hooks as well, so we just extract this into its own function. Signed-off-by: Christian Decker <[email protected]>
The format is very similar to the one for `listpeers` except we only list a single channel, and we list the actual netaddr that connected instead of all known from gossip. Signed-off-by: Christian Decker <[email protected]>
Signed-off-by: Christian Decker <[email protected]>
Final step for the `peer_connected` hook, we parse the result and act accordingly. Currently we just close the underlying connection, but we may want to clean up peers that did not end up with a channel. Signed-off-by: Christian Decker <[email protected]>
This plugin just rejects `node_id`s it gets told about. Signed-off-by: Christian Decker <[email protected]>
Signed-off-by: Christian Decker <[email protected]>
cdecker
force-pushed
the
connected_hook
branch
from
a09dc49 to
968e912
Compare
|
Interpreting the approval above as an ACK: ACK 968e912 |
2 participants
This is a simpler example than the
htlc_acceptedhook that I'm still working on, but it already allows us to do quite some interesting things. For example it'd allow us to implement a drain-plugin that allows connections to and from peers we are still negotiating a close with, but reject all connections that could result in a new channel.It also serves as an example on how hooks are implemented.