invoices: Make the invoice_details more idiomatic

This seems like a premature optimization: it tried to cut down the number of
allocations by reusing the same `struct invoice_details` while iterating through
a number of results. But this sidesteps the checks by `valgrind` and we'd miss a
missing field that was set by the previous iteration.

Reported-by: @rustyrussell
Signed-off-by: Christian Decker <@cdecker>

lightningd/invoice.c

@@ -60,11 +60,11 @@ static void json_add_invoice(struct json_result *response,
 static void tell_waiter(struct command *cmd, const struct invoice *inv)
 {
 	struct json_result *response = new_json_result(cmd);
-	struct invoice_details details;
+	const struct invoice_details *details;
 
-	wallet_invoice_details(cmd, cmd->ld->wallet, *inv, &details);
-	json_add_invoice(response, &details);
-	if (details.state == PAID)
+	details = wallet_invoice_details(cmd, cmd->ld->wallet, *inv);
+	json_add_invoice(response, details);
+	if (details->state == PAID)
 		command_success(cmd, response);
 	else {
 		/* FIXME: -2 should be a constant in jsonrpc_errors.h.  */
@@ -153,7 +153,7 @@ static void json_invoice(struct command *cmd,
 			 const char *buffer, const jsmntok_t *params)
 {
 	struct invoice invoice;
-	struct invoice_details details;
+	const struct invoice_details *details;
 	const jsmntok_t *msatoshi, *label, *desctok, *fallbacks;
 	const jsmntok_t *preimagetok;
 	u64 *msatoshi_val;
@@ -320,23 +320,23 @@ static void json_invoice(struct command *cmd,
 	}
 
 	/* Get details */
-	wallet_invoice_details(cmd, cmd->ld->wallet, invoice, &details);
+	details = wallet_invoice_details(cmd, cmd->ld->wallet, invoice);
 
 	json_object_start(response, NULL);
-	json_add_hex(response, "payment_hash",
-		     &details.rhash, sizeof(details.rhash));
-	json_add_u64(response, "expires_at", details.expiry_time);
-	json_add_string(response, "bolt11", details.bolt11);
+	json_add_hex(response, "payment_hash", details->rhash.u.u8,
+		     sizeof(details->rhash));
+	json_add_u64(response, "expires_at", details->expiry_time);
+	json_add_string(response, "bolt11", details->bolt11);
 	json_object_end(response);
 
 	command_success(cmd, response);
 }
 
 static const struct json_command invoice_command = {
-	"invoice",
-	json_invoice,
-	"Create an invoice for {msatoshi} with {label} and {description} with optional {expiry} seconds (default 1 hour) and optional {preimage} (default autogenerated)"
-};
+    "invoice", json_invoice, "Create an invoice for {msatoshi} with {label} "
+			     "and {description} with optional {expiry} seconds "
+			     "(default 1 hour) and optional {preimage} "
+			     "(default autogenerated)"};
 AUTODATA(json_command, &invoice_command);
 
 static void json_add_invoices(struct json_result *response,
@@ -344,23 +344,22 @@ static void json_add_invoices(struct json_result *response,
 			      const struct json_escaped *label)
 {
 	struct invoice_iterator it;
-	struct invoice_details details;
+	const struct invoice_details *details;
 
 	/* Don't iterate entire db if we're just after one. */
 	if (label) {
 		struct invoice invoice;
 		if (wallet_invoice_find_by_label(wallet, &invoice, label)) {
-			wallet_invoice_details(response, wallet, invoice,
-					       &details);
-			json_add_invoice(response, &details);
+			details = wallet_invoice_details(response, wallet, invoice);
+			json_add_invoice(response, details);
 		}
 		return;
 	}
 
 	memset(&it, 0, sizeof(it));
 	while (wallet_invoice_iterate(wallet, &it)) {
-		wallet_invoice_iterator_deref(response, wallet, &it, &details);
-		json_add_invoice(response, &details);
+		details = wallet_invoice_iterator_deref(response, wallet, &it);
+		json_add_invoice(response, details);
 	}
 }
 
@@ -408,7 +407,7 @@ static void json_delinvoice(struct command *cmd,
 			    const char *buffer, const jsmntok_t *params)
 {
 	struct invoice i;
-	struct invoice_details details;
+	const struct invoice_details *details;
 	const jsmntok_t *labeltok, *statustok;
 	struct json_result *response = new_json_result(cmd);
 	const char *status, *actual_status;
@@ -433,13 +432,14 @@ static void json_delinvoice(struct command *cmd,
 		command_fail(cmd, LIGHTNINGD, "Unknown invoice");
 		return;
 	}
-	wallet_invoice_details(cmd, cmd->ld->wallet, i, &details);
+
+	details = wallet_invoice_details(cmd, cmd->ld->wallet, i);
 
 	status = tal_strndup(cmd, buffer + statustok->start,
 			     statustok->end - statustok->start);
 	/* This is time-sensitive, so only call once; otherwise error msg
 	 * might not make sense if it changed! */
-	actual_status = invoice_status_str(&details);
+	actual_status = invoice_status_str(details);
 	if (!streq(actual_status, status)) {
 		command_fail(cmd, LIGHTNINGD, "Invoice status is %s not %s",
 			     actual_status, status);
@@ -448,7 +448,7 @@ static void json_delinvoice(struct command *cmd,
 
 	/* Get invoice details before attempting to delete, as
 	 * otherwise the invoice will be freed. */
-	json_add_invoice(response, &details);
+	json_add_invoice(response, details);
 
 	if (!wallet_invoice_delete(wallet, i)) {
 		log_broken(cmd->ld->log,
@@ -562,7 +562,7 @@ static void json_waitinvoice(struct command *cmd,
 			      const char *buffer, const jsmntok_t *params)
 {
 	struct invoice i;
-	struct invoice_details details;
+	const struct invoice_details *details;
 	struct wallet *wallet = cmd->ld->wallet;
 	const jsmntok_t *labeltok;
 	struct json_escaped *label;
@@ -586,10 +586,10 @@ static void json_waitinvoice(struct command *cmd,
 		command_fail(cmd, LIGHTNINGD, "Label not found");
 		return;
 	}
-	wallet_invoice_details(cmd, cmd->ld->wallet, i, &details);
+	details = wallet_invoice_details(cmd, cmd->ld->wallet, i);
 
 	/* If paid or expired return immediately */
-	if (details.state == PAID || details.state == EXPIRED) {
+	if (details->state == PAID || details->state == EXPIRED) {
 		tell_waiter(cmd, &i);
 		return;
 	} else {

lightningd/peer_htlcs.c

@@ -249,7 +249,7 @@ static void handle_localpay(struct htlc_in *hin,
 {
 	enum onion_type failcode;
 	struct invoice invoice;
-	struct invoice_details details;
+	const struct invoice_details *details;
 	struct lightningd *ld = hin->key.channel->peer->ld;
 
 	/* BOLT #4:
@@ -282,7 +282,7 @@ static void handle_localpay(struct htlc_in *hin,
 		failcode = WIRE_UNKNOWN_PAYMENT_HASH;
 		goto fail;
 	}
-	wallet_invoice_details(tmpctx, ld->wallet, invoice, &details);
+	details = wallet_invoice_details(tmpctx, ld->wallet, invoice);
 
 	/* BOLT #4:
 	 *
@@ -298,10 +298,10 @@ static void handle_localpay(struct htlc_in *hin,
 	 *         leakage by altering the amount while not allowing for
 	 *         accidental gross overpayment.
 	 */
-	if (details.msatoshi != NULL && hin->msatoshi < *details.msatoshi) {
+	if (details->msatoshi != NULL && hin->msatoshi < *details->msatoshi) {
 		failcode = WIRE_INCORRECT_PAYMENT_AMOUNT;
 		goto fail;
-	} else if (details.msatoshi != NULL && hin->msatoshi > *details.msatoshi * 2) {
+	} else if (details->msatoshi != NULL && hin->msatoshi > *details->msatoshi * 2) {
 		failcode = WIRE_INCORRECT_PAYMENT_AMOUNT;
 		goto fail;
 	}
@@ -324,10 +324,10 @@ static void handle_localpay(struct htlc_in *hin,
 	}
 
 	log_info(ld->log, "Resolving invoice '%s' with HTLC %"PRIu64,
-		 details.label->s, hin->key.id);
+		 details->label->s, hin->key.id);
 	log_debug(ld->log, "%s: Actual amount %"PRIu64"msat, HTLC expiry %u",
-		  details.label->s, hin->msatoshi, cltv_expiry);
-	fulfill_htlc(hin, &details.r);
+		  details->label->s, hin->msatoshi, cltv_expiry);
+	fulfill_htlc(hin, &details->r);
 	wallet_invoice_resolve(ld->wallet, invoice, hin->msatoshi);
 
 	return;

wallet/invoices.c

@@ -87,20 +87,20 @@ trigger_invoice_waiter_expire_or_delete(struct invoices *invoices,
 	}
 }
 
-static void wallet_stmt2invoice_details(const tal_t *ctx,
-					sqlite3_stmt *stmt,
-					struct invoice_details *dtl)
+static struct invoice_details *wallet_stmt2invoice_details(const tal_t *ctx,
+							   sqlite3_stmt *stmt)
 {
+	struct invoice_details *dtl = tal(ctx, struct invoice_details);
 	dtl->state = sqlite3_column_int(stmt, 0);
 
 	sqlite3_column_preimage(stmt, 1, &dtl->r);
 
 	sqlite3_column_sha256(stmt, 2, &dtl->rhash);
 
-	dtl->label = sqlite3_column_json_escaped(ctx, stmt, 3);
+	dtl->label = sqlite3_column_json_escaped(dtl, stmt, 3);
 
 	if (sqlite3_column_type(stmt, 4) != SQLITE_NULL) {
-		dtl->msatoshi = tal(ctx, u64);
+		dtl->msatoshi = tal(dtl, u64);
 		*dtl->msatoshi = sqlite3_column_int64(stmt, 4);
 	} else {
 		dtl->msatoshi = NULL;
@@ -114,14 +114,16 @@ static void wallet_stmt2invoice_details(const tal_t *ctx,
 		dtl->paid_timestamp = sqlite3_column_int64(stmt, 8);
 	}
 
-	dtl->bolt11 = tal_strndup(ctx, sqlite3_column_blob(stmt, 9),
+	dtl->bolt11 = tal_strndup(dtl, sqlite3_column_blob(stmt, 9),
 				  sqlite3_column_bytes(stmt, 9));
 
 	if (sqlite3_column_type(stmt, 10) != SQLITE_NULL)
 		dtl->description = tal_strdup(
-		    ctx, (const char *)sqlite3_column_text(stmt, 10));
+		    dtl, (const char *)sqlite3_column_text(stmt, 10));
 	else
 		dtl->description = NULL;
+
+	return dtl;
 }
 
 struct invoices *invoices_new(const tal_t *ctx,
@@ -492,13 +494,12 @@ bool invoices_iterate(struct invoices *invoices,
 		return true;
 	}
 }
-void invoices_iterator_deref(const tal_t *ctx,
-			     struct invoices *invoices UNUSED,
-			     const struct invoice_iterator *it,
-			     struct invoice_details *details)
+const struct invoice_details *
+invoices_iterator_deref(const tal_t *ctx, struct invoices *invoices UNUSED,
+			const struct invoice_iterator *it)
 {
 	assert(it->p);
-	wallet_stmt2invoice_details(ctx, (sqlite3_stmt*) it->p, details);
+	return wallet_stmt2invoice_details(ctx, (sqlite3_stmt*) it->p);
 }
 
 static s64 get_next_pay_index(struct db *db)
@@ -638,13 +639,13 @@ void invoices_waitone(const tal_t *ctx,
 			   false, invoice.id, cb, cbarg);
 }
 
-void invoices_get_details(const tal_t *ctx,
-			  struct invoices *invoices,
-			  struct invoice invoice,
-			  struct invoice_details *dtl)
+const struct invoice_details *invoices_get_details(const tal_t *ctx,
+						   struct invoices *invoices,
+						   struct invoice invoice)
 {
 	sqlite3_stmt *stmt;
 	int result;
+	struct invoice_details *details;
 
 	stmt = db_prepare(invoices->db,
 			  "SELECT state, payment_key, payment_hash"
@@ -657,7 +658,7 @@ void invoices_get_details(const tal_t *ctx,
 	result = sqlite3_step(stmt);
 	assert(result == SQLITE_ROW);
 
-	wallet_stmt2invoice_details(ctx, stmt, dtl);
-
+	details = wallet_stmt2invoice_details(ctx, stmt);
 	db_stmt_done(stmt);
+	return details;
 }

wallet/invoices.h

@@ -169,13 +169,12 @@ bool invoices_iterate(struct invoices *invoices,
  * @ctx - the owner of the label and msatoshi fields returned.
  * @wallet - the wallet whose invoices are to be iterated over.
  * @iterator - the iterator object to use.
- * @details - pointer to details object to load.
+ * @return The invoice details allocated off of `ctx`
  *
  */
-void invoices_iterator_deref(const tal_t *ctx,
-			     struct invoices *invoices,
-			     const struct invoice_iterator *it,
-			     struct invoice_details *details);
+const struct invoice_details *invoices_iterator_deref(
+	const tal_t *ctx, struct invoices *invoices,
+	const struct invoice_iterator *it);
 
 /**
  * invoices_resolve - Mark an invoice as paid
@@ -239,11 +238,10 @@ void invoices_waitone(const tal_t *ctx,
  * @ctx - the owner of the label and msatoshi fields returned.
  * @invoices - the invoice handler,
  * @invoice - the invoice to get details on.
- * @details - pointer to details object to load.
+ * @return pointer to the invoice details allocated off of `ctx`.
  */
-void invoices_get_details(const tal_t *ctx,
-			  struct invoices *invoices,
-			  struct invoice invoice,
-			  struct invoice_details *details);
+const struct invoice_details *invoices_get_details(const tal_t *ctx,
+						   struct invoices *invoices,
+						   struct invoice invoice);
 
 #endif /* LIGHTNING_WALLET_INVOICES_H */

wallet/test/run-wallet.c

@@ -137,20 +137,18 @@ bool invoices_find_unpaid(struct invoices *invoices UNNEEDED,
 			  const struct sha256 *rhash UNNEEDED)
 { fprintf(stderr, "invoices_find_unpaid called!\n"); abort(); }
 /* Generated stub for invoices_get_details */
-void invoices_get_details(const tal_t *ctx UNNEEDED,
-			  struct invoices *invoices UNNEEDED,
-			  struct invoice invoice UNNEEDED,
-			  struct invoice_details *details UNNEEDED)
+const struct invoice_details *invoices_get_details(const tal_t *ctx UNNEEDED,
+						   struct invoices *invoices UNNEEDED,
+						   struct invoice invoice UNNEEDED)
 { fprintf(stderr, "invoices_get_details called!\n"); abort(); }
 /* Generated stub for invoices_iterate */
 bool invoices_iterate(struct invoices *invoices UNNEEDED,
 		      struct invoice_iterator *it UNNEEDED)
 { fprintf(stderr, "invoices_iterate called!\n"); abort(); }
 /* Generated stub for invoices_iterator_deref */
-void invoices_iterator_deref(const tal_t *ctx UNNEEDED,
-			     struct invoices *invoices UNNEEDED,
-			     const struct invoice_iterator *it UNNEEDED,
-			     struct invoice_details *details UNNEEDED)
+const struct invoice_details *invoices_iterator_deref(
+	const tal_t *ctx UNNEEDED, struct invoices *invoices UNNEEDED,
+	const struct invoice_iterator *it UNNEEDED)
 { fprintf(stderr, "invoices_iterator_deref called!\n"); abort(); }
 /* Generated stub for invoices_load */
 bool invoices_load(struct invoices *invoices UNNEEDED)

wallet/wallet.c

@@ -1444,12 +1444,11 @@ bool wallet_invoice_iterate(struct wallet *wallet,
 {
 	return invoices_iterate(wallet->invoices, it);
 }
-void wallet_invoice_iterator_deref(const tal_t *ctx,
-				   struct wallet *wallet,
-				   const struct invoice_iterator *it,
-				   struct invoice_details *details)
+const struct invoice_details *
+wallet_invoice_iterator_deref(const tal_t *ctx, struct wallet *wallet,
+			      const struct invoice_iterator *it)
 {
-	return invoices_iterator_deref(ctx, wallet->invoices, it, details);
+	return invoices_iterator_deref(ctx, wallet->invoices, it);
 }
 void wallet_invoice_resolve(struct wallet *wallet,
 			    struct invoice invoice,
@@ -1473,15 +1472,14 @@ void wallet_invoice_waitone(const tal_t *ctx,
 {
 	invoices_waitone(ctx, wallet->invoices, invoice, cb, cbarg);
 }
-void wallet_invoice_details(const tal_t *ctx,
-			    struct wallet *wallet,
-			    struct invoice invoice,
-			    struct invoice_details *details)
+
+const struct invoice_details *wallet_invoice_details(const tal_t *ctx,
+						     struct wallet *wallet,
+						     struct invoice invoice)
 {
-	invoices_get_details(ctx, wallet->invoices, invoice, details);
+	return invoices_get_details(ctx, wallet->invoices, invoice);
 }
 
-
 struct htlc_stub *wallet_htlc_stubs(const tal_t *ctx, struct wallet *wallet,
 				    struct channel *chan)
 {

wallet/wallet.h

@@ -605,13 +605,11 @@ bool wallet_invoice_iterate(struct wallet *wallet,
  * @ctx - the owner of the label and msatoshi fields returned.
  * @wallet - the wallet whose invoices are to be iterated over.
  * @iterator - the iterator object to use.
- * @details - pointer to details object to load.
- *
+ * @return pointer to the invoice details allocated off of `ctx`.
  */
-void wallet_invoice_iterator_deref(const tal_t *ctx,
-				   struct wallet *wallet,
-				   const struct invoice_iterator *it,
-				   struct invoice_details *details);
+const struct invoice_details *
+wallet_invoice_iterator_deref(const tal_t *ctx, struct wallet *wallet,
+			      const struct invoice_iterator *it);
 
 /**
  * wallet_invoice_resolve - Mark an invoice as paid
@@ -676,12 +674,11 @@ void wallet_invoice_waitone(const tal_t *ctx,
  * @ctx - the owner of the label and msatoshi fields returned.
  * @wallet - the wallet to query.
  * @invoice - the invoice to get details on.
- * @details - pointer to details object to load.
+ * @return pointer to the invoice details allocated off of `ctx`.
  */
-void wallet_invoice_details(const tal_t *ctx,
-			    struct wallet *wallet,
-			    struct invoice invoice,
-			    struct invoice_details *details);
+const struct invoice_details *wallet_invoice_details(const tal_t *ctx,
+						     struct wallet *wallet,
+						     struct invoice invoice);
 
 /**
  * wallet_htlc_stubs - Retrieve HTLC stubs for the given channel