Track output confirmations, spends and maintain an internal UTXO set #1117

cdecker · 2018-02-27T11:57:29Z

This PR aims to provide better tracking of the blockchain and internalizing some of the information which is now being fetched on demand. A number of use-cases should be improved with it:

Owned funds now receive a confirmation height and a spend height in order to reduce the chances of running out of sync with the bitcoin UTXO
Adds an outpointfilter that can be used to filter outpoint spends that we are interested in, so that we can be notified when our owned outputs are being spent, or when a P2WSH UTXO has been spent
Tracks P2WSH UTXOs in the wallet to avoid calling out to bitcoin-cli for each channel we learn about. Should allow us to reduce the sync time and is needed if we want to persist gossip announcement across restarts.
Allows us not to rescan the entire blockchain from the first channel, instead starting to scan from the last known height - overlap.

I'm currently planning to have the sync start at last_sync_height - 432 which should cover the cases in which we have an active close and some HTLCs waiting to be redeemed as well, i.e., starting onchaind when seeing the funding spend, without additional state to be persisted.

ZmnSCPxj · 2018-02-27T12:16:42Z

I believe this fixes #451?

cdecker · 2018-02-27T12:21:34Z

Indeed it does, I'll collect the issues once I'm back online 😉

cdecker · 2018-02-27T12:23:11Z

Oh and it may be way too soon to review, which is why I didn't assign reviewers yet.

ZmnSCPxj · 2018-02-27T12:17:58Z

wallet/db.c

+    /* What do we think the head of the blockchain looks like? Used
+     * primarily to track confirmations across restarts and making
+     * sure we handle reorgs correctly. */
+    "CREATE TABLE blocks (height INT, hash BLOB, prev_hash BLOB, UNIQUE(height));",


The UNIQUE(height) constraint implies that a reorg deletes entries in this table, right?

It does, yes, and it's also what I'm using to set spend_height and confirmation_height to NULL in case of a reorg.

The idea is to only track what is actually the valid chain, and have a rollback to the fork point in case of a reorg.

ZmnSCPxj · 2018-02-27T12:23:39Z

wallet/wallet.c

+void wallet_block_remove(struct wallet *w, struct block *b)
+{
+	sqlite3_stmt *stmt = db_prepare(w->db, "DELETE FROM blocks "
+					"WHERE hash = ? OR height >= ?");


The OR in the condition means the equality check on hash does not buy you any additional assurance of correctness. It will only be relevant if the hash and height of the given struct block * does not match the entry in the DB, but if your struct block * does not match the corresponding entry in the DB you have bigger problems (a cache incoherence). If the hash and height on the struct block * maches the DB row, then height >= ? is sufficient. But that then means that wallet_block_rollback is sufficient and wallet_block_remove is unnecessary.

Right, I should remove the height check, I was hoping to replicate the remove_tip logic, hoping that it'd remove in reverse height order.

ZmnSCPxj · 2018-02-27T12:35:22Z

wallet/wallet.c

@@ -16,6 +16,17 @@
 #define DIRECTION_INCOMING 0
 #define DIRECTION_OUTGOING 1

+static void outpointfilters_init(struct wallet *w)
+{
+	struct utxo **utxos = wallet_get_utxos(NULL, w, output_state_any);


We will monitor all outputs, even those already spent?

I'm planning to have a cleanup that triggers periodically and garbage collects spent outputs every once in a while if they are deeply buried, i.e., cannot be reorged out. That'll drop from the DB and from the filter set.

okay, seems good.

Yep, we treat 100 as "forever" elsewhere, so that seems a good benchmark.

ZmnSCPxj · 2018-02-27T12:38:23Z

Overall OK so far, even if WIP; github messes up their ordering so I just got it directly to my local, pleasant to review. A few questions though in my review.

cdecker · 2018-02-27T12:43:40Z

Thanks for the quick review @ZmnSCPxj, I'll address the blockheight issue now and build on top 😉

cdecker · 2018-03-04T04:22:23Z

I think we should be good for a first iteration of reviews. It doesn't yet contain all the features I eventually want to build on top of the utxoset being tracked in the wallet but it is a self-contained set of changes and it's starting to get hard to rebase on top of master every day :-)

I decided to keep the owned outputs and the utxoset separate, though the terms are sometimes a bit intermingled.

We track all P2WSH utxos because they might be channels and might be needed for later lookups, and prune the spent ones that are older than a day. Each output being tracked adds about 200 bytes to the DB size, so the impact isn't too big. Performance wise the overhead is tiny compared to the bitcoin_txid calls in filter_block_txs.

Please review again

ZmnSCPxj

Thank you, each commit seems reasonably clear, have some questions though.

ZmnSCPxj · 2018-03-04T05:45:42Z

wallet/wallet.h

+void wallet_block_add(struct wallet *w, struct block *b);
+
+/**
+ * wallet_block_remove - Remove a block (and all its descendants) from the tracked blockchain


Comment is no longer accurate --- now it only removes a block without affecting its descendants if any.

Looking over the rest of the code it seems wallet_block_remove is only called on the current tip? If so, suggest adding an assertion somewhere in the implementation of wallet_block_remove.

/* after DELETE */ stmt = db_prepare(w->db, "SELECT * FROM blocks WHERE height >= ?;"); sqlite3_bind_int(stmt, 1, b->height); assert(sqlite3_step(stmt) == SQLITE_DONE); sqlite3_finalize(stmt);

ZmnSCPxj · 2018-03-04T05:46:09Z

wallet/wallet.c

+	sqlite3_stmt *stmt = db_prepare(w->db,
+					"DELETE FROM blocks WHERE hash = ?");
+	sqlite3_bind_sha256_double(stmt, 1, &b->blkid.shad);
+	sqlite3_bind_int(stmt, 2, b->height);


Unnecessary bind.

ZmnSCPxj · 2018-03-04T05:51:48Z

tests/test_lightningd.py

@@ -1131,11 +1131,11 @@ def test_bitcoin_failure(self):

        # This should cause both estimatefee and getblockhash fail
        l1.daemon.wait_for_logs(['estimatesmartfee .* exited with status 1',
-                                 'getblockhash .* exited with status 1'])
+                                 'getblock.* exited with status 1'])


