Fix the tracking of immature coinbase deposits #578
Conversation
We need to keep track of such coins to: - Track their maturation - Avoid using them (for instance in coin selection) Reorg handling for coinbase deposits that become immature is not implemented (yet). That's reasonable because: 1. It would be very unlikely that we'd move back, so it's most likely gonna be mature again immediately. 2. If there's a reorg of more than 100 blocks we've got bigger problems.
We detect and store immature deposits (cause otherwise we won't go through them again with listsinceblock), but mark them as such and unconfirmed. We only mark them as confirmed once they've matured. It's a bit clumsy but it's not as if most of our users had coinbase deposits.
|
I'd like to this in master rather earlier than later. I think i'll have another pass at reviewing tomorrow and merge it if i don't find anything. @edouardparis please do still review it, it's pretty critical that we get this right. |
|
self-ACK 097d5e7. Didn't have the chance to re-review it but getting it in is best at this time. Edouard will still have a look post-merge. |
| tx.execute( | ||
| "UPDATE coins SET is_immature = 0 WHERE is_immature IS NULL", | ||
| rusqlite::params![], | ||
| )?; |
There was a problem hiding this comment.
After the update, I would alter the column to make the is_immature not null
to match the schema
There was a problem hiding this comment.
Wouldn't that mean we'd have to ALTER it also when creating it for the first time? The schema currently doesn't have NOT NULL.
There was a problem hiding this comment.
https://github.com/wizardsardine/liana/pull/578/files#diff-9afc8c965492b82a57677899c77edda2d3c6ec9da5a31863b3550ba3d072da88R60
I may misunderstand this line then
There was a problem hiding this comment.
oh, no, you're right. It doesn't match what i did for the first migration..
There was a problem hiding this comment.
I guess i could even go with the one liner
ALTER TABLE coins ADD COLUMN is_immature NOT NULL DEFAULT 0Or maybe even with the CHECK (is_immature IN (0,1)).
There was a problem hiding this comment.
So i decided to not go with my approach to have exactly the same schema as if the database was freshly created (without any DEFAULT). But it's not possible to use your approach in SQLite: https://www3.sqlite.org/matrix/lang_altertable.html.
I'll just go with my one liner then.
| @@ -42,6 +44,7 @@ fn update_coins( | |||
| outpoint, | |||
| amount, | |||
| address, | |||
| is_immature, | |||
There was a problem hiding this comment.
This may trigger the assertion and the SQL constraint. My bad. See #1001.
#567 uncovered that we were actually not tracking coinbase deposits correctly. In fact we would most likely miss them all in any real situation. This is because we would filter out immature coinbase deposits from the result of
listsinceblockand not consider them newly received coins. But they would be confirmed, and as the chain moves forward we'd not scan this range anymore even once they've become mature.This PR fixes it by the simplest possible manner: record immature coinbase deposits as unconfirmed and only mark them as confirmed once they've become mature. This is a bit clumsy, but should be fine for the number of users that would receive payouts from coinbase transactions (ie most likely 0). Also, we don't accurately update coinbase coins on reorg. This is unnecessary as it'd be very unlikely that a mature coinbase would become immature and if there is a 100 blocks reorg that would invalidate it altogether we'd have bigger problems.
Fixes #567.
Still as draft as i want to go over this one more time before asking for review, as this if pretty intricate and touches core parts of our codebase.