plugins: make chained hooks have two different callbacks.

One is called on every plugin return, and tells us whether to continue;
the other is only called if every plugin says ok.

This works for things like payload replacement, where we need to process
the results from each plugin, not just the final one!

We should probably turn everything into a chained callback next
release.

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

lightningd/invoice.c

@@ -278,11 +278,10 @@ invoice_payment_hook_cb(struct invoice_payment_hook_payload *payload STEALS,
 	htlc_set_fulfill(payload->set, &payload->preimage);
 }
 
-REGISTER_PLUGIN_HOOK(invoice_payment,
-		     PLUGIN_HOOK_SINGLE,
-		     invoice_payment_hook_cb,
-		     invoice_payment_serialize,
-		     struct invoice_payment_hook_payload *);
+REGISTER_SINGLE_PLUGIN_HOOK(invoice_payment,
+			    invoice_payment_hook_cb,
+			    invoice_payment_serialize,
+			    struct invoice_payment_hook_payload *);
 
 const struct invoice_details *
 invoice_check_payment(const tal_t *ctx,

lightningd/jsonrpc.c

@@ -769,10 +769,10 @@ rpc_command_hook_callback(struct rpc_command_hook_payload *p STEALS,
 	                         "Bad response to 'rpc_command' hook."));
 }
 
-REGISTER_PLUGIN_HOOK(rpc_command, PLUGIN_HOOK_SINGLE,
-		     rpc_command_hook_callback,
-                     rpc_command_hook_serialize,
-                     struct rpc_command_hook_payload *);
+REGISTER_SINGLE_PLUGIN_HOOK(rpc_command,
+			    rpc_command_hook_callback,
+			    rpc_command_hook_serialize,
+			    struct rpc_command_hook_payload *);
 
 static void call_rpc_command_hook(struct rpc_command_hook_payload *p)
 {

lightningd/onion_message.c

@@ -41,17 +41,15 @@ onion_message_serialize(struct onion_message_hook_payload *payload,
 }
 
 static void
-onion_message_hook_cb(struct onion_message_hook_payload *payload STEALS,
-			 const char *buffer,
-			 const jsmntok_t *toks)
+onion_message_hook_cb(struct onion_message_hook_payload *payload STEALS)
 {
-	/* The core infra checks the "result"; anything other than continue
+	/* plugin_hook_continue checks the "result"; anything other than continue
 	 * just stops. */
 	tal_free(payload);
 }
 
 REGISTER_PLUGIN_HOOK(onion_message,
-		     PLUGIN_HOOK_CHAIN,
+		     plugin_hook_continue,
 		     onion_message_hook_cb,
 		     onion_message_serialize,
 		     struct onion_message_hook_payload *);

lightningd/opening_control.c

@@ -826,11 +826,10 @@ static void openchannel_hook_cb(struct openchannel_hook_payload *payload STEALS,
 							  our_upfront_shutdown_script)));
 }
 
-REGISTER_PLUGIN_HOOK(openchannel,
-		     PLUGIN_HOOK_SINGLE,
-		     openchannel_hook_cb,
-		     openchannel_hook_serialize,
-		     struct openchannel_hook_payload *);
+REGISTER_SINGLE_PLUGIN_HOOK(openchannel,
+			    openchannel_hook_cb,
+			    openchannel_hook_serialize,
+			    struct openchannel_hook_payload *);
 
 static void opening_got_offer(struct subd *openingd,
 			      const u8 *msg,

lightningd/peer_control.c

@@ -972,10 +972,10 @@ send_error:
 	tal_free(payload);
 }
 
-REGISTER_PLUGIN_HOOK(peer_connected, PLUGIN_HOOK_SINGLE,
-		     peer_connected_hook_cb,
-		     peer_connected_serialize,
-		     struct peer_connected_hook_payload *);
+REGISTER_SINGLE_PLUGIN_HOOK(peer_connected,
+			    peer_connected_hook_cb,
+			    peer_connected_serialize,
+			    struct peer_connected_hook_payload *);
 
 /* Connectd tells us a peer has connected: it never hands us duplicates, since
  * it holds them until we say peer_died. */
