tx: Strengthen transaction construction checks

We roll the `elements_add_fee_output` function and the cropping of
overallocated arrays into the `bitcoin_tx_finalize` function. This is supposed
to be the final cleanup and compaction step before a tx can be sent to bitcoin
or passed off to other daemons.

This is the cleanup promised in ElementsProject#3491

Author: cdecker

bitcoin/tx.c

@@ -101,25 +101,29 @@ static struct amount_sat bitcoin_tx_compute_fee(const struct bitcoin_tx *tx)
 int elements_tx_add_fee_output(struct bitcoin_tx *tx)
 {
 	struct amount_sat fee = bitcoin_tx_compute_fee(tx);
-	int pos = -1;
+	int pos;
 	struct witscript *w;
 
 	/* If we aren't using elements, we don't add explicit fee outputs */
 	if (!chainparams->is_elements || amount_sat_eq(fee, AMOUNT_SAT(0)))
 		return -1;
 
 	/* Try to find any existing fee output */
-	for (int i=0; i<tx->wtx->num_outputs; i++) {
-		if (elements_tx_output_is_fee(tx, i)) {
-			assert(pos == -1);
-			pos = i;
-		}
+	for (pos = 0; pos < tx->wtx->num_outputs; pos++) {
+		if (elements_tx_output_is_fee(tx, pos))
+			break;
 	}
 
-	if (pos == -1) {
+	if (pos == tx->wtx->num_outputs) {
 		w = tal(tx->output_witscripts, struct witscript);
 		w->ptr = tal_arr(w, u8, 0);
-		tx->output_witscripts[tx->wtx->num_outputs] = w;
+
+		/* Make sure we have a place to stash the witness script in. */
+		if (tal_count(tx->output_witscripts) < pos + 1) {
+			tal_resize(&tx->output_witscripts, pos + 1);
+		}
+		tx->output_witscripts[pos] = w;
+
 		return bitcoin_tx_add_output(tx, NULL, fee);
 	} else {
 		bitcoin_tx_output_set_amount(tx, pos, fee);
@@ -160,6 +164,12 @@ bool bitcoin_tx_check(const struct bitcoin_tx *tx)
 	size_t written;
 	int flags = WALLY_TX_FLAG_USE_WITNESS;
 
+	if (tal_count(tx->input_amounts) != tx->wtx->num_inputs)
+		return false;
+
+	if (tal_count(tx->output_witscripts) != tx->wtx->num_outputs)
+		return false;
+
 	if (wally_tx_get_length(tx->wtx, flags, &written) != WALLY_OK)
 		return false;
 
@@ -408,6 +418,19 @@ struct bitcoin_tx *bitcoin_tx(const tal_t *ctx,
 	return tx;
 }
 
+void bitcoin_tx_finalize(struct bitcoin_tx *tx)
+{
+	size_t num_outputs, num_inputs;
+	elements_tx_add_fee_output(tx);
+
+	num_outputs = tx->wtx->num_outputs;
+	tal_resize(&(tx->output_witscripts), num_outputs);
+
+	num_inputs = tx->wtx->num_inputs;
+	tal_resize(&tx->input_amounts, num_inputs);
+	assert(bitcoin_tx_check(tx));
+}
+
 struct bitcoin_tx *pull_bitcoin_tx(const tal_t *ctx, const u8 **cursor,
 				   size_t *max)
 {
@@ -470,6 +493,12 @@ struct bitcoin_tx *bitcoin_tx_from_hex(const tal_t *ctx, const char *hex,
 		goto fail_free_tx;
 
 	tal_free(linear_tx);
+
+	tx->output_witscripts =
+	    tal_arrz(tx, struct witscript *, tx->wtx->num_outputs);
+	tx->input_amounts =
+	    tal_arrz(tx, struct amount_sat *, tx->wtx->num_inputs);
+
 	return tx;
 
 fail_free_tx:

bitcoin/tx.h

@@ -156,6 +156,15 @@ void bitcoin_tx_input_get_txid(const struct bitcoin_tx *tx, int innum,
  */
 bool bitcoin_tx_check(const struct bitcoin_tx *tx);
 
+
+/**
+ * Finalize a transaction by truncating overallocated and temporary
+ * fields. This includes adding a fee output for elements transactions or
+ * adjusting an existing fee output, and resizing metadata arrays for inputs
+ * and outputs.
+ */
+void bitcoin_tx_finalize(struct bitcoin_tx *tx);
+
 /**
  * Add an explicit fee output if necessary.
  *

channeld/commit_tx.c

@@ -305,8 +305,8 @@ struct bitcoin_tx *commit_tx(const tal_t *ctx,
 	u32 sequence = (0x80000000 | ((obscured_commitment_number>>24) & 0xFFFFFF));
 	bitcoin_tx_add_input(tx, funding_txid, funding_txout, sequence, funding, NULL);
 
-	elements_tx_add_fee_output(tx);
-	tal_resize(&(tx->output_witscripts), tx->wtx->num_outputs);
+	bitcoin_tx_finalize(tx);
+	assert(bitcoin_tx_check(tx));
 
 	return tx;
 }

common/close_tx.c

@@ -60,8 +60,8 @@ struct bitcoin_tx *create_close_tx(const tal_t *ctx,
 		return tal_free(tx);
 
 	permute_outputs(tx, NULL, NULL);
-	elements_tx_add_fee_output(tx);
 
+	bitcoin_tx_finalize(tx);
 	assert(bitcoin_tx_check(tx));
 	return tx;
 }

common/funding_tx.c

@@ -50,7 +50,7 @@ struct bitcoin_tx *funding_tx(const tal_t *ctx,
 
 	permute_inputs(tx, (const void **)utxomap);
 
-	elements_tx_add_fee_output(tx);
+	bitcoin_tx_finalize(tx);
 	assert(bitcoin_tx_check(tx));
 	return tx;
 }

common/htlc_tx.c

@@ -61,7 +61,9 @@ static struct bitcoin_tx *htlc_tx(const tal_t *ctx,
 	wscript = bitcoin_wscript_htlc_tx(tx, to_self_delay, revocation_pubkey,
 					  local_delayedkey);
 	bitcoin_tx_add_output(tx, scriptpubkey_p2wsh(tx, wscript), amount);
-	elements_tx_add_fee_output(tx);
+
+	bitcoin_tx_finalize(tx);
+	assert(bitcoin_tx_check(tx));
 
 	tal_free(wscript);
 

common/initial_commit_tx.c

@@ -241,9 +241,7 @@ struct bitcoin_tx *initial_commit_tx(const tal_t *ctx,
 	sequence = (0x80000000 | ((obscured_commitment_number>>24) & 0xFFFFFF));
 	bitcoin_tx_add_input(tx, funding_txid, funding_txout, sequence, funding, NULL);
 
-	elements_tx_add_fee_output(tx);
-	tal_resize(&(tx->output_witscripts), tx->wtx->num_outputs);
-
+	bitcoin_tx_finalize(tx);
 	assert(bitcoin_tx_check(tx));
 
 	return tx;

common/withdraw_tx.c

@@ -52,7 +52,8 @@ struct bitcoin_tx *withdraw_tx(const tal_t *ctx,
 		*change_outnum = -1;
 
 	permute_inputs(tx, (const void **)utxos);
-	elements_tx_add_fee_output(tx);
+
+	bitcoin_tx_finalize(tx);
 	assert(bitcoin_tx_check(tx));
 	return tx;
 }