Save transactions in database

[jp1ac4]

This is the first step of #56 (comment).

The poller will now save transactions in our own database. These transactions are selected based on the deposit and spend transactions of coins. Only the txid and transaction itself are saved, with the corresponding block height and time taken from the coins table.

In a couple of follow-up commits, I've replaced some RPC calls to bitcoind with DB queries.

[edouardparis]

I think list_missing_txids may fetch a too big batch of txids. I was thinking maybe it is better to loop over list_txids: https://github.com/wizardsardine/liana/blob/master/src/database/mod.rs#L148 from current time to genesis block timestamp, and for unconfirmed transactions use the coins(&[Status::Unconfirmed], ...)

[jp1ac4]

Perhaps yes, although in that case, we should probably also do the same for the coins() method, as the max number of missing txids should be of a similar order to the total number of coins in the DB.

[jp1ac4]

I'll change list_missing_txids() to use a loop.

[jp1ac4]

I used a loop to get the missing txids using ROW_NUMBER() as it seemed the simplest way. Most of the time, we don't expect there to be many missing values, only when the transactions table is initially populated for an existing wallet or perhaps if the poller has not been run for a long time.

[edouardparis]

Sorry If I was unclear, my point was to fetch missing txids and store transactions by batch in order to not keep a whole set in memory.

In fact, i am thinking that we do not need to paginate txids to do that, just fetch 20 txids from the coins table where the txid is not in the txid field of the table transactions, fetch their raw tx with bitcoind then store them in transactions table and repeat until no txids are fund.

[jp1ac4]

Ah I see, yep that makes sense, will change it.

[jp1ac4]

In fact, i am thinking that we do not need to paginate txids to do that, just fetch 20 txids from the coins table where the txid is not in the txid field of the table transactions, fetch their raw tx with bitcoind then store them in transactions table and repeat until no txids are fund.

I couldn't quite do it like this in the end as there's a chance we won't be able to retrieve the transaction from bitcoind, and so the loop might have continued forever. Instead, I get all missing txids in one query still, but then retrieve chunks of transactions at a time so that we don't store them all in memory at once. As the transactions are larger than txids, and we already store the same order of coins in memory as there are txids, I felt it was fine to do it this way.

[darosior]

I think we should do something closer to what we do for the backend and its poller:

• Have the coins table's txid row reference a transaction in the transactions table (thereby introducing a constraint that a transaction must exist in database for any coin);
• When inserting new coins to the database, first insert the corresponding transaction.

Of course this means you need access to transactions at the time of the migration. I'm still thinking about ways to reduce ugliness of doing that.

[jp1ac4]

Of course this means you need access to transactions at the time of the migration. I'm still thinking about ways to reduce ugliness of doing that.

Perhaps we could add a migration step in Poller::new() around the same place we call looper::maybe_initialize_tip(&bit, &db).

[darosior]

Alright, so here is how i think we should proceed:

1. Separate migration from the SQLite database constructor
2. When setting up SQLite at init, first poll the database version before proceeding with any migration
3. If the database version is one for which Bitcoin transactions weren't stored in DB, query them from bitcoind
4. Only then proceed with the SQLite migration and pass the Bitcoin transactions which need to be stored

At the expense of making the code less encapsulated and a bit harder to follow, this gives us two nice robustness properties:

• If any Bitcoin transaction is missing (which should never happen ™️) the migration will fail.
• For any utxo in DB its transaction is always in DB too.

I've implemented this on this branch: https://github.com/darosior/liana/commits/pr_1180. Feel free to take anything from there, of course.

[jp1ac4]

Thanks for the updated approach and code.

I wonder whether for such migrations that need to query the DB (to get the txids in the present case), we should first migrate the DB to the most recent version, then query the DB, and then apply the final migration. Although I don't think it would cause any problems in the present case, the DB query used in the current code base may not always be compatible with the user's DB version.

Perhaps this could be done by passing the bitcoind connection to maybe_apply_migrations instead of the transactions themselves?

[darosior]

I think we should strive to make migrations atomics: your database is either fully compatible with the previous version or the next version. For instance with what you suggest a user could use Liana version N+1 with a database created with Liana version N, apply the first migration, for whatever reason encounter an error a crash or just stop the application before applying the second migration, and be stuck on a database which can neither be used by Liana version N nor by Liana version N+1.

If the existing query methods on the SQLite connection struct were not appropriate to an old database we could just write a custom one for this purpose.

Perhaps this could be done by passing the bitcoind connection to maybe_apply_migrations instead of the transactions themselves?

I precisely tried to avoid entangling the Bitcoin and Database modules.

[jp1ac4]

Sorry, I wasn't clear in my description. I didn't mean that the migration from DB versions 4 to 5 would be split in two separate steps. Each migration from one DB version to the following would still be atomic.

So the function migrate_v4_to_v5 would still perform all changes on the DB. But instead of receiving the txs, it would instead get these from bitcoind as an initial step within the function.

But I agree that keeping in mind the potential need to modify DB queries for previous DB versions should be sufficient. Indeed, that could still be required using my approach if version N - 1 is different.

I'll proceed using your code and start adding it into this PR.

[jp1ac4]

Does bitcoind's watchonly wallet keep transactions that are no longer in the best chain or were never confirmed and are no longer in the mempool?

[darosior]

Yes.

[jp1ac4]

I've now updated it to use foreign key constraints from the coins to transactions tables and to retrieve transactions as part of the migration.

[darosior]

Just a few comments as i read through the diff, which looks good.

[darosior]

I've written a unit to specifically exercise the migration from v4 to v5, including the failure case: darosior@c9c82f2.

It only trips on the last check which compares the transactions from DB with the transactions created in the test because i think your list_wallet_transactions returns duplicates. See #1180 (comment).

[jp1ac4]

The failing test may be due to the spend_txid having multiple different block infos, whereas list_wallet_transactions assumes that a given txid can't have inconsistent block info in the coins table.

[jp1ac4]

I could add a corresponding assertion to list_wallet_transactions to catch duplicates, as that would indicate corrupt DB data.

[darosior]

The failing test may be due to the spend_txid having multiple different block infos, whereas list_wallet_transactions assumes that a given txid can't have inconsistent block info in the coins table.

Ah, good catch. Fixing it now.

[jp1ac4]

I think I've now made all the requested changes.

[darosior]

Tested migrating from v4.0, v5.0 and v6.0 using the migration functional test.

[darosior]

Tested by upgrading a mainnet wallet. No issue.

[darosior]

ACK 3953a0b

[darosior]

re-ACK bf1e90e

[jp1ac4]

Thanks for the review and additional commits 😃

[darosior]

Thanks for bearing through the many iterations!

…

-------- Original Message --------

On 8/2/24 12:25 PM, Michael Mallan wrote: Thanks for the review and additional commits 😃 — Reply to this email directly, [view it on GitHub](#1180 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F5P2U6R7WUMV4XJMJTZPNNA5AVCNFSM6AAAAABKSKEGOOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRVGA3DGMZYHE). You are receiving this because you modified the open/close state.Message ID: ***@***.***>