@@ -2309,10 +2309,10 @@ static void custommsg_payload_serialize(struct custommsg_payload *payload,
 	json_add_node_id(stream, "peer_id", &payload->peer_id);
 }
 
-REGISTER_PLUGIN_HOOK(custommsg, PLUGIN_HOOK_SINGLE,
-		     custommsg_callback,
-		     custommsg_payload_serialize,
-		     struct custommsg_payload *);
+REGISTER_SINGLE_PLUGIN_HOOK(custommsg,
+			    custommsg_callback,
+			    custommsg_payload_serialize,
+			    struct custommsg_payload *);
 
 void handle_custommsg_in(struct lightningd *ld, const struct node_id *peer_id,
 			 const u8 *msg)

lightningd/peer_htlcs.c

@@ -795,14 +795,6 @@ struct htlc_accepted_hook_payload {
 	size_t failtlvpos;
 };
 
-/* The possible return value types that a plugin may return for the
- * `htlc_accepted` hook. */
-enum htlc_accepted_result {
-	htlc_accepted_continue,
-	htlc_accepted_fail,
-	htlc_accepted_resolve,
-};
-
 /* We only handle the simplest cases here */
 static u8 *convert_failcode(const tal_t *ctx,
 			    struct lightningd *ld,
@@ -835,22 +827,19 @@ static u8 *convert_failcode(const tal_t *ctx,
 }
 
 /**
- * Parses the JSON-RPC response into a struct understood by the callback.
+ * Callback when a plugin answers to the htlc_accepted hook
  */
-static enum htlc_accepted_result
-htlc_accepted_hook_deserialize(const tal_t *ctx,
-			       struct lightningd *ld,
-			       const char *buffer, const jsmntok_t *toks,
-			       /* If accepted */
-			       struct preimage *payment_preimage,
-			       /* If rejected (tallocated off ctx) */
-                               const u8 **failmsg)
+static bool htlc_accepted_hook_deserialize(struct htlc_accepted_hook_payload *request,
+					   const char *buffer,
+					   const jsmntok_t *toks)
 {
+	struct htlc_in *hin = request->hin;
+	struct lightningd *ld = request->ld;
+	struct preimage payment_preimage;
 	const jsmntok_t *resulttok, *paykeytok;
-	enum htlc_accepted_result result;
 
 	if (!toks || !buffer)
-		return htlc_accepted_continue;
+		return true;
 
 	resulttok = json_get_member(buffer, toks, "result");
 
@@ -862,18 +851,18 @@ htlc_accepted_hook_deserialize(const tal_t *ctx,
 	}
 
 	if (json_tok_streq(buffer, resulttok, "continue")) {
-		return htlc_accepted_continue;
+		return true;
 	}
 
 	if (json_tok_streq(buffer, resulttok, "fail")) {
+		u8 *failmsg;
 		const jsmntok_t *failmsgtok, *failcodetok;
 
-		result = htlc_accepted_fail;
 		failmsgtok = json_get_member(buffer, toks, "failure_message");
 		if (failmsgtok) {
-			*failmsg = json_tok_bin_from_hex(ctx, buffer,
-							 failmsgtok);
-			if (!*failmsg)
+			failmsg = json_tok_bin_from_hex(NULL, buffer,
+							failmsgtok);
+			if (!failmsg)
 				fatal("Bad failure_message for htlc_accepted"
 				      " hook: %.*s",
 				      failmsgtok->end - failmsgtok->start,
@@ -888,11 +877,12 @@ htlc_accepted_hook_deserialize(const tal_t *ctx,
 				      failcodetok->end
 				      - failcodetok->start,
 				      buffer + failcodetok->start);
-			*failmsg = convert_failcode(ctx, ld, failcode);
+			failmsg = convert_failcode(NULL, ld, failcode);
 		} else
-			*failmsg = towire_temporary_node_failure(ctx);
+			failmsg = towire_temporary_node_failure(NULL);
+		local_fail_in_htlc(hin, take(failmsg));
+		return false;
 	} else if (json_tok_streq(buffer, resulttok, "resolve")) {
-		result = htlc_accepted_resolve;
 		paykeytok = json_get_member(buffer, toks, "payment_key");
 		if (!paykeytok)
 			fatal(
@@ -900,18 +890,16 @@ htlc_accepted_hook_deserialize(const tal_t *ctx,
 			    "value to the htlc_accepted hook: %s",
 			    json_strdup(tmpctx, buffer, resulttok));
 
-		if (!json_to_preimage(buffer, paykeytok,
-				      payment_preimage))
+		if (!json_to_preimage(buffer, paykeytok, &payment_preimage))
 			fatal("Plugin specified an invalid 'payment_key': %s",
 			      json_tok_full(buffer, resulttok));
+		fulfill_htlc(hin, &payment_preimage);
+		return false;
 	} else {
 		fatal("Plugin responded with an unknown result to the "
 		      "htlc_accepted hook: %s",
 		      json_strdup(tmpctx, buffer, resulttok));
 	}
-
-	/* cppcheck-suppress uninitvar - false positive on fatal() above */
-	return result;
 }
 
 static void htlc_accepted_hook_serialize(struct htlc_accepted_hook_payload *p,
@@ -976,57 +964,40 @@ static void htlc_accepted_hook_serialize(struct htlc_accepted_hook_payload *p,
  * Callback when a plugin answers to the htlc_accepted hook
  */
 static void
-htlc_accepted_hook_callback(struct htlc_accepted_hook_payload *request STEALS,
-			    const char *buffer, const jsmntok_t *toks)
+htlc_accepted_hook_final(struct htlc_accepted_hook_payload *request STEALS)
 {
 	struct route_step *rs = request->route_step;
 	struct htlc_in *hin = request->hin;
 	struct channel *channel = request->channel;
-	struct lightningd *ld = request->ld;
-	struct preimage payment_preimage;
-	enum htlc_accepted_result result;
-	const u8 *failmsg;
-	result = htlc_accepted_hook_deserialize(request, ld, buffer, toks, &payment_preimage, &failmsg);
-
-	switch (result) {
-	case htlc_accepted_continue:
-		/* *Now* we barf if it failed to decode */
-		if (!request->payload) {
-			log_debug(channel->log,
-				  "Failing HTLC because of an invalid payload");
-			local_fail_in_htlc(hin,
-					   take(towire_invalid_onion_payload(
-					       NULL, request->failtlvtype,
-					       request->failtlvpos)));
-		} else if (rs->nextcase == ONION_FORWARD) {
-			forward_htlc(hin, hin->cltv_expiry,
-				     request->payload->amt_to_forward,
-				     request->payload->outgoing_cltv,
-				     request->payload->forward_channel,
-				     serialize_onionpacket(tmpctx, rs->next),
-				     request->next_blinding);
-		} else
-			handle_localpay(hin,
-					request->payload->amt_to_forward,
-					request->payload->outgoing_cltv,
-					*request->payload->total_msat,
-					request->payload->payment_secret);
-		break;
-	case htlc_accepted_fail:
+
+	/* *Now* we barf if it failed to decode */
+	if (!request->payload) {
 		log_debug(channel->log,
-			  "Failing incoming HTLC as instructed by plugin hook");
-		local_fail_in_htlc(hin, take(failmsg));
-		break;
-	case htlc_accepted_resolve:
-		fulfill_htlc(hin, &payment_preimage);
-		break;
-	}
+			  "Failing HTLC because of an invalid payload");
+		local_fail_in_htlc(hin,
+				   take(towire_invalid_onion_payload(
+						NULL, request->failtlvtype,
+						request->failtlvpos)));
+	} else if (rs->nextcase == ONION_FORWARD) {
+		forward_htlc(hin, hin->cltv_expiry,
+			     request->payload->amt_to_forward,
+			     request->payload->outgoing_cltv,
+			     request->payload->forward_channel,
+			     serialize_onionpacket(tmpctx, rs->next),
+			     request->next_blinding);
+	} else
+		handle_localpay(hin,
+				request->payload->amt_to_forward,
+				request->payload->outgoing_cltv,
+				*request->payload->total_msat,
+				request->payload->payment_secret);
 
 	tal_free(request);
 }
 
-REGISTER_PLUGIN_HOOK(htlc_accepted, PLUGIN_HOOK_CHAIN,
-		     htlc_accepted_hook_callback,
+REGISTER_PLUGIN_HOOK(htlc_accepted,
+		     htlc_accepted_hook_deserialize,
+		     htlc_accepted_hook_final,
 		     htlc_accepted_hook_serialize,
 		     struct htlc_accepted_hook_payload *);
 

lightningd/plugin_hook.c

@@ -139,6 +139,12 @@ static void plugin_hook_killed(struct plugin_hook_call_link *link)
 	}
 }
 
+bool plugin_hook_continue(void *unused, const char *buffer, const jsmntok_t *toks)
+{
+	const jsmntok_t *resrestok = json_get_member(buffer, toks, "result");
+	return resrestok && json_tok_streq(buffer, resrestok, "continue");
+}
+
 /**
  * Callback to be passed to the jsonrpc_request.
  *
@@ -149,10 +155,10 @@ static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks,
 				 const jsmntok_t *idtok,
 				 struct plugin_hook_request *r)
 {
-	const jsmntok_t *resulttok, *resrestok;
+	const jsmntok_t *resulttok;
 	struct db *db = r->db;
-	bool more_plugins, cont;
 	struct plugin_hook_call_link *last, *it;
+	bool in_transaction = false;
 
 	if (r->ld->state == LD_STATE_SHUTDOWN) {
 		log_debug(r->ld->log,
@@ -173,40 +179,47 @@ static void plugin_hook_callback(const char *buffer, const jsmntok_t *toks,
 			      r->hook->name, toks->end - toks->start,
 			      buffer + toks->start);
 
-		resrestok = json_get_member(buffer, resulttok, "result");
+		if (r->hook->type == PLUGIN_HOOK_CHAIN) {
+			db_begin_transaction(db);
+			if (!r->hook->deserialize_cb(r->cb_arg, buffer,
+						     resulttok)) {
+				tal_free(r->cb_arg);
+				db_commit_transaction(db);
+				goto cleanup;
+			}
+			in_transaction = true;
+		}
 	} else {
-		/* Buffer and / or resulttok could be used by the reponse_cb
-		 * to identify no-result responses. So make sure both are
-		 * set */
+		/* plugin died */
 		resulttok = NULL;
-		/* cppcheck isn't smart enough to notice that `resrestok`
-		 * doesn't need to be initialized in the expression
-		 * initializing `cont`, so set it to NULL to shut it up. */
-		resrestok = NULL;
 	}
 
-	/* If this is a hook response containing a `continue` and we have more
-	 * plugins queue the next call. In that case we discard the remainder
-	 * of the result, and let the next plugin decide. */
-	cont = buffer == NULL || (resrestok && json_tok_streq(buffer, resrestok, "continue"));
-	more_plugins = !list_empty(&r->call_chain);
-	if (cont && more_plugins) {
+	if (!list_empty(&r->call_chain)) {
+		if (in_transaction)
+			db_commit_transaction(db);
 		plugin_hook_call_next(r);
-	} else {
-		db_begin_transaction(db);
-		r->hook->response_cb(r->cb_arg, buffer, resulttok);
-		db_commit_transaction(db);
-
-		/* We need to remove the destructors from the remaining
-		 * call-chain, otherwise they'd still be called when the
-		 * plugin dies or we shut down. */
-		list_for_each(&r->call_chain, it, list) {
-			tal_del_destructor(it, plugin_hook_killed);
-			tal_steal(r, it);
-		}
+		return;
+	}
 
-		tal_free(r);
+	/* We optimize for the case where we already called deserialize_cb */
+	if (!in_transaction)
+		db_begin_transaction(db);
+	if (r->hook->type == PLUGIN_HOOK_CHAIN)
+		r->hook->final_cb(r->cb_arg);
+	else
+		r->hook->single_response_cb(r->cb_arg, buffer, resulttok);
+	db_commit_transaction(db);
+
+cleanup:
+	/* We need to remove the destructors from the remaining
+	 * call-chain, otherwise they'd still be called when the
+	 * plugin dies or we shut down. */
+	list_for_each(&r->call_chain, it, list) {
+		tal_del_destructor(it, plugin_hook_killed);
+		tal_steal(r, it);
 	}
+
+	tal_free(r);
 }
 
 static void plugin_hook_call_next(struct plugin_hook_request *ph_req)
@@ -258,7 +271,10 @@ void plugin_hook_call_(struct lightningd *ld, const struct plugin_hook *hook,
 		 * roundtrip to the serializer and deserializer. If we
 		 * were expecting a default response it should have
 		 * been part of the `cb_arg`. */
-		hook->response_cb(cb_arg, NULL, NULL);
+		if (hook->type == PLUGIN_HOOK_CHAIN)
+			hook->final_cb(cb_arg);
+		else
+			hook->single_response_cb(cb_arg, NULL, NULL);
 	}
 }
 

lightningd/plugin_hook.h

@@ -31,15 +31,20 @@
  * - `serialize_payload` which takes a payload of type `cb_arg_type`
  *   and serializes it into the given `json_stream`. `
  *
- * - `response_cb` is called once the plugin has responded (or with
+ * For single-plugin hooks:
+ * - `single_response_cb` is called once the plugin has responded (or with
  *   buffer == NULL if there's no plugin).  In addition an arbitrary
  *   additional argument of type `cb_arg_type` can be passed along
  *   that may contain any additional context necessary.  It must free
  *   or otherwise take ownership of the cb_arg_type argument.
  *
+ * For chained-plugin hooks:
+ * - `deserialize_cb` is called for each plugin, if it returns true the
+ *   next one is called, otherwise the cb_arg_type argument is free.
+ * - If all `deserialize_cb` return true, `final_cb` is called.  It must free
+ *   or otherwise take ownership of the cb_arg_type argument.
  *
- * To make hook invocations easier, each hook registered with
- * `REGISTER_PLUGIN_HOOK` provides a `plugin_hook_call_hookname`
+ * To make hook invocations easier, each hook provides a `plugin_hook_call_hookname`
  * function that performs typechecking at compile time, and makes sure
  * that all the provided functions for serialization, deserialization
  * and callback have the correct type.
@@ -57,7 +62,17 @@ struct plugin_hook {
 	 * register this hook, and how the hooks are called. */
 	enum plugin_hook_type type;
 
-	void (*response_cb)(void *arg, const char *buffer, const jsmntok_t *toks);
+	/* For PLUGIN_HOOK_SINGLE hooks */
+	void (*single_response_cb)(void *arg,
+				   const char *buffer, const jsmntok_t *toks);
+
+	/* For PLUGIN_HOOK_CHAIN hooks: */
+	/* Returns false if we should stop iterating (and free arg). */
+	bool (*deserialize_cb)(void *arg,
+			       const char *buffer, const jsmntok_t *toks);
+	void (*final_cb)(void *arg);
+
+	/* To send the payload to the plugin */
 	void (*serialize_payload)(void *src, struct json_stream *dest);
 
 	/* Which plugins have registered this hook? This is a `tal_arr`
@@ -72,6 +87,8 @@ AUTODATA_TYPE(hooks, struct plugin_hook);
 void plugin_hook_call_(struct lightningd *ld, const struct plugin_hook *hook,
 		       tal_t *cb_arg STEALS);
 
+/* Generic deserialize_cb: returns true iff 'result': 'continue' */
+bool plugin_hook_continue(void *arg, const char *buffer, const jsmntok_t *toks);
 
 /* Create a small facade in from of `plugin_hook_call_` to make sure
  * arguments are of the correct type before downcasting them to `void
@@ -94,15 +111,39 @@ void plugin_hook_call_(struct lightningd *ld, const struct plugin_hook *hook,
  * response_cb function accepts the deserialized response format and
  * an arbitrary extra argument used to maintain context.
  */
-#define REGISTER_PLUGIN_HOOK(name, type, response_cb,			       \
-			     serialize_payload, cb_arg_type)	               \
+#define REGISTER_SINGLE_PLUGIN_HOOK(name, response_cb,                         \
+				    serialize_payload, cb_arg_type)            \
 	struct plugin_hook name##_hook_gen = {                                 \
 	    stringify(name),                                                   \
