Store onchaind state in the database and reduce rescan on startup #1398

cdecker · 2018-04-20T18:34:54Z

With these changes onchaind no longer relies on rescanning the blockchain from the wallet's birth height on each startup. Instead we remember the transactions that triggered some behavior in onchaind and replay them before syncing with the blockchain.

This is the continuation of #1364 which took care of storing transactions we are interested in and tracking their confirmations when syncing. The onchaind persistence is a small layer on top of those transactions remembering which transaction triggered what notification to onchaind.

In addition the sync offset calculation was simplified, and we can now use the --rescan command line option to specify either a relative or absolute height to continue syncing from. The default is 30 blocks for testnet and 15 for mainnet, to counter potential forks.

This may also partially address pruning nodes, as long as the sync horizon doesn't get pruned out from under our feet.

Fixes #1168
Fixes #1179
Fixes #1283
Fixes #1331
Fixes #1358
Fixes #1246
Fixes #1243

And maybe some more?

ZmnSCPxj · 2018-04-21T05:58:32Z

Minor nits.

Ack ab7280d

We used to queue the preimages to be sent to onchaind only after receiving the onchaind_init_reply. Once we start replaying we might end up in a situation in which we queue the tx that onchaind should react to before providing it with the preimages. This commit just moves the preimages being sent, making it atomic with the init, and without changing the order. Signed-off-by: Christian Decker <[email protected]>

These transactions being seen on the blockchain triggered some action in onchaind so we need to replay them when we restore the onchaind. Signed-off-by: Christian Decker <[email protected]>

This will be used to replay transactions that were witnessed in the blockchain during startup, so that onchaind can be recreate its state. Signed-off-by: Christian Decker <[email protected]>

ZmnSCPxj · 2018-04-21T08:32:55Z

wallet/wallet.c

+			  " type,"
+			  " transaction_id,"
+			  " input_num"
+			  ", blockheight"


Inconsistent comma usage

Fixed. I started using your comma placement, which is much nicer, half way through and forgot to fixup.

Another wrinkle of comma placement, is that one notices that SQL consistently, uses longer command keywords, than the keywords for clauses: SELECT vs WHERE vs ORDER vs FROM; INSERT vs VALUES; UPDATE vs WHERE vs SET, etc.

SELECT col1 , col2 FROM table WHERE col3 = 'whatever' AND col4 = 42 ;

This has the advantage of never requiring spaces before the end of strings being concatenated, since everything after the initial command keyword gets an extra space before it:

"SELECT col1" " , col2" " FROM table" " WHERE col3 = 'whatever'" " AND col4 = 42" " ;"

That's a bit much imho, with the spacing that I added we actually get the same spacing as if we'd written it in the shell (.schema will show excessive spaces in the CREATE statement for example, which is hard to read).

ZmnSCPxj · 2018-04-21T08:33:38Z

wallet/wallet.h

@@ -128,7 +128,7 @@ struct channel_stats {
 	u64 out_msatoshi_offered, out_msatoshi_fulfilled;
 };

