lightningd: clean up htlc_in->shared_secret to be optional.

We currently use 'all-zeroes' as 'unknown', but NULL is more natural even if we have to send it as all-zeroes over the wire due to expressiveness limitations in our generation code. Signed-off-by: Rusty Russell <[email protected]>
sha-265 · Jan 8, 2019 · 7e01efb · 7e01efb
1 parent 90cdc47
commit 7e01efb
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 28 deletions.
diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c
@@ -111,7 +111,7 @@ struct htlc_in *new_htlc_in(const tal_t *ctx,
 			    struct channel *channel, u64 id,
 			    u64 msatoshi, u32 cltv_expiry,
 			    const struct sha256 *payment_hash,
-			    const struct secret *shared_secret,
+			    const struct secret *shared_secret TAKES,
 			    const u8 *onion_routing_packet)
 {
 	struct htlc_in *hin = tal(ctx, struct htlc_in);
@@ -122,7 +122,10 @@ struct htlc_in *new_htlc_in(const tal_t *ctx,
 	hin->msatoshi = msatoshi;
 	hin->cltv_expiry = cltv_expiry;
 	hin->payment_hash = *payment_hash;
-	hin->shared_secret = *shared_secret;
+	if (shared_secret)
+		hin->shared_secret = tal_dup(hin, struct secret, shared_secret);
+	else
+		hin->shared_secret = NULL;
 	memcpy(hin->onion_routing_packet, onion_routing_packet,
 	       sizeof(hin->onion_routing_packet));
 

diff --git a/lightningd/htlc_end.h b/lightningd/htlc_end.h
@@ -31,8 +31,8 @@ struct htlc_in {
 	/* Onion information */
 	u8 onion_routing_packet[TOTAL_PACKET_SIZE];
 
-	/* Shared secret for us to send any failure message. */
-	struct secret shared_secret;
+	/* Shared secret for us to send any failure message (NULL if malformed) */
+	struct secret *shared_secret;
 
 	/* If a local error, this is non-zero. */
 	enum onion_type failcode;
@@ -124,7 +124,7 @@ struct htlc_in *new_htlc_in(const tal_t *ctx,
 			    struct channel *channel, u64 id,
 			    u64 msatoshi, u32 cltv_expiry,
 			    const struct sha256 *payment_hash,
-			    const struct secret *shared_secret,
+			    const struct secret *shared_secret TAKES,
 			    const u8 *onion_routing_packet);
 
 /* You need to set the ID, then connect_htlc_out this! */

diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c
@@ -640,30 +640,25 @@ static bool peer_accepted_htlc(struct channel *channel,
 	 * a subset of the cltv check done in handle_localpay and
 	 * forward_htlc. */
 
-	/* channeld tests this, so it should have set ss to zeroes. */
-	op = parse_onionpacket(tmpctx, hin->onion_routing_packet,
-			       sizeof(hin->onion_routing_packet));
-	if (!op) {
-		if (!memeqzero(&hin->shared_secret, sizeof(hin->shared_secret))){
-			channel_internal_error(channel,
-				   "bad onion in got_revoke: %s",
-				   tal_hexstr(channel, hin->onion_routing_packet,
-					     sizeof(hin->onion_routing_packet)));
-			return false;
-		}
-		/* FIXME: could be bad version, bad key. */
-		*failcode = WIRE_INVALID_ONION_VERSION;
+	/* Channeld sets this to NULL if couldn't parse onion */
+	if (!hin->shared_secret) {
+		*failcode = WIRE_INVALID_ONION_KEY;
 		goto out;
 	}
 
-	/* Channeld sets this to zero if HSM won't ecdh it */
-	if (memeqzero(&hin->shared_secret, sizeof(hin->shared_secret))) {
-		*failcode = WIRE_INVALID_ONION_KEY;
-		goto out;
+	/* channeld tests this, so it should pass. */
+	op = parse_onionpacket(tmpctx, hin->onion_routing_packet,
+			       sizeof(hin->onion_routing_packet));
+	if (!op) {
+		channel_internal_error(channel,
+				       "bad onion in got_revoke: %s",
+				       tal_hexstr(channel, hin->onion_routing_packet,
+						  sizeof(hin->onion_routing_packet)));
+		return false;
 	}
 
 	/* If it's crap, not channeld's fault, just fail it */
-	rs = process_onionpacket(tmpctx, op, hin->shared_secret.data,
+	rs = process_onionpacket(tmpctx, op, hin->shared_secret->data,
 				 hin->payment_hash.u.u8,
 				 sizeof(hin->payment_hash));
 	if (!rs) {
@@ -1115,6 +1110,11 @@ static bool channel_added_their_htlc(struct channel *channel,
 		return false;
 	}
 
+	/* FIXME: Our wire generator can't handle optional elems in arrays,
+	 * so we translate all-zero-shared-secret to NULL. */
+	if (memeqzero(shared_secret, sizeof(&shared_secret)))
+		shared_secret = NULL;
+
 	/* This stays around even if we fail it immediately: it *is*
 	 * part of the current commitment. */
 	hin = new_htlc_in(channel, channel, added->id, added->amount_msat,

diff --git a/wallet/wallet.c b/wallet/wallet.c
@@ -2,6 +2,7 @@
 #include "wallet.h"
 
 #include <bitcoin/script.h>
+#include <ccan/mem/mem.h>
 #include <ccan/tal/str/str.h>
 #include <common/key_derive.h>
 #include <common/wireaddr.h>
@@ -1186,8 +1187,11 @@ void wallet_htlc_save_in(struct wallet *wallet,
 		sqlite3_bind_null(stmt, 7);
 	sqlite3_bind_int(stmt, 8, in->hstate);
 
-	sqlite3_bind_blob(stmt, 9, &in->shared_secret,
-			  sizeof(in->shared_secret), SQLITE_TRANSIENT);
+	if (!in->shared_secret)
+		sqlite3_bind_null(stmt, 9);
+	else
+		sqlite3_bind_blob(stmt, 9, in->shared_secret,
+				  sizeof(*in->shared_secret), SQLITE_TRANSIENT);
 
 	sqlite3_bind_blob(stmt, 10, &in->onion_routing_packet,
 			  sizeof(in->onion_routing_packet), SQLITE_TRANSIENT);
@@ -1314,9 +1318,18 @@ static bool wallet_stmt2htlc_in(struct channel *channel,
 	in->failuremsg = sqlite3_column_arr(in, stmt, 8, u8);
 	in->failcode = sqlite3_column_int(stmt, 9);
 
-	assert(sqlite3_column_bytes(stmt, 11) == sizeof(struct secret));
-	memcpy(&in->shared_secret, sqlite3_column_blob(stmt, 11),
-	       sizeof(struct secret));
+	if (sqlite3_column_type(stmt, 11) == SQLITE_NULL) {
+		in->shared_secret = NULL;
+	} else {
+		assert(sqlite3_column_bytes(stmt, 11) == sizeof(struct secret));
+		in->shared_secret = tal(in, struct secret);
+		memcpy(in->shared_secret, sqlite3_column_blob(stmt, 11),
+		       sizeof(struct secret));
+#ifdef COMPAT_V062
+		if (memeqzero(in->shared_secret, sizeof(*in->shared_secret)))
+			in->shared_secret = tal_free(in->shared_secret);
+#endif
+	}
 
 	return ok;
 }