From b5a41f9fa13ede10f4dcb261c4a4492b3c1fe0c2 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sun, 26 Jan 2020 13:44:53 +0100 Subject: [PATCH 01/10] pytest: Actually make sure that the direcory exists Some tests may not spawn a node at all, so make sure that our assumption that the directory exists in the fixture cleanup is correct by creating the directory. --- contrib/pyln-testing/pyln/testing/fixtures.py | 3 +++ tests/test_onion.py | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/fixtures.py b/contrib/pyln-testing/pyln/testing/fixtures.py index 3d21394dc193..cda03c263961 100644 --- a/contrib/pyln-testing/pyln/testing/fixtures.py +++ b/contrib/pyln-testing/pyln/testing/fixtures.py @@ -41,6 +41,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 diff --git a/tests/test_onion.py b/tests/test_onion.py index 80f85d830f99..fb1142c9afed 100644 --- a/tests/test_onion.py +++ b/tests/test_onion.py @@ -30,7 +30,6 @@ def oniontool(): def test_onion(directory, oniontool): """ Generate a 5 hop onion and then decode it. """ - os.makedirs(directory) tempfile = os.path.join(directory, 'onion') out = subprocess.check_output( [oniontool, 'generate'] + pubkeys @@ -53,7 +52,6 @@ def store_onion(o): def test_rendezvous_onion(directory, oniontool): """Create a compressed onion, decompress it at the RV node and then forward normally. """ - os.makedirs(directory) tempfile = os.path.join(directory, 'onion') out = subprocess.check_output( [oniontool, '--rendezvous-id', pubkeys[0], 'generate'] + pubkeys From 3eaa7a527a32503fae11f2dbd8a98d8db00c74f4 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sat, 8 Feb 2020 21:26:55 +0100 Subject: [PATCH 02/10] 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 #3491 --- bitcoin/tx.c | 45 +++++++++++++++++++++++++++++++------- bitcoin/tx.h | 9 ++++++++ channeld/commit_tx.c | 4 ++-- common/close_tx.c | 2 +- common/funding_tx.c | 2 +- common/htlc_tx.c | 4 +++- common/initial_commit_tx.c | 4 +--- common/withdraw_tx.c | 3 ++- 8 files changed, 56 insertions(+), 17 deletions(-) diff --git a/bitcoin/tx.c b/bitcoin/tx.c index faead344dd84..8a8d727a0193 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -101,7 +101,7 @@ 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 */ @@ -109,17 +109,21 @@ int elements_tx_add_fee_output(struct bitcoin_tx *tx) return -1; /* Try to find any existing fee output */ - for (int i=0; iwtx->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: diff --git a/bitcoin/tx.h b/bitcoin/tx.h index d36f66ba9cce..8c4d62e68664 100644 --- a/bitcoin/tx.h +++ b/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. * diff --git a/channeld/commit_tx.c b/channeld/commit_tx.c index 3663d5e3eb98..44be92c563fc 100644 --- a/channeld/commit_tx.c +++ b/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; } diff --git a/common/close_tx.c b/common/close_tx.c index cb2d02c2e939..caf42fe7c30d 100644 --- a/common/close_tx.c +++ b/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; } diff --git a/common/funding_tx.c b/common/funding_tx.c index e299b440443f..f75cbd9d25f5 100644 --- a/common/funding_tx.c +++ b/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; } diff --git a/common/htlc_tx.c b/common/htlc_tx.c index 2fc8950eb8a8..d3669b17408d 100644 --- a/common/htlc_tx.c +++ b/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); diff --git a/common/initial_commit_tx.c b/common/initial_commit_tx.c index 39a7818b726b..f1a451cca062 100644 --- a/common/initial_commit_tx.c +++ b/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; diff --git a/common/withdraw_tx.c b/common/withdraw_tx.c index a13c58c0dd66..1aee140bd288 100644 --- a/common/withdraw_tx.c +++ b/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; } From 85493081790598b476b5433d5951bc015852d5f7 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Sat, 8 Feb 2020 21:30:03 +0100 Subject: [PATCH 03/10] tx: Make elements_add_fee_output private --- bitcoin/tx.c | 12 +++++++++++- bitcoin/tx.h | 12 ------------ onchaind/onchaind.c | 8 ++++---- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/bitcoin/tx.c b/bitcoin/tx.c index 8a8d727a0193..6ac439d84e58 100644 --- a/bitcoin/tx.c +++ b/bitcoin/tx.c @@ -98,7 +98,17 @@ 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; diff --git a/bitcoin/tx.h b/bitcoin/tx.h index 8c4d62e68664..29eeb897484f 100644 --- a/bitcoin/tx.h +++ b/bitcoin/tx.h @@ -165,16 +165,4 @@ bool bitcoin_tx_check(const struct bitcoin_tx *tx); */ void bitcoin_tx_finalize(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. - */ -int elements_tx_add_fee_output(struct bitcoin_tx *tx); - #endif /* LIGHTNING_BITCOIN_TX_H */ diff --git a/onchaind/onchaind.c b/onchaind/onchaind.c index b53d1db7a7f2..9209721e0fc9 100644 --- a/onchaind/onchaind.c +++ b/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"); From 9aeb50193e261072836f0a967713ba5f0c770041 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 11 Feb 2020 23:04:21 +0100 Subject: [PATCH 04/10] pyln: Migrate remaining uses of the deprecated pylightning module `pylightning` is not much more than an alias for `pyln-client`, so this removes the need to install that as well just to run the tests. --- Makefile | 2 +- contrib/plugins/helloworld.py | 2 +- tests/plugins/accepter_close_to.py | 2 +- tests/plugins/asynctest.py | 2 +- tests/plugins/broken.py | 2 +- tests/plugins/dblog.py | 2 +- tests/plugins/fail_htlcs.py | 2 +- tests/plugins/forward_payment_status.py | 2 +- tests/plugins/hold_htlcs.py | 2 +- tests/plugins/hold_invoice.py | 2 +- tests/plugins/millisatoshis.py | 2 +- tests/plugins/misc_notifications.py | 2 +- tests/plugins/options.py | 2 +- tests/plugins/pretend_badlog.py | 2 +- tests/plugins/print_htlc_onion.py | 2 +- tests/plugins/reject.py | 2 +- tests/plugins/reject_odd_funding_amounts.py | 2 +- tests/plugins/reject_some_invoices.py | 2 +- tests/plugins/rpc_command.py | 2 +- tests/plugins/sendpay_notifications.py | 2 +- tests/plugins/shortcircuit.py | 2 +- tests/plugins/slow_init.py | 2 +- tests/plugins/static.py | 2 +- tests/plugins/utf8.py | 2 +- 24 files changed, 24 insertions(+), 24 deletions(-) diff --git a/Makefile b/Makefile index 9f233ab3e3da..704c3c69c4f5 100644 --- a/Makefile +++ b/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. diff --git a/contrib/plugins/helloworld.py b/contrib/plugins/helloworld.py index d74b9662778f..1a5ede345676 100755 --- a/contrib/plugins/helloworld.py +++ b/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() diff --git a/tests/plugins/accepter_close_to.py b/tests/plugins/accepter_close_to.py index 1b027b74fa6e..3711154d84f8 100755 --- a/tests/plugins/accepter_close_to.py +++ b/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() diff --git a/tests/plugins/asynctest.py b/tests/plugins/asynctest.py index 97dd4df61db9..9566a670c873 100755 --- a/tests/plugins/asynctest.py +++ b/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() diff --git a/tests/plugins/broken.py b/tests/plugins/broken.py index 28b871c88220..4a88b9bfe665 100755 --- a/tests/plugins/broken.py +++ b/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) diff --git a/tests/plugins/dblog.py b/tests/plugins/dblog.py index 85a0ade61638..ef9eb79e16b8 100755 --- a/tests/plugins/dblog.py +++ b/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() diff --git a/tests/plugins/fail_htlcs.py b/tests/plugins/fail_htlcs.py index 9141549a0728..59a37b3ff994 100755 --- a/tests/plugins/fail_htlcs.py +++ b/tests/plugins/fail_htlcs.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/forward_payment_status.py b/tests/plugins/forward_payment_status.py index 1bb35a8108c4..12a15aee56c2 100755 --- a/tests/plugins/forward_payment_status.py +++ b/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() diff --git a/tests/plugins/hold_htlcs.py b/tests/plugins/hold_htlcs.py index cd879934339a..3b9e5caaa53a 100755 --- a/tests/plugins/hold_htlcs.py +++ b/tests/plugins/hold_htlcs.py @@ -7,7 +7,7 @@ """ -from lightning import Plugin +from pyln.client import Plugin import json import os import tempfile diff --git a/tests/plugins/hold_invoice.py b/tests/plugins/hold_invoice.py index b3190db5e336..b3a0f376045f 100755 --- a/tests/plugins/hold_invoice.py +++ b/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() diff --git a/tests/plugins/millisatoshis.py b/tests/plugins/millisatoshis.py index 5404aa8e086d..2155a3fd8eac 100755 --- a/tests/plugins/millisatoshis.py +++ b/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) diff --git a/tests/plugins/misc_notifications.py b/tests/plugins/misc_notifications.py index 9742b4263c4b..79d416abb9f3 100755 --- a/tests/plugins/misc_notifications.py +++ b/tests/plugins/misc_notifications.py @@ -4,7 +4,7 @@ Only used for 'channel_opened' for now. """ -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/options.py b/tests/plugins/options.py index cb08d0b0bb8b..27e1874d7d35 100755 --- a/tests/plugins/options.py +++ b/tests/plugins/options.py @@ -3,7 +3,7 @@ The plugin offers 3 options, one of each supported type. """ -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/pretend_badlog.py b/tests/plugins/pretend_badlog.py index 23c7ec877765..a255fe11874b 100755 --- a/tests/plugins/pretend_badlog.py +++ b/tests/plugins/pretend_badlog.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 """This plugin is used to check that warning(unusual/broken level log) calls are working correctly. """ -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/print_htlc_onion.py b/tests/plugins/print_htlc_onion.py index a97cc0ced6a1..f1557dd20190 100755 --- a/tests/plugins/print_htlc_onion.py +++ b/tests/plugins/print_htlc_onion.py @@ -5,7 +5,7 @@ """ -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/reject.py b/tests/plugins/reject.py index da39519ee3ae..cf34b49b9e6b 100755 --- a/tests/plugins/reject.py +++ b/tests/plugins/reject.py @@ -7,7 +7,7 @@ """ -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/reject_odd_funding_amounts.py b/tests/plugins/reject_odd_funding_amounts.py index 6dc5dc310bde..eb3460f91ec2 100755 --- a/tests/plugins/reject_odd_funding_amounts.py +++ b/tests/plugins/reject_odd_funding_amounts.py @@ -4,7 +4,7 @@ We just refuse to let them open channels with an odd amount of millisatoshis. """ -from lightning import Plugin, Millisatoshi +from pyln.client import Plugin, Millisatoshi plugin = Plugin() diff --git a/tests/plugins/reject_some_invoices.py b/tests/plugins/reject_some_invoices.py index 1c5b3d2cd18a..321e4e7a2d52 100755 --- a/tests/plugins/reject_some_invoices.py +++ b/tests/plugins/reject_some_invoices.py @@ -4,7 +4,7 @@ We just refuse to let them pay invoices with preimages divisible by 16. """ -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/rpc_command.py b/tests/plugins/rpc_command.py index 3d951246ef27..a993a8d6a045 100755 --- a/tests/plugins/rpc_command.py +++ b/tests/plugins/rpc_command.py @@ -2,7 +2,7 @@ """ This plugin is used to test the `rpc_command` hook. """ -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/sendpay_notifications.py b/tests/plugins/sendpay_notifications.py index 418fb9434115..0f83290d24a5 100755 --- a/tests/plugins/sendpay_notifications.py +++ b/tests/plugins/sendpay_notifications.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 """This plugin is used to check that sendpay_success and sendpay_failure calls are working correctly. """ -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/shortcircuit.py b/tests/plugins/shortcircuit.py index 8403b68ec141..bdb088c15c58 100755 --- a/tests/plugins/shortcircuit.py +++ b/tests/plugins/shortcircuit.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() diff --git a/tests/plugins/slow_init.py b/tests/plugins/slow_init.py index 8587531be7e1..82881d7d3d40 100755 --- a/tests/plugins/slow_init.py +++ b/tests/plugins/slow_init.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -from lightning import Plugin +from pyln.client import Plugin import time plugin = Plugin() diff --git a/tests/plugins/static.py b/tests/plugins/static.py index ff89eeffc106..daa9ac3fa079 100755 --- a/tests/plugins/static.py +++ b/tests/plugins/static.py @@ -5,7 +5,7 @@ has been started. """ -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin(dynamic=False) diff --git a/tests/plugins/utf8.py b/tests/plugins/utf8.py index 52cce06d4ace..16c3afe45083 100755 --- a/tests/plugins/utf8.py +++ b/tests/plugins/utf8.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -from lightning import Plugin +from pyln.client import Plugin plugin = Plugin() From 374d4ee36bdbb448755283e0c6981cb88fe873bd Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 19 Feb 2020 17:51:55 +0100 Subject: [PATCH 05/10] pyln-testing: Print a list of files if we can't remove the test dir For some reason we fail to remove the test directory in some cases. My hypothesis is that it is a daemon that is not completely shut down yet, and still writes to the directory. This commit intercepts the error, prints any files in the directory and re-raises the error. This should allow us to debug the reappears. --- contrib/pyln-testing/pyln/testing/fixtures.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contrib/pyln-testing/pyln/testing/fixtures.py b/contrib/pyln-testing/pyln/testing/fixtures.py index cda03c263961..a5e88832298c 100644 --- a/contrib/pyln-testing/pyln/testing/fixtures.py +++ b/contrib/pyln-testing/pyln/testing/fixtures.py @@ -55,7 +55,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)) From f5bb709b3e7aa66f1162dad670491b6984512255 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Wed, 19 Feb 2020 17:57:56 +0100 Subject: [PATCH 06/10] pytest: Configure logging in a fixture to match stdout capturing pytest captures the output by monkey patching out `sys.stdout`. This may conflict with our use of `sys.stdout` when configuring logging, resulting in the "Write to closed file" issue that is spamming the logs. By making the logging configuration a fixture hopefully we always use the correct stdout (after pytest has monkey-patched it). --- contrib/pyln-testing/pyln/testing/fixtures.py | 15 ++++++++++++++- contrib/pyln-testing/pyln/testing/utils.py | 5 ----- tests/fixtures.py | 2 +- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/fixtures.py b/contrib/pyln-testing/pyln/testing/fixtures.py index a5e88832298c..3f61bc9301d3 100644 --- a/contrib/pyln-testing/pyln/testing/fixtures.py +++ b/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. diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index 79351ec55d71..828f92027c1f 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/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 diff --git a/tests/fixtures.py b/tests/fixtures.py index 478d50970e2c..beb9d50bdf82 100644 --- a/tests/fixtures.py +++ b/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 From 136ec7a54f29d0725383188eb2fafbebba86043e Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Thu, 27 Feb 2020 17:07:18 +0100 Subject: [PATCH 07/10] pytest: Unbreak the test_feerate_spam test for elementsd Looking for specific feerates, but not adjusting the amounts involved doesn't work. --- tests/test_connection.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/test_connection.py b/tests/test_connection.py index fd819fc74e2d..930a8bc232b9 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -2207,7 +2207,11 @@ def test_change_chaining(node_factory, bitcoind): def test_feerate_spam(node_factory, chainparams): l1, l2 = node_factory.line_graph(2) - slack = 45000000 + # We constrain the value the funder has at its disposal so we get the + # REMOTE feerate we are looking for below. This may be fragile and depends + # on the transactions we generate. + slack = 45000000 if not chainparams['elements'] else 68000000 + # Pay almost everything to l2. l1.pay(l2, 10**9 - slack) From 378037cb97ab78d88254e4ad7cac0405e036cc22 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 3 Mar 2020 17:55:11 +0100 Subject: [PATCH 08/10] pytest: Make test_pay_retry less flaky for liquid-regtest --- tests/test_pay.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/test_pay.py b/tests/test_pay.py index 282247949147..b187b405f9e3 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -8,8 +8,6 @@ DEVELOPER, wait_for, only_one, sync_blockheight, SLOW_MACHINE, TIMEOUT, VALGRIND ) - -import concurrent.futures import copy import os import pytest @@ -1549,14 +1547,17 @@ def test_pay_variants(node_factory): @unittest.skipIf(not DEVELOPER, "gossip without DEVELOPER=1 is slow") @unittest.skipIf(VALGRIND and SLOW_MACHINE, "Travis times out under valgrind") -def test_pay_retry(node_factory, bitcoind): +def test_pay_retry(node_factory, bitcoind, executor, chainparams): """Make sure pay command retries properly. """ + def exhaust_channel(funder, fundee, scid, already_spent=0): - """Spend all available capacity (10^6 - 1%) of channel""" - maxpay = (10**6 - 10**6 // 100 - 16020) * 1000 - already_spent - inv = fundee.rpc.invoice(maxpay, - ''.join(random.choice(string.ascii_letters + string.digits) for _ in range(20)), - "exhaust_channel") + """Spend all available capacity (10^6 - 1%) of channel + """ + peer = funder.rpc.listpeers(fundee.info['id'])['peers'][0] + chan = peer['channels'][0] + maxpay = chan['spendable_msatoshi'] + lbl = ''.join(random.choice(string.ascii_letters) for _ in range(20)) + inv = fundee.rpc.invoice(maxpay, lbl, "exhaust_channel") routestep = { 'msatoshi': maxpay, 'id': fundee.info['id'], @@ -1607,7 +1608,6 @@ def listpays_nofail(b11): # Make sure listpays doesn't transiently show failure while pay # is retrying. - executor = concurrent.futures.ThreadPoolExecutor() fut = executor.submit(listpays_nofail, inv['bolt11']) # Pay l1->l5 should succeed via straight line (eventually) From 1c3037fce4ee351be2095163ce239a7254b54db2 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 17 Mar 2020 18:11:14 +0100 Subject: [PATCH 09/10] json-rpc: Fix test_txprepare if running with postgres Postgres does not guarantee that the insertion order is the returned order, which leads us to skip outputs that have already been stolen onto the selected utxos set, but not added to it because it isn't confirmed. This may also happen with sqlite3 though it's a lot rarer in that case. --- wallet/wallet.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/wallet/wallet.c b/wallet/wallet.c index 0ed551894a73..37edd2eae81d 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -403,8 +403,10 @@ static const struct utxo **wallet_select(const tal_t *ctx, struct wallet *w, * confirmation height and that it is below the required * maxheight (current_height - minconf) */ if (maxheight != 0 && - (!u->blockheight || *u->blockheight > maxheight)) + (!u->blockheight || *u->blockheight > maxheight)) { + tal_free(u); continue; + } tal_arr_expand(&utxos, u); From c85988f2da83518d09a1575ab21a0dab83f95ea8 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 17 Mar 2020 18:16:39 +0100 Subject: [PATCH 10/10] pytest: Fix test_closing_fee regression in elements Constants once again. --- tests/test_closing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_closing.py b/tests/test_closing.py index 4547225a5255..13ab6b1441e5 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -465,7 +465,7 @@ def test_closing_fee(node_factory, bitcoind, chainparams): 'funder_feerate_per_kw': 29006, 'fundee_feerate_per_kw': 27625, 'close_initiated_by': 'funder', - 'expected_close_fee': 20333 + 'expected_close_fee': 33533 if chainparams['elements'] else 20333 } closing_fee(node_factory, bitcoind, chainparams, opts)