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 <rusty@rustcorp.com.au>

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)

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

tests/test_db.py

@@ -7,7 +7,9 @@ import unittest
 
 
 @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()
@@ -126,7 +128,8 @@ def test_max_channel_id(node_factory, bitcoind):
 @unittest.skipIf(not COMPAT, "needs COMPAT to convert obsolete db")
 @unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "This test is based on a sqlite3 snapshot")
 @unittest.skipIf(TEST_NETWORK != 'regtest', "The network must match the DB snapshot")
-def test_scid_upgrade(node_factory):
+def test_scid_upgrade(node_factory, bitcoind):
+    bitcoind.generate_block(1)
 
     # Created through the power of sed "s/X'\([0-9]*\)78\([0-9]*\)78\([0-9]*\)'/X'\13A\23A\3'/"
     l1 = node_factory.get_node(dbfile='oldstyle-scids.sqlite3.xz')

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)