Fix broken tests on elements and various cleanups

Makefile

@@ -272,7 +272,7 @@ ifeq ($(PYTEST),)
 	exit 1
 else
 # Explicitly hand DEVELOPER and VALGRIND so you can override on make cmd line.
-	PYTHONPATH=`pwd`/contrib/pyln-client:`pwd`/contrib/pyln-testing:`pwd`/contrib/pylightning:`pwd`/contrib/pyln-proto/:$(PYTHONPATH) TEST_DEBUG=1 DEVELOPER=$(DEVELOPER) VALGRIND=$(VALGRIND) $(PYTEST) tests/ $(PYTEST_OPTS)
+	PYTHONPATH=`pwd`/contrib/pyln-client:`pwd`/contrib/pyln-testing:`pwd`/contrib/pyln-proto/:$(PYTHONPATH) TEST_DEBUG=1 DEVELOPER=$(DEVELOPER) VALGRIND=$(VALGRIND) $(PYTEST) tests/ $(PYTEST_OPTS)
 endif
 
 # Keep includes in alpha order.

bitcoin/tx.c

@@ -98,28 +98,42 @@ static struct amount_sat bitcoin_tx_compute_fee(const struct bitcoin_tx *tx)
 	return fee;
 }
 
-int elements_tx_add_fee_output(struct bitcoin_tx *tx)
+/**
+ * Add an explicit fee output if necessary.
+ *
+ * An explicit fee output is only necessary if we are using an elements
+ * transaction, and we have a non-zero fee. This method may be called multiple
+ * times.
+ *
+ * Returns the position of the fee output, or -1 in the case of non-elements
+ * transactions.
+ */
+static 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 +174,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 +428,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 +503,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,16 +156,13 @@ void bitcoin_tx_input_get_txid(const struct bitcoin_tx *tx, int innum,
  */
 bool bitcoin_tx_check(const struct bitcoin_tx *tx);
 
+
 /**
- * Add an explicit fee output if necessary.
- *
- * An explicit fee output is only necessary if we are using an elements
- * transaction, and we have a non-zero fee. This method may be called multiple
- * times.
- *
- * Returns the position of the fee output, or -1 in the case of non-elements
- * transactions.
+ * 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.
  */
-int elements_tx_add_fee_output(struct bitcoin_tx *tx);
+void bitcoin_tx_finalize(struct bitcoin_tx *tx);
 
 #endif /* LIGHTNING_BITCOIN_TX_H */

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;
 }

contrib/plugins/helloworld.py

@@ -1,5 +1,5 @@
 #!/usr/bin/env python3
-from lightning import Plugin
+from pyln.client import Plugin
 import time
 
 plugin = Plugin()

contrib/pyln-testing/pyln/testing/fixtures.py

@@ -1,12 +1,13 @@
 from concurrent import futures
 from pyln.testing.db import SqliteDbProvider, PostgresDbProvider
-from pyln.testing.utils import NodeFactory, BitcoinD, ElementsD, env, DEVELOPER, LightningNode
+from pyln.testing.utils import NodeFactory, BitcoinD, ElementsD, env, DEVELOPER, LightningNode, TEST_DEBUG
 
 import logging
 import os
 import pytest
 import re
 import shutil
+import sys
 import tempfile
 
 
@@ -28,6 +29,18 @@ def test_base_dir():
         shutil.rmtree(directory)
 
 