-struct onchaindtx {
+struct channeltx {


Maybe this particular change can be put in the previous commit instead?

ZmnSCPxj · 2018-04-21T08:40:57Z

tests/test_lightningd.py

@@ -312,10 +312,10 @@ def fund_channel(self, l1, l2, amount):
        decoded2 = bitcoind.rpc.decoderawtransaction(tx)
        raise ValueError("Can't find {} payment in {} (1={} 2={})".format(amount, tx, decoded, decoded2))

-    def line_graph(self, n=2):
+    def line_graph(self, n=2, options=None):


An improvement certainly, but not used in the same commit that contains it. In principle, better to separate in a different commit. Fine by me either way though.

Removed changes, I initially used line_graph in the onchaind test but then needed to instruct one to fail, so I used the primitives instead.

ZmnSCPxj · 2018-04-21T08:44:21Z

lightningd/options.c

@@ -481,6 +488,9 @@ static const struct config mainnet_config = {

 	/* Mainnet should have more stable fees */
 	.ignore_fee_limits = false,
+
+	/* Do not rescan */
+	.rescan = 15,


This will rescan 15 blocks before the current block height? The comment seems not matching? (should it not be 0 if we just continue scanning at where we left off?)

Also, I remember we had an issue before that our block height would keep getting pushed back if we kept getting restarted, would that behavior occur again?

Good catch, I had 0 in there initially and then I remembered that we don't handle reorgs below our in-memory chain all too well, so I made it 15 blocks. If we process less than 15 blocks before crashing, then yes, this could end up walking backwards, we could add the lower bound at the segwit activation back in, but that's also painful.

An alternative could be to use last_processed_block and not decrement that if it is above our current blockheight. That would make sure that our offset if computed from a fixed point, rather than risking sliding back in time.

Comment is still /* Do not rescan */ even though default is to rescan by 15 blocks back?

ZmnSCPxj · 2018-04-21T08:47:25Z

lightningd/options.c

+	opt_register_arg("--rescan", opt_set_s32, opt_show_s32,
+			 &ld->config.rescan,
+			 "Number of blocks to rescan from the current head, or "
+			 "absolute blockheight if negative");


Seems more sensible, to use negative to mean "current blockheight minus absolute value" and to use positive for "absolute blockheight". And to use 0 to mean "current blockheight, no rescan". Similar to Python arrays/strings, with the "end" being the current known blockheight and a negative index being relative from the end.

But this comment is not a blocking one and I think the current standard is reasonable.

I preferred making the more complex case, i.e., choosing a fixed offset, the case with the added sign. Everybody should just use relative offsets from their current head, in particular if they run from a startup script, and only use the absolute offset (with negation) in a recovery scenario.

I understand.

Yeah, nobody should be using this anyway, so I think -ve for relative is pretty common. Otherwise you want --rescan-absolute or something...

Signed-off-by: Christian Decker <[email protected]>

This will allow us in the next commit to store the transactions that triggered this event in the DB and thus allowing us to replay them later on. Signed-off-by: Christian Decker <[email protected]>

Since we reference the channel ID to allow cascades in the database we also need the ability to look up a channel by its database ID. Signed-off-by: Christian Decker <[email protected]>

Signed-off-by: Christian Decker <[email protected]>

This is a big simplification, we just report the DBs current blockchain height as the point to continue scanning, or the passed in default. No more guessing where to continue from or whether the wallet was used and when it first saw the light of day. Signed-off-by: Christian Decker <[email protected]>

cdecker · 2018-04-21T09:29:31Z

Since this may break a thing or too, I'd like to wait for @rustyrussell to sign off as well, just as a sanity check :-)

robtex · 2018-04-21T12:27:16Z

i want this nooow!
thanks @cdecker, good work !

This is intended to recover from an inconsistent state, involving `onchaind`. Should we for some reason not restore the `onchaind` process correctly we can instruct `lightningd` to go back in time and just replay everything. Signed-off-by: Christian Decker <[email protected]>

Simplification of the offset calculation to use the rescan parameter, and rename of `wallet_first_blocknum`. We now use either relative rescan from our last known location, or absolute if a negative rescan was given. It's all handled in a single location (except the case in which the blockcount is below our precomputed offset), so this should reduce surprises. Signed-off-by: Christian Decker <[email protected]>

Signed-off-by: Christian Decker <[email protected]>

This shaves off about 15% of our integration testing suite on my machine. It assumes we never reorg below the first block the node starts with, which is true for all tests, so it's safe. Signed-off-by: Christian Decker <[email protected]>

rustyrussell

My only concern is over existing deployments monitoring channels which upgrade and don't get replay.

rustyrussell · 2018-04-23T05:46:31Z

wallet/wallet.h

@@ -128,6 +128,16 @@ struct channel_stats {
 	u64 out_msatoshi_offered, out_msatoshi_fulfilled;
 };

+struct onchaindtx {
+	u32 channel_id;
+	int type;


int type always makes me wince. I prefer an enum, if only for readability.

rustyrussell · 2018-04-23T05:48:26Z

wallet/wallet.h

+	struct bitcoin_txid txid;
+	struct bitcoin_tx *tx;
+	u32 input_num;
+	u32 depth;


depth and blockheight seem implicitly related; it seems odd to have this here. Perhaps I'll revise that comment when reviewing a followon patch?

They are different: the blockheight is the height at which the TX was confirmed, the depth is the difference between current blockheight and confirmation blockheight at the time the original notification was delivered (notice that this is not the current blockheight when we are replaying the events).

rustyrussell · 2018-04-23T05:51:39Z

wallet/wallet.c

+			  ", input_num"
+			  ", blockheight"
+			  ") VALUES (?, ?, ?, ?, ?);");
+	sqlite3_bind_int(stmt, 1, chan->dbid);


Is the 'id' database field used at all?

It's an autoincrement field that is used in the SELECT below to make sure we replay the events in the same order they were created, i.e., seen originally. This is just for added consistency should we have some dependency on the order (multiple transactions being confirmed in the same block).

It is declared INTEGER PRIMARY KEY, but note, this implies it is equivalent to ROWID (an internal detail) under SQLITE. In particular, if a row is deleted, it is possible for SQLITE to reuse that row when another row is inserted. We need an explicit AUTOINCREMENT in the declaration of id in CREATE TABLE channeltxs.

https://www.sqlite.org/autoinc.html

If the largest ROWID is equal to the largest possible integer (9223372036854775807) then the database engine starts picking positive candidate ROWIDs at random until it finds one that is not previously used. If no unused ROWID can be found after a reasonable number of attempts, the insert operation fails with an SQLITE_FULL error.

...skip...

The normal ROWID selection algorithm described above will generate monotonically increasing unique ROWIDs as long as you never use the maximum ROWID value and you never delete the entry in the table with the largest ROWID. If you ever delete rows or if you ever create a row with the maximum possible ROWID, then ROWIDs from previously deleted rows might be reused when creating new rows and newly created ROWIDs might not be in strictly ascending order.

Admittedly the condition of reaching 2^63 is extremely unlikely, and if we neglect that, we can generally think of INTEGER PRIMARY KEY as implicitly AUTOINCREMENT, but I think explicitness is a win here.

Indeed, later on it pretty much admits that the only difference between INTEGER PRIMARY KEY and INTEGER PRIMARY KEY AUTOINCREMENT is that the latter immediately fails hard when 2^63 is reached, whereas the former will try to recover at the cost of violating monotonically-increasing numbers.

rustyrussell · 2018-04-23T05:58:22Z

wallet/wallet.c

+		sqlite3_finalize(stmt);
+		return blockheight;
+	} else
+		return first_possible;
 }


If they are currently onchain, and upgrade, this will break them? I think this logic needs to stay for a while :(

rustyrussell · 2018-04-23T06:01:02Z

lightningd/options.c

+	opt_register_arg("--rescan", opt_set_s32, opt_show_s32,
+			 &ld->config.rescan,
+			 "Number of blocks to rescan from the current head, or "
+			 "absolute blockheight if negative");


Yeah, nobody should be using this anyway, so I think -ve for relative is pretty common. Otherwise you want --rescan-absolute or something...

ZmnSCPxj · 2018-04-23T06:07:39Z

ACK ddd53f1

cdecker · 2018-04-23T17:46:45Z

Thanks for the review @rustyrussell. Regarding backward compatibility we could insert a dummy block at a given height as a migration and start the scan from there (when lightning first became interesting seems like a good place to start).

The no-rescan change requires us to rescan one last time from the first_blocknum of our channels (if we have any). The migrations just drop blocks that are higher, then insert a dummy with the first_blocknum, and then clean up after us. If we don't have any channels we don't go back at all. Signed-off-by: Christian Decker <[email protected]>

cdecker · 2018-04-23T22:02:58Z

@rustyrussell I added 3 migrations that'll insert a dummy block with height=first_blocknum (which then gets dropped upon rescanning immediately) if we have a channel, and do nothing otherwise 😉

I tested it with a testnet node with and without channels, and it does what I expected.

Issues addressed

ZmnSCPxj · 2018-04-25T03:47:42Z

ACK fc2b6c0

Sjors · 2018-04-30T10:45:34Z

I have one c-lightning node that's normally connected to a pruned node, though I temporarily connected it to an unpruned node. I updated to the latest master and it's currently catching up again using the full node. Once that's done, I'll switch it back to the pruned node and see what happens.

cdecker · 2018-04-30T11:47:45Z

That'd be great @Sjors, though this is not really a permanent solution for pruned nodes: if the c-lightning node is not running for a long time and it's sync horizon drops below the pruning limit for bitcoind it'll be unable to fetch the pruned nodes. We might want to eventually attach to the P2P network directly and fetch blocks from there.

cdecker added enhancement onchaind db labels Apr 20, 2018

cdecker self-assigned this Apr 20, 2018

cdecker requested review from rustyrussell and ZmnSCPxj April 20, 2018 18:34

cdecker force-pushed the onchaind-restart branch from 2cc84be to ab7280d Compare April 20, 2018 18:38

cdecker added 3 commits April 21, 2018 09:57

db: Add table for onchaind transactions

3b3f62f

These transactions being seen on the blockchain triggered some action in onchaind so we need to replay them when we restore the onchaind. Signed-off-by: Christian Decker <[email protected]>

wallet: Add a struct to represent an onchaind transaction

961e55b

This will be used to replay transactions that were witnessed in the blockchain during startup, so that onchaind can be recreate its state. Signed-off-by: Christian Decker <[email protected]>

cdecker force-pushed the onchaind-restart branch from ab7280d to bf93d17 Compare April 21, 2018 08:00

cdecker mentioned this pull request Apr 21, 2018

channel state not synced #1397

Closed

ZmnSCPxj reviewed Apr 21, 2018

View reviewed changes

cdecker added 3 commits April 21, 2018 11:21

wallet: Add primitives to store onchaind transactions in the DB

2bdbcc4

Signed-off-by: Christian Decker <[email protected]>

onchaind: Add a level of indirection to txwatches and txowatches

57f9129

This will allow us in the next commit to store the transactions that triggered this event in the DB and thus allowing us to replay them later on. Signed-off-by: Christian Decker <[email protected]>

channel: Allow channel lookup by database id

e147967

Since we reference the channel ID to allow cascades in the database we also need the ability to look up a channel by its database ID. Signed-off-by: Christian Decker <[email protected]>

cdecker force-pushed the onchaind-restart branch from bf93d17 to 5377f46 Compare April 21, 2018 09:21

cdecker added 4 commits April 21, 2018 11:27

pytest: test onchaind restarts from the DB

84376f8

onchaind: Store channeltxs so we can restore later

ea97404

Signed-off-by: Christian Decker <[email protected]>

onchaind: Replay stored channeltxs to restore onchaind state

155335e

Signed-off-by: Christian Decker <[email protected]>

cdecker force-pushed the onchaind-restart branch from 5377f46 to e4ed39a Compare April 21, 2018 09:27

cdecker added 3 commits April 22, 2018 12:53

pytest: Add a test for the new --rescan option

8ef2e8a

Signed-off-by: Christian Decker <[email protected]>

pytest: Start nodes with --rescan=1

ddd53f1

This shaves off about 15% of our integration testing suite on my machine. It assumes we never reorg below the first block the node starts with, which is true for all tests, so it's safe. Signed-off-by: Christian Decker <[email protected]>

cdecker force-pushed the onchaind-restart branch from 3679fef to ddd53f1 Compare April 22, 2018 10:53

rustyrussell previously requested changes Apr 23, 2018

View reviewed changes

ZmnSCPxj approved these changes Apr 23, 2018

View reviewed changes

cdecker merged commit 89ff46f into ElementsProject:master Apr 25, 2018

This was referenced Apr 30, 2018

Channel state is CHANNELD_NORMAL but channels are not visible #837

Closed

listchannels empty but CHANNELD_NORMAL with listpeers #874

Closed

Saibato mentioned this pull request May 14, 2018

sync with pruned bitcoind: getblock ... false exited with status 1 #1502

Closed

Store onchaind state in the database and reduce rescan on startup #1398

Store onchaind state in the database and reduce rescan on startup #1398

Uh oh!

Conversation

cdecker commented Apr 20, 2018

Uh oh!

ZmnSCPxj commented Apr 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdecker commented Apr 21, 2018

Uh oh!

robtex commented Apr 21, 2018

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZmnSCPxj Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZmnSCPxj commented Apr 23, 2018

Uh oh!

cdecker commented Apr 23, 2018

Uh oh!

cdecker commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZmnSCPxj commented Apr 25, 2018 • edited by cdecker Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Apr 30, 2018

Uh oh!

cdecker commented Apr 30, 2018

Uh oh!

Uh oh!

ZmnSCPxj Apr 23, 2018 •

edited

Loading

cdecker commented Apr 23, 2018 •

edited

Loading

ZmnSCPxj commented Apr 25, 2018 •

edited by cdecker

Loading