From 10a74c7cf6f2881a40984c60bb577d527f5d36a7 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 21 Nov 2019 13:24:01 +1030 Subject: [PATCH] lightningd: don't start if bitcoind is behind. This leads to all sorts of problems; in particular it's incredibly slow (days, weeks!) if bitcoind is a long way back. This also changes the behaviour of a rescan argument referring to a future block: we will also refuse to start in that case, which I think is the correct behavior. We already ignore bitcoind if it goes backwards while we're running. Also cover a false positive memleak. Changelog-Fixed: If bitcoind goes backwards (e.g. reindex) refuse to start (unless forced with --rescan). Signed-off-by: Rusty Russell --- lightningd/bitcoind.c | 5 +++-- lightningd/chaintopology.c | 19 +++++++------------ tests/test_db.py | 4 +++- tests/test_misc.py | 13 ++++++++++--- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index 95e712f48beb..8a061ff40633 100644 --- a/lightningd/bitcoind.c +++ b/lightningd/bitcoind.c @@ -171,8 +171,9 @@ static void bcli_failure(struct bitcoind *bitcoind, bitcoind->error_count++; /* Retry in 1 second (not a leak!) */ - new_reltimer(bitcoind->ld->timers, notleak(bcli), time_from_sec(1), - retry_bcli, bcli); + notleak(new_reltimer(bitcoind->ld->timers, notleak(bcli), + time_from_sec(1), + retry_bcli, bcli)); } static void bcli_finished(struct io_conn *conn UNUSED, struct bitcoin_cli *bcli) diff --git a/lightningd/chaintopology.c b/lightningd/chaintopology.c index 715886802fd9..91c3cd1a1b0b 100644 --- a/lightningd/chaintopology.c +++ b/lightningd/chaintopology.c @@ -804,10 +804,11 @@ static void get_init_block(struct bitcoind *bitcoind, static void get_init_blockhash(struct bitcoind *bitcoind, u32 blockcount, struct chain_topology *topo) { - /* If bitcoind's current blockheight is below the requested height, just - * go back to that height. This might be a new node catching up, or - * bitcoind is processing a reorg. */ + /* If bitcoind's current blockheight is below the requested + * height, refuse. You can always explicitly request a reindex from + * that block number using --rescan=. */ if (blockcount < topo->max_blockheight) { + /* UINT32_MAX == no blocks in database */ if (topo->max_blockheight == UINT32_MAX) { /* Relative rescan, but we didn't know the blockheight */ /* Protect against underflow in subtraction. @@ -816,15 +817,9 @@ static void get_init_blockhash(struct bitcoind *bitcoind, u32 blockcount, topo->max_blockheight = 0; else topo->max_blockheight = blockcount - bitcoind->ld->config.rescan; - } else { - /* Absolute blockheight, but bitcoind's blockheight isn't there yet */ - /* Protect against underflow in subtraction. - * Possible in regtest mode. */ - if (blockcount < 1) - topo->max_blockheight = 0; - else - topo->max_blockheight = blockcount - 1; - } + } else + fatal("bitcoind has gone backwards from %u to %u blocks!", + topo->max_blockheight, blockcount); } /* Rollback to the given blockheight, so we start track diff --git a/tests/test_db.py b/tests/test_db.py index 2462a1da1ff4..85fa6d079e14 100644 --- a/tests/test_db.py +++ b/tests/test_db.py @@ -7,7 +7,9 @@ @unittest.skipIf(TEST_NETWORK != 'regtest', "The DB migration is network specific due to the chain var.") -def test_db_dangling_peer_fix(node_factory): +def test_db_dangling_peer_fix(node_factory, bitcoind): + # Make sure bitcoind doesn't think it's going backwards + bitcoind.generate_block(104) # This was taken from test_fail_unconfirmed() node. l1 = node_factory.get_node(dbfile='dangling-peer.sqlite3.xz') l2 = node_factory.get_node() diff --git a/tests/test_misc.py b/tests/test_misc.py index 3f88ee94db82..e1f01d4794c3 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1135,17 +1135,24 @@ def test_rescan(node_factory, bitcoind): assert l1.daemon.is_in_log(r'Adding block 31') assert not l1.daemon.is_in_log(r'Adding block 30') - # Restarting with a future absolute blockheight should just start with - # the current height + # Restarting with a future absolute blockheight should *fail* if we + # can't find that height l1.daemon.opts['rescan'] = -500000 l1.stop() bitcoind.generate_block(4) + with pytest.raises(ValueError): + l1.start() + + # Restarting with future absolute blockheight is fine if we can find it. + l1.daemon.opts['rescan'] = -105 + oldneedle = l1.daemon.logsearch_start l1.start() + # This could occur before pubkey msg, so move search needle back. + l1.daemon.logsearch_start = oldneedle l1.daemon.wait_for_log(r'Adding block 105') assert not l1.daemon.is_in_log(r'Adding block 102') -@pytest.mark.xfail(strict=True) def test_bitcoind_goes_backwards(node_factory, bitcoind): """Check that we refuse to acknowledge bitcoind giving a shorter chain without explicit rescan""" l1 = node_factory.get_node(may_fail=True, allow_broken_log=True)