+@pytest.fixture(autouse=True)
+def setup_logging():
+    logger = logging.getLogger()
+    before_handlers = list(logger.handlers)
+
+    if TEST_DEBUG:
+        logging.basicConfig(level=logging.DEBUG, stream=sys.stdout)
+
+    yield
+    logger.handlers = before_handlers
+
+
 @pytest.fixture
 def directory(request, test_base_dir, test_name):
     """Return a per-test specific directory.
@@ -41,6 +54,9 @@ def directory(request, test_base_dir, test_name):
     directory = os.path.join(test_base_dir, "{}_{}".format(test_name, __attempts[test_name]))
     request.node.has_errors = False
 
+    if not os.path.exists(directory):
+        os.makedirs(directory)
+
     yield directory
 
     # This uses the status set in conftest.pytest_runtest_makereport to
@@ -52,7 +68,12 @@ def directory(request, test_base_dir, test_name):
     failed = not outcome or request.node.has_errors or outcome != 'passed'
 
     if not failed:
-        shutil.rmtree(directory)
+        try:
+            shutil.rmtree(directory)
+        except Exception:
+            files = [os.path.join(dp, f) for dp, dn, fn in os.walk(directory) for f in fn]
+            print("Directory still contains files:", files)
+            raise
     else:
         logging.debug("Test execution failed, leaving the test directory {} intact.".format(directory))
 

contrib/pyln-testing/pyln/testing/utils.py

@@ -17,7 +17,6 @@
 import string
 import struct
 import subprocess
-import sys
 import threading
 import time
 
@@ -69,10 +68,6 @@ def env(name, default=None):
 TIMEOUT = int(env("TIMEOUT", 180 if SLOW_MACHINE else 60))
 
 
-if TEST_DEBUG:
-    logging.basicConfig(level=logging.DEBUG, stream=sys.stdout)
-
-
 def wait_for(success, timeout=TIMEOUT):
     start_time = time.time()
     interval = 0.25

onchaind/onchaind.c

@@ -148,7 +148,7 @@ static bool grind_htlc_tx_fee(struct amount_sat *fee,
 			break;
 
 		bitcoin_tx_output_set_amount(tx, 0, out);
-		elements_tx_add_fee_output(tx);
+		bitcoin_tx_finalize(tx);
 		if (!check_tx_sig(tx, 0, NULL, wscript,
 				  &keyset->other_htlc_key, remotesig))
 			continue;
@@ -196,7 +196,7 @@ static bool set_htlc_timeout_fee(struct bitcoin_tx *tx,
 			      type_to_string(tmpctx, struct bitcoin_tx, tx));
 
 	bitcoin_tx_output_set_amount(tx, 0, amount);
-	elements_tx_add_fee_output(tx);
+	bitcoin_tx_finalize(tx);
 	return check_tx_sig(tx, 0, NULL, wscript,
 			    &keyset->other_htlc_key, remotesig);
 }
@@ -240,7 +240,7 @@ static void set_htlc_success_fee(struct bitcoin_tx *tx,
 			      type_to_string(tmpctx, struct amount_sat, &fee),
 			      type_to_string(tmpctx, struct bitcoin_tx, tx));
 	bitcoin_tx_output_set_amount(tx, 0, amt);
-	elements_tx_add_fee_output(tx);
+	bitcoin_tx_finalize(tx);
 
 	if (check_tx_sig(tx, 0, NULL, wscript,
 			 &keyset->other_htlc_key, remotesig))
@@ -372,7 +372,7 @@ static struct bitcoin_tx *tx_to_us(const tal_t *ctx,
 					     &amt));
 	}
 	bitcoin_tx_output_set_amount(tx, 0, amt);
-	elements_tx_add_fee_output(tx);
+	bitcoin_tx_finalize(tx);
 
 	if (!wire_sync_write(HSM_FD, take(hsm_sign_msg(NULL, tx, wscript))))
 		status_failed(STATUS_FAIL_HSM_IO, "Writing sign request to hsm");

tests/fixtures.py

@@ -1,5 +1,5 @@
 from utils import DEVELOPER, TEST_NETWORK  # noqa: F401,F403
-from pyln.testing.fixtures import directory, test_base_dir, test_name, chainparams, node_factory, bitcoind, teardown_checks, db_provider, executor  # noqa: F401,F403
+from pyln.testing.fixtures import directory, test_base_dir, test_name, chainparams, node_factory, bitcoind, teardown_checks, db_provider, executor, setup_logging  # noqa: F401,F403
 from pyln.testing import utils
 
 import pytest

tests/plugins/accepter_close_to.py

@@ -9,7 +9,7 @@
       - otherwise: we don't include the close_to
 """
 
-from lightning import Plugin, Millisatoshi
+from pyln.client import Plugin, Millisatoshi
 
 plugin = Plugin()
 

tests/plugins/asynctest.py

@@ -6,7 +6,7 @@
 will then return the argument of the fifth call.
 
 """
-from lightning import Plugin
+from pyln.client import Plugin
 
 plugin = Plugin()
 

tests/plugins/broken.py

@@ -3,7 +3,7 @@
 misbehaving plugin via RPC.
 """
 
-from lightning import Plugin
+from pyln.client import Plugin
 import an_unexistent_module_that_will_make_me_crash
 
 plugin = Plugin(dynamic=False)

tests/plugins/dblog.py

@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
 """This plugin is used to check that db_write calls are working correctly.
 """
-from lightning import Plugin, RpcError
+from pyln.client import Plugin, RpcError
 import sqlite3
 
 plugin = Plugin()

tests/plugins/fail_htlcs.py

@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 
-from lightning import Plugin
+from pyln.client import Plugin
 
 plugin = Plugin()
 

tests/plugins/forward_payment_status.py

@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
 """This plugin is used to check that forward_event calls are working correctly.
 """
-from lightning import Plugin
+from pyln.client import Plugin
 
 plugin = Plugin()
 

tests/plugins/hold_htlcs.py

@@ -7,7 +7,7 @@
 """
 
 
-from lightning import Plugin
+from pyln.client import Plugin
 import json
 import os
 import tempfile

tests/plugins/hold_invoice.py

@@ -2,7 +2,7 @@
 """Simple plugin to allow testing while closing of HTLC is delayed.
 """
 
-from lightning import Plugin
+from pyln.client import Plugin
 import time
 
 plugin = Plugin()

tests/plugins/millisatoshis.py

@@ -1,5 +1,5 @@
 #!/usr/bin/env python3
-from lightning import Plugin, Millisatoshi
+from pyln.client import Plugin, Millisatoshi
 
 
 plugin = Plugin(autopatch=True)