I believe #1128 makes this superfluous?

Removed 😉

ZmnSCPxj · 2018-03-04T05:55:23Z

wallet/wallet.c

+	stmt = db_prepare(w->db, "SELECT txid, outnum FROM utxoset WHERE spendheight = ?");
+	sqlite3_bind_null(stmt, 1);
+
+	for (size_t i=0; sqlite3_step(stmt) == SQLITE_ROW; i++) {


i seems unused? Why not simply while (sqlite3_step(stmt) == SQLITE_ROW)?

Greta catch, this was a copy-paste leftover

cdecker · 2018-03-04T15:29:21Z

Thanks @ZmnSCPxj for the in-depth review, I think I addressed the issues and it should be ready for another round ^^

rustyrussell

Minor nits only. Note that commit ab79f7f is empty :)

Changing the startup not to go all the way back will need the ability to replay old notifications for onchaind, which we need anyway for onchaind handling reorgs. I look forward to that

rustyrussell · 2018-03-05T23:00:40Z

common/utxo.h

@@ -26,6 +26,9 @@ struct utxo {
 	/* Optional unilateral close information, NULL if this is just
 	 * a HD key */
 	struct unilateral_close_info *close_info;
+
+	/* Blockchain height references */
+	const int *confirmation_height, *spend_height;


I'd like a bit more description for these fields; and we use u32 for block heights (the other option is to make these non-pointers and use -1, but I like the crash we get if they're NULL and we make a mistake):

/* NULL if we haven't seen it in a block, otherwise the block it's in */
const u32 *confirmation_height;

/* NULL if not spent yet, otherwise, the block the spending transaction is in */
const u32 *spend_height;

Also, 'confirmation_height' here and in db could also be called 'block_height'? The word confirmation is usually used with depth, not height, so it seems a bit awkward here? In other places you use blockheight and spendheight, it seems.

confirmed_at_height and spent_at_height?

Now using blockheight and spendheight, consistent with the utxoset nomenclature. The block->height is of type int, I'll change that in a cleanup PR.

rustyrussell · 2018-03-05T23:03:40Z

wallet/db.c

