Store the last sync time and block height by afilini · Pull Request #459 · bitcoindevkit/bdk

afilini · 2021-10-26T12:50:07Z

Description

Update the Database trait to allow storing the block height and timestamp of the last sync of a Wallet. This will allow having a rough estimation of the "current" height, even in parts of the code that don't have network access (tx creation, computing the balance, etc), at least in non-airgapped wallets that are periodically synced.

After this is ready I'm planning to work on a PR to close #413, essentially making BDK aware of what can be spent and what is still not "mature" (initially only for coinbase UTXOs which are trivial, later on ideally also for any UTXO that has a timelock, which is a lot trickier).

Closes #455.

Notes to the reviewers

Opening this in draft mode, since it includes some commits from #454.

Random open points:

I was thinking about renaming ConfirmationTime because that name doesn't really make sense in this context, but I couldn't come up with a decent alternative. I'm open to suggestions if you have any.
I'm not sure what I've done with the sqlite database is ideal, let me know if you think I could use a better structure. Initially I used to add an entry in the table for every sync, and then I would only return the latest one. That seemed pretty inefficient, considering that I was storing stuff that were by definition never read, so I changed it to the current structure.. but this feels more NoSQL-like so I still don't know which one I like best.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

Bugfixes:

This pull request breaks the existing API
I'm linking the issue being fixed by this PR

notmandatory · 2021-10-27T03:17:27Z

My rename suggestion for ConfirmationTime is simply Confirmationand then the field would be TransactionDetails.confirmation.

notmandatory · 2021-10-27T15:55:58Z

It looks like this PR has come commits that are already in master, can it be rebased to simplify the history?
Don't need:
a348dbd
548e43d
548e43d
548e43d

LLFourn · 2021-10-28T05:23:14Z

Can we think about getting rid of the block height request in Wallet::new after this and just using the one from the database.

afilini · 2021-10-28T12:31:40Z

Can we think about getting rid of the block height request in Wallet::new after this and just using the one from the database.

Yup, that would be great. With that you would only need the network to sync, everything else would work offline as well, which is reasonable

LLFourn

LGTM -- one thing I'd like you to consider changing.

LLFourn · 2021-10-29T00:39:23Z

            Ok(())
        }

+        fn set_last_sync_time(&mut self, ct: ConfirmationTime) -> Result<(), Error> {


So I'm working towards adding different types of syncs. e.g. only sync addresses given out (no stop-gap). Maybe only sync addresses given out recently or only sync external keychain etc.

Could we leave the door open to that by making a SyncTime struct which has the blocktime (whose type is unfortunately called ConfirmationTime) as a field? It wouldn't hurt to add the local time to it. It's possible we'd want more metadata about a sync in the future and it'd make it easier from a DB updating point of view to serialize it in this way.

I tried implementing this but I got stuck because we don't have a way to get the tip hash with the current Blockchain trait.

Since I would have to make a breaking change to the trait I'm thinking I might as well split it up like you described during our last call.

I'm not sure if we should hold this PR until that's ready or just go ahead and merge it without block_hash and add that later on.

I think merge without that now and follow up with a PR that adds Option<BlockHash> to the struct. I still think that we don't want to add this to BlockTime but rather a struct like SyncTime that contains a BlockTime. Also SyncTime should probably contain the UTC time that the sync was finished (in this PR).

Yeah in my local branch I had a separate structure that contained the BlockTime and the block hash.

I will add just the UTC time for now, we'll add the block hash in a following PR

You might consider adding a field for it in the sqlite table otherwise we have to do a table update and I'm not sure we have a db upgrade plan yet :S?

artfuldev · 2021-11-02T20:51:15Z

Regarding ConfirmationTime, I think we can just use something like BlockInfo which has a few details from:

Currently, we use the height and timestamp as an indication of confirmation and last sync "time". Maybe we add hash next. And we can refer to it in a confirmed transaction as block: BlockInfo / while syncing as last_synced: BlockInfo.

notmandatory · 2021-11-09T22:25:31Z

Looks like it needs a rebase to fix the CHANGELOG then is ready to go.

This will be used to store the height and timestamp after every sync.

Closes bitcoindevkit#455

fields in the future

afilini · 2021-11-10T11:30:55Z

Done!

notmandatory

ACK 5830226

afilini changed the title ~~Feat/store sync time~~ Store the last sync time and block height Oct 26, 2021

afilini force-pushed the feat/store-sync-time branch from b47ef60 to ca41644 Compare October 28, 2021 12:31

LLFourn requested changes Oct 29, 2021

View reviewed changes

notmandatory added this to the Release 0.14.0 Feature Freeze milestone Nov 2, 2021

afilini force-pushed the feat/store-sync-time branch 3 times, most recently from 8486b60 to 8dfe72f Compare November 8, 2021 09:38

afilini added 6 commits November 10, 2021 12:29

[db] Add the last_sync_time database entry

2892edf

This will be used to store the height and timestamp after every sync.

[wallet] Expose an immutable reference to a wallet's database

12e51b3

[wallet] Store the block height and timestamp after syncing

7c06f52

Closes bitcoindevkit#455

Update CHANGELOG.md

3e5bb07

Rename ConfirmationTime to BlockTime

2c77329

[database] Wrap BlockTime in another struct to allow adding more

5830226

fields in the future

afilini force-pushed the feat/store-sync-time branch from 8dfe72f to 5830226 Compare November 10, 2021 11:30

notmandatory approved these changes Nov 10, 2021

View reviewed changes

notmandatory merged commit 5830226 into bitcoindevkit:master Nov 10, 2021

afilini mentioned this pull request Nov 11, 2021

BDK will select coinbase inputs that are not matured yet #413

Closed

Conversation

afilini commented Oct 26, 2021

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

notmandatory commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory commented Oct 27, 2021

Uh oh!

LLFourn commented Oct 28, 2021

Uh oh!

afilini commented Oct 28, 2021

Uh oh!

LLFourn left a comment

Choose a reason for hiding this comment

Uh oh!

LLFourn Oct 29, 2021

Choose a reason for hiding this comment

Uh oh!

afilini Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

LLFourn Nov 4, 2021 • 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!

afilini Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

LLFourn Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

artfuldev commented Nov 2, 2021

Uh oh!

notmandatory commented Nov 9, 2021

Uh oh!

afilini commented Nov 10, 2021

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

notmandatory commented Oct 27, 2021 •

edited

Loading

LLFourn Nov 4, 2021 •

edited

Loading