-	    type,                                                              \
+	    PLUGIN_HOOK_SINGLE,                                                \
 	    typesafe_cb_cast(                                                  \
 		void (*)(void *STEALS, const char *, const jsmntok_t *),       \
 		void (*)(cb_arg_type STEALS, const char *, const jsmntok_t *), \
 		response_cb),                                                  \
+	    NULL, NULL,                                                        \
+	    typesafe_cb_cast(void (*)(void *, struct json_stream *),           \
+			     void (*)(cb_arg_type, struct json_stream *),      \
+			     serialize_payload),                               \
+	    NULL, /* .plugins */                                               \
+	};                                                                     \
+	AUTODATA(hooks, &name##_hook_gen);                                     \
+	PLUGIN_HOOK_CALL_DEF(name, cb_arg_type)
+
+
+#define REGISTER_PLUGIN_HOOK(name, deserialize_cb, final_cb,                   \
+			     serialize_payload, cb_arg_type)		       \
+	struct plugin_hook name##_hook_gen = {                                 \
+	    stringify(name),                                                   \
+	    PLUGIN_HOOK_CHAIN,                                                 \
+            NULL,                                                              \
+	    typesafe_cb_cast(                                                  \
+		bool (*)(void *, const char *, const jsmntok_t *),             \
+		bool (*)(cb_arg_type, const char *, const jsmntok_t *),        \
+		deserialize_cb),                                               \
+	    typesafe_cb_cast(                                                  \
+		void (*)(void *STEALS),	                                       \
+		void (*)(cb_arg_type STEALS),                                  \
+		final_cb),                                                     \
 	    typesafe_cb_cast(void (*)(void *, struct json_stream *),           \
 			     void (*)(cb_arg_type, struct json_stream *),      \
 			     serialize_payload),                               \