@@ -215,6 +215,8 @@ char *dbmigrations[] = {
     * die. */
    "ALTER TABLE outputs ADD COLUMN confirmation_height INTEGER REFERENCES blocks(height) ON DELETE SET NULL;",
    "ALTER TABLE outputs ADD COLUMN spend_height INTEGER REFERENCES blocks(height) ON DELETE SET NULL;",
+    /* Create a covering index that covers both fiels */


rustyrussell · 2018-03-05T23:05:11Z

lightningd/txfilter.c

+static bool outpoint_eq(const struct outpointfilter_entry *o1,
+			const struct outpointfilter_entry *o2)
+{
+	return memeq(&o1->txid, sizeof(o1->txid), &o2->txid,


structeq() is preferred over memeq because it make sure they really are the same type. And it's safe on bitcoin_txid.

rustyrussell · 2018-03-05T23:08:11Z

lightningd/txfilter.c

+		return;
+	/* Have to mark the entries as notleak since they'll not be
+	 * pointed to by anything other than the htable */
+	op = notleak(tal(of->set, struct outpointfilter_entry));


OK for now, but better is to add outpointfilter_mark_pointers_used; see chaintopology_mark_pointers_used().

rustyrussell · 2018-03-05T23:10:06Z

wallet/wallet.c

@@ -16,6 +16,17 @@
 #define DIRECTION_INCOMING 0
 #define DIRECTION_OUTGOING 1

+static void outpointfilters_init(struct wallet *w)
+{
+	struct utxo **utxos = wallet_get_utxos(NULL, w, output_state_any);


Yep, we treat 100 as "forever" elsewhere, so that seems a good benchmark.

rustyrussell · 2018-03-05T23:20:04Z

wallet/db.c

+    " spendheight INT REFERENCES blocks(height) ON DELETE SET NULL,"
+    " txindex INT,"
+    " scriptpubkey BLOB,"
+    " satoshis INT,"


satoshis should be BIGINT. Doesn't matter for sqlite, but could for other dbs. Wonder if we get this wrong elsewhere too?

Right, presumably we can just replace these without migrations since they map to the same internal datatype in sqlite3. Will change this instance and then go through the others in a new PR.

rustyrussell · 2018-03-05T23:23:18Z

wallet/wallet.c

+
+	w->utxoset_outpoints = outpointfilter_new(w);
+	stmt = db_prepare(w->db, "SELECT txid, outnum FROM utxoset WHERE spendheight = ?");
+	sqlite3_bind_null(stmt, 1);


Binding here seems wierd: why not just say "WHERE spendheight IS NULL" directly?

rustyrussell · 2018-03-05T23:24:44Z

wallet/wallet.h

@@ -34,6 +34,7 @@ struct wallet {
 	u64 max_channel_dbid;

 	struct outpointfilter *owned_outpoints;
+	struct outpointfilter *utxoset_outpoints;


The relationship between these two is not immediately obvious to me?

owned_outputs is all outputs we own, including spent ones? utxoset_outpoints is a complete UTXO set? Or UTXOs corresponding to channel opens?

Added a comment to each about what they include.

rustyrussell · 2018-03-05T23:29:03Z

lightningd/chaintopology.c

+		}
+	}
+}
+


Is this a wise choice? That may be putting a lot of data into the db...

It only stores P2WSH outputs, which may actually be funding txs. The cost is about 200 bytes / output so without pruning spent ones all P2WSH outputs since segwit was activated take about 30MB in size, much less with pruning. It's the usual space-vs-speed tradeoff and this way we can run on pruned nodes and not parse blocks over and over again.

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

Avoids performing a table scan, now deletes on blocks are a lot faster. Signed-off-by: Christian Decker <[email protected]>

This can be used both for our own outputs as well as the utxos we are tracking. Signed-off-by: Christian Decker <[email protected]>

Will be used later to filter out outputs we are interested in, and trigger db updates with them. Signed-off-by: Christian Decker <[email protected]>

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

Transaction filters are strongly related to the wallet, this move just makes it a bit more explicit. Signed-off-by: Christian Decker <[email protected]>

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

When we already know about an output we would stop scanning the remaining outputs. Known outputs happen whenever we extracted from our own transactions and then extracted again from blocks. We would not update if the first update fails. Signed-off-by: Christian Decker <[email protected]>

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

Otherwise we would be doing a table scan per block being reorged/rescanned. Signed-off-by: Christian Decker <[email protected]>

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

cdecker · 2018-03-06T15:15:30Z

Minor changes to address the feedback from @rustyrussell, merging as soon as CI is happy 👍

cdecker added enhancement Work in Progress gossip bitcoind labels Feb 27, 2018

ZmnSCPxj requested review from ZmnSCPxj and rustyrussell February 27, 2018 12:16

ZmnSCPxj previously requested changes Feb 27, 2018

View reviewed changes

ZmnSCPxj removed the request for review from rustyrussell February 27, 2018 12:36

cdecker force-pushed the trackblocks branch from 15c1541 to 52e0248 Compare March 3, 2018 17:16

cdecker mentioned this pull request Mar 4, 2018

Re-broadcast funding tx #1083

Closed

cdecker force-pushed the trackblocks branch 2 times, most recently from 15f9e83 to 2f4e0d1 Compare March 4, 2018 03:44

cdecker removed the Work in Progress label Mar 4, 2018

cdecker force-pushed the trackblocks branch from 2f4e0d1 to cc1f32c Compare March 4, 2018 03:49

cdecker changed the title ~~WIP: Track output confirmations, spends and maintain an internal UTXO set~~ Track output confirmations, spends and maintain an internal UTXO set Mar 4, 2018

ZmnSCPxj suggested changes Mar 4, 2018

View reviewed changes

cdecker force-pushed the trackblocks branch from cc1f32c to 1cc54c9 Compare March 4, 2018 15:28

cdecker mentioned this pull request Mar 4, 2018

Feature request: sync faster using stored state from last time. #1168

Closed

rustyrussell approved these changes Mar 5, 2018

View reviewed changes

ZmnSCPxj approved these changes Mar 6, 2018

View reviewed changes

cdecker added 8 commits March 6, 2018 15:30

db: Add a table to track blockheaders

57f60d8

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

wallet: Add primitives to store blockchain internally

320d4ab

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

wallet: Add primitive to roll back to a specific height

9a02521

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

topology: Track blockchain changes in the DB

5b3644a

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

db: Add confirmation_height and spend_height columns to outputs

fa8c785

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

utxo: Add blockheight and spendheight to outputs to track state

b712021

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

jsonrpc: Add confirmation status to listfunds output

33f900b

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

pytest: Add blockchain tracking test

7dbf484

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

cdecker force-pushed the trackblocks branch from 1cc54c9 to 7a360b8 Compare March 6, 2018 14:55

cdecker added 14 commits March 6, 2018 16:09

db: Add index covering the output heights

4188c05

Avoids performing a table scan, now deletes on blocks are a lot faster. Signed-off-by: Christian Decker <[email protected]>

txfilter: Add an outpoint filter

6ffca70

This can be used both for our own outputs as well as the utxos we are tracking. Signed-off-by: Christian Decker <[email protected]>

wallet: Add outpointfilter to wallet so we can pass it all outputs

741a749

Will be used later to filter out outputs we are interested in, and trigger db updates with them. Signed-off-by: Christian Decker <[email protected]>

wallet: Update existing owned outputs with their confirmation height

2cf401d

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

wallet: Move txfilter into wallet

ae1ce73

Transaction filters are strongly related to the wallet, this move just makes it a bit more explicit. Signed-off-by: Christian Decker <[email protected]>

chaintopo: Record outpoint spends for owned outputs

6d3961b

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

db: Add utxoset table

b4fd077

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

wallet: Add outpointfilter for the utxoset

96bd84c

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

wallet: Add primitive to register new utxoset outpoint to the wallet

a06f552

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

topology: Add new P2WSH outpoints to the wallet utxoset

84ac913

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

db: Add index on utxoset.spendheight to speed up blockchain reorgs

52a447a

Otherwise we would be doing a table scan per block being reorged/rescanned. Signed-off-by: Christian Decker <[email protected]>

wallet: Mark utxo outpoints as spent

a3c89cf

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

wallet: Clean up spent UTXOs once they are buried deeply enough

a49618f

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

cdecker force-pushed the trackblocks branch from 7a360b8 to a49618f Compare March 6, 2018 15:09

cdecker merged commit aba3d5f into ElementsProject:master Mar 6, 2018

This was referenced Mar 6, 2018

Reduce rescanning on startup #1179

Closed

Track owned outputs when they are being spent on the blockchain #451

Closed

Track output confirmations, spends and maintain an internal UTXO set #1117

Track output confirmations, spends and maintain an internal UTXO set #1117

Uh oh!

Conversation

cdecker commented Feb 27, 2018

Uh oh!

ZmnSCPxj commented Feb 27, 2018

Uh oh!

cdecker commented Feb 27, 2018

Uh oh!

cdecker commented Feb 27, 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!

ZmnSCPxj commented Feb 27, 2018

Uh oh!

cdecker commented Feb 27, 2018

Uh oh!

cdecker commented Mar 4, 2018

Uh oh!

ZmnSCPxj 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!

Choose a reason for hiding this comment

Uh oh!

cdecker commented Mar 4, 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!

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!