From b9f28aa9cac65680bc8242c08bf4148715345d76 Mon Sep 17 00:00:00 2001 From: Peter Neuroth Date: Fri, 19 Apr 2024 12:40:42 +0200 Subject: [PATCH 1/8] bcli: allow non 0 exit status for getblock call This allows us to intercept getblock on a non 0 exit when the block was not found. We fill this with something meaningull in future commits. Signed-off-by: Peter Neuroth --- plugins/bcli.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index e04159b44377..4fc899ad7cc6 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -562,7 +562,7 @@ struct getrawblock_stash { const char *block_hex; }; -static struct command_result *process_getrawblock(struct bitcoin_cli *bcli) +static struct command_result *process_rawblock(struct bitcoin_cli *bcli) { struct json_stream *response; struct getrawblock_stash *stash = bcli->stash; @@ -577,6 +577,17 @@ static struct command_result *process_getrawblock(struct bitcoin_cli *bcli) return command_finished(bcli->cmd, response); } +static struct command_result *process_getrawblock(struct bitcoin_cli *bcli) +{ + /* We failed to get the raw block. */ + if (bcli->exitstatus && *bcli->exitstatus != 0) { + /* retry */ + return NULL; + } + + return process_rawblock(bcli); +} + static struct command_result * getrawblockbyheight_notfound(struct bitcoin_cli *bcli) { @@ -607,7 +618,7 @@ static struct command_result *process_getblockhash(struct bitcoin_cli *bcli) return command_err_bcli_badjson(bcli, "bad blockhash"); } - start_bitcoin_cli(NULL, bcli->cmd, process_getrawblock, false, + start_bitcoin_cli(NULL, bcli->cmd, process_getrawblock, true, BITCOIND_HIGH_PRIO, stash, "getblock", stash->block_hash, From c5cbcc2515e803f5f04eb4463431edec5bbb8f0c Mon Sep 17 00:00:00 2001 From: Peter Neuroth Date: Fri, 19 Apr 2024 14:40:08 +0200 Subject: [PATCH 2/8] bcli: move getblock call to a separate function We move the `geblock` call into a separate function. This allows us to call if from various places in the future. Signed-off-by: Peter Neuroth --- plugins/bcli.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index 4fc899ad7cc6..219adf4e63ec 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -600,6 +600,19 @@ getrawblockbyheight_notfound(struct bitcoin_cli *bcli) return command_finished(bcli->cmd, response); } +static struct command_result *getrawblock(struct bitcoin_cli *bcli) +{ + struct getrawblock_stash *stash = bcli->stash; + + start_bitcoin_cli(NULL, bcli->cmd, process_getrawblock, true, + BITCOIND_HIGH_PRIO, stash, "getblock", + stash->block_hash, + /* Non-verbose: raw block. */ + "0", NULL); + + return command_still_pending(bcli->cmd); +} + static struct command_result *process_getblockhash(struct bitcoin_cli *bcli) { struct getrawblock_stash *stash = bcli->stash; @@ -618,15 +631,7 @@ static struct command_result *process_getblockhash(struct bitcoin_cli *bcli) return command_err_bcli_badjson(bcli, "bad blockhash"); } - start_bitcoin_cli(NULL, bcli->cmd, process_getrawblock, true, - BITCOIND_HIGH_PRIO, stash, - "getblock", - stash->block_hash, - /* Non-verbose: raw block. */ - "0", - NULL); - - return command_still_pending(bcli->cmd); + return getrawblock(bcli); } /* Get a raw block given its height. From d3785c9aecb94a235ff6f6f53e605db36487a339 Mon Sep 17 00:00:00 2001 From: Peter Neuroth Date: Fri, 19 Apr 2024 14:51:32 +0200 Subject: [PATCH 3/8] bcli: add peers array to the stash We introduce the peers array that contains the known set of connected peers for future reference in case that we couldn't find the block we are looking for. Signed-off-by: Peter Neuroth --- plugins/bcli.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/plugins/bcli.c b/plugins/bcli.c index 219adf4e63ec..a795accd15b9 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -560,8 +560,12 @@ struct getrawblock_stash { const char *block_hash; u32 block_height; const char *block_hex; + int *peers; }; +/* Mutual recursion. */ +static struct command_result *getrawblock(struct bitcoin_cli *bcli); + static struct command_result *process_rawblock(struct bitcoin_cli *bcli) { struct json_stream *response; @@ -577,10 +581,59 @@ static struct command_result *process_rawblock(struct bitcoin_cli *bcli) return command_finished(bcli->cmd, response); } +static struct command_result *process_getpeerinfo(struct bitcoin_cli *bcli) +{ + const jsmntok_t *t, *toks; + struct getrawblock_stash *stash = bcli->stash; + size_t i; + + toks = + json_parse_simple(bcli->output, bcli->output, bcli->output_bytes); + + if (!toks) { + return command_err_bcli_badjson(bcli, "cannot parse"); + } + + stash->peers = tal_arr(bcli->stash, int, 0); + + json_for_each_arr(i, t, toks) + { + int id; + if (json_scan(tmpctx, bcli->output, t, "{id:%}", + JSON_SCAN(json_to_int, &id)) == NULL) { + tal_arr_expand(&stash->peers, id); + } + } + + if (tal_count(stash->peers) <= 0) { + /* We don't have peers yet, retry from `getrawblock` */ + plugin_log(bcli->cmd->plugin, LOG_DBG, + "got an empty peer list."); + return getrawblock(bcli); + } + + return getrawblock(bcli); +} + static struct command_result *process_getrawblock(struct bitcoin_cli *bcli) { /* We failed to get the raw block. */ if (bcli->exitstatus && *bcli->exitstatus != 0) { + struct getrawblock_stash *stash = bcli->stash; + + plugin_log(bcli->cmd->plugin, LOG_DBG, + "failed to fetch block %s from the bitcoin backend (maybe pruned).", + stash->block_hash); + + if (!stash->peers) { + /* We don't have peers to fetch blocks from, get some! */ + start_bitcoin_cli(NULL, bcli->cmd, process_getpeerinfo, true, + BITCOIND_HIGH_PRIO, stash, + "getpeerinfo", NULL); + + return command_still_pending(bcli->cmd); + } + /* retry */ return NULL; } @@ -653,6 +706,7 @@ static struct command_result *getrawblockbyheight(struct command *cmd, stash = tal(cmd, struct getrawblock_stash); stash->block_height = *height; + stash->peers = NULL; tal_free(height); start_bitcoin_cli(NULL, cmd, process_getblockhash, true, From 744fbf0dc9f32a592cf539614dae766405b35a77 Mon Sep 17 00:00:00 2001 From: Peter Neuroth Date: Fri, 19 Apr 2024 14:57:22 +0200 Subject: [PATCH 4/8] bcli: try to fetch block from peer if unknown In the case that we have peers left in the peers array and that we could not find the block yet, we ask the next peer for the desired block. Signed-off-by: Peter Neuroth --- plugins/bcli.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index a795accd15b9..10771f91903f 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -581,6 +581,35 @@ static struct command_result *process_rawblock(struct bitcoin_cli *bcli) return command_finished(bcli->cmd, response); } +static struct command_result *process_getblockfrompeer(struct bitcoin_cli *bcli) +{ + /* Remove the peer that we tried to get the block from and move along, + * we may also check on errors here */ + struct getrawblock_stash *stash = bcli->stash; + + if (bcli->exitstatus && *bcli->exitstatus != 0) { + /* We still continue with the execution if we can not fetch the + * block from peer */ + plugin_log(bcli->cmd->plugin, LOG_DBG, + "failed to fetch block %s from peer %i, skip.", + stash->block_hash, stash->peers[0]); + } else { + plugin_log(bcli->cmd->plugin, LOG_DBG, + "try to fetch block %s from peer %i.", + stash->block_hash, stash->peers[0]); + } + + stash->peers[0] = stash->peers[tal_count(stash->peers) - 1]; + tal_resize(&stash->peers, tal_count(stash->peers) - 1); + + /* `getblockfrompeer` is an async call. sleep for a second to allow the + * block to be delivered by the peer. fixme: We could also sleep for + * double the last ping here (with sanity limit)*/ + sleep(1); + + return getrawblock(bcli); +} + static struct command_result *process_getpeerinfo(struct bitcoin_cli *bcli) { const jsmntok_t *t, *toks; @@ -601,6 +630,8 @@ static struct command_result *process_getpeerinfo(struct bitcoin_cli *bcli) int id; if (json_scan(tmpctx, bcli->output, t, "{id:%}", JSON_SCAN(json_to_int, &id)) == NULL) { + // fixme: future optimization: a) filter for full nodes, + // b) sort by last ping tal_arr_expand(&stash->peers, id); } } @@ -612,7 +643,12 @@ static struct command_result *process_getpeerinfo(struct bitcoin_cli *bcli) return getrawblock(bcli); } - return getrawblock(bcli); + start_bitcoin_cli(NULL, bcli->cmd, process_getblockfrompeer, true, + BITCOIND_HIGH_PRIO, stash, "getblockfrompeer", + stash->block_hash, + take(tal_fmt(NULL, "%i", stash->peers[0])), NULL); + + return command_still_pending(bcli->cmd); } static struct command_result *process_getrawblock(struct bitcoin_cli *bcli) @@ -634,7 +670,22 @@ static struct command_result *process_getrawblock(struct bitcoin_cli *bcli) return command_still_pending(bcli->cmd); } - /* retry */ + if (tal_count(stash->peers) > 0) { + /* We have peers left that we can ask for the block */ + start_bitcoin_cli( + NULL, bcli->cmd, process_getblockfrompeer, true, + BITCOIND_HIGH_PRIO, stash, "getblockfrompeer", + stash->block_hash, + take(tal_fmt(NULL, "%i", stash->peers[0])), NULL); + + return command_still_pending(bcli->cmd); + } + + /* We failed to fetch the block from from any peer we got. */ + plugin_log(bcli->cmd->plugin, LOG_DBG, + "asked all known peers about block %s, retry", + stash->block_hash); + stash->peers = tal_free(stash->peers); return NULL; } From 17a8be50f6c756af6ca52ca6cc7c5e8097f1bd27 Mon Sep 17 00:00:00 2001 From: Peter Neuroth Date: Fri, 19 Apr 2024 15:48:27 +0200 Subject: [PATCH 5/8] bcli: add version check `getblockfrompeer` was introduced in v23.0.0, we want to skip this path if the version of bitcoind used is below that. Signed-off-by: Peter Neuroth --- plugins/bcli.c | 57 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 23 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index 10771f91903f..41f08feecd8f 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -660,32 +660,43 @@ static struct command_result *process_getrawblock(struct bitcoin_cli *bcli) plugin_log(bcli->cmd->plugin, LOG_DBG, "failed to fetch block %s from the bitcoin backend (maybe pruned).", stash->block_hash); - - if (!stash->peers) { - /* We don't have peers to fetch blocks from, get some! */ - start_bitcoin_cli(NULL, bcli->cmd, process_getpeerinfo, true, - BITCOIND_HIGH_PRIO, stash, - "getpeerinfo", NULL); - - return command_still_pending(bcli->cmd); - } - - if (tal_count(stash->peers) > 0) { - /* We have peers left that we can ask for the block */ - start_bitcoin_cli( - NULL, bcli->cmd, process_getblockfrompeer, true, - BITCOIND_HIGH_PRIO, stash, "getblockfrompeer", - stash->block_hash, - take(tal_fmt(NULL, "%i", stash->peers[0])), NULL); - return command_still_pending(bcli->cmd); + if (bitcoind->version >= 230000) { + /* `getblockformpeer` was introduced in v23.0.0 */ + + if (!stash->peers) { + /* We don't have peers to fetch blocks from, get + * some! */ + start_bitcoin_cli(NULL, bcli->cmd, + process_getpeerinfo, true, + BITCOIND_HIGH_PRIO, stash, + "getpeerinfo", NULL); + + return command_still_pending(bcli->cmd); + } + + if (tal_count(stash->peers) > 0) { + /* We have peers left that we can ask for the + * block */ + start_bitcoin_cli( + NULL, bcli->cmd, process_getblockfrompeer, + true, BITCOIND_HIGH_PRIO, stash, + "getblockfrompeer", stash->block_hash, + take(tal_fmt(NULL, "%i", stash->peers[0])), + NULL); + + return command_still_pending(bcli->cmd); + } + + /* We failed to fetch the block from from any peer we + * got. */ + plugin_log( + bcli->cmd->plugin, LOG_DBG, + "asked all known peers about block %s, retry", + stash->block_hash); + stash->peers = tal_free(stash->peers); } - /* We failed to fetch the block from from any peer we got. */ - plugin_log(bcli->cmd->plugin, LOG_DBG, - "asked all known peers about block %s, retry", - stash->block_hash); - stash->peers = tal_free(stash->peers); return NULL; } From 64c3b1bdaf38924bbd746db538d12e70f6a61c4d Mon Sep 17 00:00:00 2001 From: Peter Neuroth Date: Fri, 19 Apr 2024 15:25:27 +0200 Subject: [PATCH 6/8] tests: add tests for getblockfrompeer Changelog-Changed: Plugins: bcli: Add a path that tries to fetch blocks from a peer if we can not find them locally. Signed-off-by: Peter Neuroth --- tests/test_misc.py | 95 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/tests/test_misc.py b/tests/test_misc.py index 8979b0532555..eb748046ca51 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -138,6 +138,101 @@ def test_bitcoin_ibd(node_factory, bitcoind): assert 'warning_bitcoind_sync' not in l1.rpc.getinfo() +def test_bitcoin_pruned(node_factory, bitcoind): + """Test that we try to fetch blocks from a peer if we can not find + them on our local bitcoind. + """ + fetched_peerblock = False + + def mock_getblock(r): + # Simulate a pruned node that reutrns an error when asked for a block. + nonlocal fetched_peerblock + if fetched_peerblock: + fetched_peerblock = False + conf_file = os.path.join(bitcoind.bitcoin_dir, "bitcoin.conf") + brpc = RawProxy(btc_conf_file=conf_file) + return { + "result": brpc._call(r["method"], *r["params"]), + "error": None, + "id": r["id"], + } + return { + "id": r["id"], + "result": None, + "error": {"code": -1, "message": "Block not available (pruned data)"}, + } + + def mock_getpeerinfo(r, error=False): + if error: + return {"id": r["id"], "error": {"code": -1, "message": "unknown"}} + return { + "id": r["id"], + "result": [ + { + "id": 1, + }, + { + "id": 2, + }, + { + "id": 3, + }, + ], + } + + def mock_getblockfrompeer(error=False, release_after=0): + getblock_counter = 0 + + def mock_getblockfrompeer_inner(r): + nonlocal getblock_counter + getblock_counter += 1 + + if error and getblock_counter < release_after: + return { + "id": r["id"], + "error": {"code": -1, "message": "peer unknown"}, + } + if getblock_counter >= release_after: + nonlocal fetched_peerblock + fetched_peerblock = True + return { + "id": r["id"], + "result": {}, + } + return mock_getblockfrompeer_inner + + l1 = node_factory.get_node(start=False) + + l1.daemon.rpcproxy.mock_rpc("getblock", mock_getblock) + l1.daemon.rpcproxy.mock_rpc("getpeerinfo", mock_getpeerinfo) + l1.daemon.rpcproxy.mock_rpc("getblockfrompeer", mock_getblockfrompeer()) + l1.start(wait_for_bitcoind_sync=False) + + # check that we fetched a block from a peer (1st peer in this case). + pruned_block = bitcoind.rpc.getblockhash(bitcoind.rpc.getblockcount()) + l1.daemon.wait_for_log(f"failed to fetch block {pruned_block} from the bitcoin backend") + l1.daemon.wait_for_log(rf"try to fetch block {pruned_block} from peer 1") + l1.daemon.wait_for_log(rf"Adding block (\d+): {pruned_block}") + + # check that we can also fetch from a peer > 1st. + l1.daemon.rpcproxy.mock_rpc("getblockfrompeer", mock_getblockfrompeer(error=True, release_after=2)) + bitcoind.generate_block(1) + + pruned_block = bitcoind.rpc.getblockhash(bitcoind.rpc.getblockcount()) + l1.daemon.wait_for_log(f"failed to fetch block {pruned_block} from the bitcoin backend") + l1.daemon.wait_for_log(rf"failed to fetch block {pruned_block} from peer 1") + l1.daemon.wait_for_log(rf"try to fetch block {pruned_block} from peer (\d+)") + l1.daemon.wait_for_log(rf"Adding block (\d+): {pruned_block}") + + # check that we retry if we could not fetch any block + l1.daemon.rpcproxy.mock_rpc("getblockfrompeer", mock_getblockfrompeer(error=True, release_after=10)) + bitcoind.generate_block(1) + + pruned_block = bitcoind.rpc.getblockhash(bitcoind.rpc.getblockcount()) + l1.daemon.wait_for_log(f"asked all known peers about block {pruned_block}, retry") + l1.daemon.wait_for_log(rf"Adding block (\d+): {pruned_block}") + + @pytest.mark.openchannel('v1') @pytest.mark.openchannel('v2') def test_lightningd_still_loading(node_factory, bitcoind, executor): From 5ab1ff55bb8d21b17c6720910baa7bc6e085aad4 Mon Sep 17 00:00:00 2001 From: Peter Neuroth Date: Mon, 22 Apr 2024 17:49:07 +0200 Subject: [PATCH 7/8] ci: Update elementsd version used for tests Using `getblockfrompeer` requires a version equal or above 23.0.0 Signed-off-by: Peter Neuroth --- .github/workflows/ci.yaml | 6 +++--- plugins/bcli.c | 10 ---------- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 60baf418c970..1493e0ef21d6 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -212,7 +212,7 @@ jobs: timeout-minutes: 120 env: BITCOIN_VERSION: "26.1" - ELEMENTS_VERSION: 22.0.2 + ELEMENTS_VERSION: 23.2.1 RUST_PROFILE: release # Has to match the one in the compile step PYTEST_OPTS: --timeout=1200 --force-flaky needs: @@ -320,7 +320,7 @@ jobs: timeout-minutes: 120 env: BITCOIN_VERSION: "26.1" - ELEMENTS_VERSION: 22.0.2 + ELEMENTS_VERSION: 23.2.1 RUST_PROFILE: release # Has to match the one in the compile step CFG: compile-gcc PYTEST_OPTS: --test-group-random-seed=42 --timeout=1800 --force-flaky @@ -390,7 +390,7 @@ jobs: timeout-minutes: 120 env: BITCOIN_VERSION: "26.1" - ELEMENTS_VERSION: 22.0.2 + ELEMENTS_VERSION: 23.2.1 RUST_PROFILE: release SLOW_MACHINE: 1 TEST_DEBUG: 1 diff --git a/plugins/bcli.c b/plugins/bcli.c index 41f08feecd8f..be2c4c3f5b06 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -1004,17 +1004,7 @@ static struct command_result *sendrawtransaction(struct command *cmd, return command_param_failed(); if (*allowhighfees) { - if (bitcoind->version >= 190001) - /* Starting in 19.0.1, second argument is - * maxfeerate, which when set to 0 means - * no max feerate. - */ highfeesarg = "0"; - else - /* in older versions, second arg is allowhighfees, - * set to true to allow high fees. - */ - highfeesarg = "true"; } else highfeesarg = NULL; From 6d60c865312a209a5f3dfd0b95ca3b6d36eb2f16 Mon Sep 17 00:00:00 2001 From: Peter Neuroth Date: Tue, 23 Apr 2024 10:18:09 +0200 Subject: [PATCH 8/8] bcli: change iteration order on peerlist It is simpler to just iterate through the peerlist backwards. Signed-off-by: Peter Neuroth --- plugins/bcli.c | 6 ++---- tests/test_misc.py | 8 ++++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index be2c4c3f5b06..4634beb11262 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -592,14 +592,12 @@ static struct command_result *process_getblockfrompeer(struct bitcoin_cli *bcli) * block from peer */ plugin_log(bcli->cmd->plugin, LOG_DBG, "failed to fetch block %s from peer %i, skip.", - stash->block_hash, stash->peers[0]); + stash->block_hash, stash->peers[tal_count(stash->peers) - 1]); } else { plugin_log(bcli->cmd->plugin, LOG_DBG, "try to fetch block %s from peer %i.", - stash->block_hash, stash->peers[0]); + stash->block_hash, stash->peers[tal_count(stash->peers) - 1]); } - - stash->peers[0] = stash->peers[tal_count(stash->peers) - 1]; tal_resize(&stash->peers, tal_count(stash->peers) - 1); /* `getblockfrompeer` is an async call. sleep for a second to allow the diff --git a/tests/test_misc.py b/tests/test_misc.py index eb748046ca51..f0bc7f52b6e3 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -208,19 +208,19 @@ def mock_getblockfrompeer_inner(r): l1.daemon.rpcproxy.mock_rpc("getblockfrompeer", mock_getblockfrompeer()) l1.start(wait_for_bitcoind_sync=False) - # check that we fetched a block from a peer (1st peer in this case). + # check that we fetched a block from a peer (1st peer (from the back) in this case). pruned_block = bitcoind.rpc.getblockhash(bitcoind.rpc.getblockcount()) l1.daemon.wait_for_log(f"failed to fetch block {pruned_block} from the bitcoin backend") - l1.daemon.wait_for_log(rf"try to fetch block {pruned_block} from peer 1") + l1.daemon.wait_for_log(rf"try to fetch block {pruned_block} from peer 3") l1.daemon.wait_for_log(rf"Adding block (\d+): {pruned_block}") - # check that we can also fetch from a peer > 1st. + # check that we can also fetch from a peer > 1st (from the back). l1.daemon.rpcproxy.mock_rpc("getblockfrompeer", mock_getblockfrompeer(error=True, release_after=2)) bitcoind.generate_block(1) pruned_block = bitcoind.rpc.getblockhash(bitcoind.rpc.getblockcount()) l1.daemon.wait_for_log(f"failed to fetch block {pruned_block} from the bitcoin backend") - l1.daemon.wait_for_log(rf"failed to fetch block {pruned_block} from peer 1") + l1.daemon.wait_for_log(rf"failed to fetch block {pruned_block} from peer 3") l1.daemon.wait_for_log(rf"try to fetch block {pruned_block} from peer (\d+)") l1.daemon.wait_for_log(rf"Adding block (\d+): {pruned_block}")