UTXO high watermark in db - #1005
Conversation
| const char *cli; | ||
| const char *cli_args; | ||
| const u64 dust_limit; | ||
| const u32 when_lightning_became_cool; |
| .cli_args = NULL, | ||
| .dust_limit = 546, | ||
| /* "Lightning Charge Powers Developers & Blockstream Store" */ | ||
| .when_lightning_became_cool = 504500, |
| "CREATE TABLE htlc_sigs (channelid INTEGER REFERENCES channels(id) ON DELETE CASCADE, signature BLOB);", | ||
| "CREATE INDEX channel_idx ON htlc_sigs (channelid)", | ||
| /* Need to track high-watermark for wallet utxos. */ | ||
| "ALTER TABLE vars ADD COLUMN last_processed_block INTEGER;", |
There was a problem hiding this comment.
This is wrong. See line 144 for correct way to add new var. with an initial value Note, if not added in migrations, db_get_intvar will return 0.
Each row in TABLE vars is a variable, so we should add a row, not a column. db_set_intvar will automatically add a row, if not present.
There was a problem hiding this comment.
Oops, will fix, thanks!
There was a problem hiding this comment.
Hmm, I don't actually need this at all, thanks! I think I prefer all 'vars' defined in the dbmigrations array, rather than allowing them to created on the fly, but that can be done later.
| if (db_get_intvar(w->db, "bip32_max_index", 0) != 0) | ||
| return true; | ||
|
|
||
| /* Or we could have had a channel terminate unilaterally, |
There was a problem hiding this comment.
How about, if we are a fresh, satoshiless node, somebody opens a channel to us, gives us money, then closes the channel bilaterally? Also, which unilateral is this?
There was a problem hiding this comment.
This would be the their_unilateral/to_us output, since it's the only case in which we don't have the secret key integrated into the HD key structure, instead it's a collaboratively generated key. In all other cases onchaind will take care to respend to a key in the hierarchy.
|
Concept ACK, but please see my review. |
|
I think this might break |
We already go back 100 from this in case of reorgs, so the block number itself is sufficient. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
With fallback depending on chainparams: this means the first upgrade will be slow, but after that it'll be fast. Fixes: ElementsProject#990 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As described by @lvaccaro in ElementsProject#990. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
|
I think we still get this right? We actually take the earlier of two measurements:
We then subtract another 100 blocks for some extra fork protection (which I wish we hadn't initially, since it papered over a number of these bugs!). I'll add clarifying comments. |
We only need to do that if it's possible there's something to find: either we have an unspent output from a unilateral close, or we've ever handed out an address. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
ZmnSCPxj queried the unilateral close case, so make that clearer. Christian raise concerns about existing channels, so make it clear what we're doing there too. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
We do a complicated dance because we don't know the current block height before setting up the topology. If we're starting at a particular block, we want to go back 100 blocks before that to cover any reorgs. If we're not (fresh startup), we still want to go back 100 blocks because we don't bother handling a reorg which removes all the blocks we know. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
6f18a0e to
2ac26dc
Compare
|
ACK 2ac26dc |
|
Ok, didn't see the ACK 2ac26dc |
3 participants
This, I believe, is the proper solution to the problem in #990