lightningd/test/run-invoice-select-inchan.c

@@ -377,6 +377,9 @@ void per_peer_state_set_fds(struct per_peer_state *pps UNNEEDED,
 void plugin_hook_call_(struct lightningd *ld UNNEEDED, const struct plugin_hook *hook UNNEEDED,
 		       tal_t *cb_arg UNNEEDED)
 { fprintf(stderr, "plugin_hook_call_ called!\n"); abort(); }
+/* Generated stub for plugin_hook_continue */
+bool plugin_hook_continue(void *arg UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t *toks UNNEEDED)
+{ fprintf(stderr, "plugin_hook_continue called!\n"); abort(); }
 /* Generated stub for subd_release_channel */
 void subd_release_channel(struct subd *owner UNNEEDED, void *channel UNNEEDED)
 { fprintf(stderr, "subd_release_channel called!\n"); abort(); }

lightningd/test/run-jsonrpc.c

@@ -98,6 +98,9 @@ struct command_result *param_tok(struct command *cmd UNNEEDED, const char *name
 void plugin_hook_call_(struct lightningd *ld UNNEEDED, const struct plugin_hook *hook UNNEEDED,
 		       tal_t *cb_arg UNNEEDED)
 { fprintf(stderr, "plugin_hook_call_ called!\n"); abort(); }
+/* Generated stub for plugin_hook_continue */
+bool plugin_hook_continue(void *arg UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t *toks UNNEEDED)
+{ fprintf(stderr, "plugin_hook_continue called!\n"); abort(); }
 /* AUTOGENERATED MOCKS END */
 
 bool deprecated_apis;

wallet/test/run-wallet.c

@@ -549,6 +549,9 @@ void per_peer_state_set_fds(struct per_peer_state *pps UNNEEDED,
 void plugin_hook_call_(struct lightningd *ld UNNEEDED, const struct plugin_hook *hook UNNEEDED,
 		       tal_t *cb_arg UNNEEDED)
 { fprintf(stderr, "plugin_hook_call_ called!\n"); abort(); }
+/* Generated stub for plugin_hook_continue */
+bool plugin_hook_continue(void *arg UNNEEDED, const char *buffer UNNEEDED, const jsmntok_t *toks UNNEEDED)
+{ fprintf(stderr, "plugin_hook_continue called!\n"); abort(); }
 /* Generated stub for process_onionpacket */
 struct route_step *process_onionpacket(
 	const tal_t * ctx UNNEEDED,