Skip to content

Wallet db to use sqlite (instead of litedbv4)#272

Merged
sondreb merged 12 commits intomasterfrom
wallet-sqlite
Feb 19, 2021
Merged

Wallet db to use sqlite (instead of litedbv4)#272
sondreb merged 12 commits intomasterfrom
wallet-sqlite

Conversation

@dangershony
Copy link
Copy Markdown
Member

The main db tests are now passing.
I still need to run all the solution tests and then also compare to other wallets on current networks

Copy link
Copy Markdown
Member

@sondreb sondreb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is persistence done? Appears to store a lot of IO to disk, perhaps there is an interval settings key that can be set a bit higher? Maybe increase it during IBD?

@dangershony
Copy link
Copy Markdown
Member Author

It will push on every trx that we find for the wallet, I don't think sqlite has a cache mechanism I didn't check.

Copy link
Copy Markdown
Member

@sondreb sondreb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have verified the performance, works very well. Have not done properly code review yet, will do later if not merged yet, but approval so far.

@MithrilMan
Copy link
Copy Markdown
Contributor

Any reason why not implementing this like other "persistence-aware" components and be able to switch db implementation at will?

@dangershony
Copy link
Copy Markdown
Member Author

Any reason why not implementing this like other "persistence-aware" components and be able to switch db implementation at will?

Well perhaps it can be done but I want to first get something working.
(perhaps we can keep the litedbv5 impl in a none used persistence feature so if in the future it becomes stable we can easy swap)

@MudDev
Copy link
Copy Markdown
Member

MudDev commented Feb 8, 2021

I did a review and this is brilliant. I don't think we had index's before with LiteDB, now we do with SQLite, great work @dangershony

Can't wait to give this a try!

@dangershony
Copy link
Copy Markdown
Member Author

All tests now passed locally and I see mac tests passed too.
So I think this is ready for serious review

Comment thread src/Features/Blockcore.Features.Wallet/Database/WalletStore.cs Outdated
Comment thread src/Features/Blockcore.Features.Wallet/Database/WalletStore.cs
Copy link
Copy Markdown
Contributor

@MithrilMan MithrilMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, commented a bit :)

Comment thread src/Features/Blockcore.Features.Wallet/Database/WalletStore.cs
Comment thread src/Features/Blockcore.Features.Wallet/Database/WalletStore.cs
Comment thread src/Features/Blockcore.Features.Wallet/Database/WalletStore.cs
Comment thread src/Features/Blockcore.Features.Wallet/Database/WalletStore.cs
Comment thread src/Features/Blockcore.Features.Wallet/Database/WalletStore.cs Outdated
Comment thread src/Features/Blockcore.Features.Wallet/Database/WalletStore.cs Outdated
Comment thread src/Features/Blockcore.Features.Wallet/Database/WalletStore.cs Outdated
@sondreb sondreb merged commit ad2fca4 into master Feb 19, 2021
@dangershony dangershony deleted the wallet-sqlite branch February 19, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants