Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement BlockCache trait for FsBlockDb #1535

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Implement BlockCache trait for FsBlockDb #1535

wants to merge 9 commits into from

Conversation

Oscar-Pepper
Copy link
Contributor

@Oscar-Pepper Oscar-Pepper commented Sep 11, 2024 •
edited
Loading

  • Implements Sync trait for BlockDb and FsBlockDb (trait bound for BlockCache trait)
  • Implements BlockCache for FsBlockDb
  • Scan_cached_blocks takes a BlockCache instead of BlockSource to solve issues with thread safety with callback and also update the sync engine to use only BlockCache trait methods
  • Changes fns and tests to async as a result of implementing Sync trait for BlockDb and FsBlockDb
  • implements BlockCache::read for BlockDb, neccessary for calling scan_cached_blocks

@Oscar-Pepper Oscar-Pepper mentioned this pull request Sep 11, 2024
Closed
@Oscar-Pepper Oscar-Pepper marked this pull request as ready for review September 11, 2024 15:57
@Oscar-Pepper Oscar-Pepper marked this pull request as draft September 11, 2024 15:58
@Oscar-Pepper Oscar-Pepper marked this pull request as ready for review September 12, 2024 09:06
@str4d str4d force-pushed the impl_block_cache_for_fsblockdb branch from 4995d68 to c91dc38 Compare October 25, 2024 06:07
@str4d
Copy link
Contributor

str4d commented Oct 25, 2024

Rebased on main to fix major merge conflicts after the test framework was moved out of zcash_client_sqlite.

I'm going to perform more rebases on this to clean up the commit history and break apart the larger changes as best I can, to make the PR easier to review and potentially break out smaller pieces of it into separate PRs.

@str4d str4d force-pushed the impl_block_cache_for_fsblockdb branch from c91dc38 to 1675bf3 Compare October 26, 2024 02:16
@str4d
Copy link
Contributor

str4d commented Oct 26, 2024

Rebased on main to fix more merge conflicts.

@str4d str4d force-pushed the impl_block_cache_for_fsblockdb branch from 1675bf3 to 2c32c73 Compare October 26, 2024 06:06
@str4d
Copy link
Contributor

str4d commented Oct 26, 2024

Force-pushed to refactor the commits without any changes. In particular, I've pulled most of the async test changes out into a final commit, because I do not want the test framework to be async (at least at this stage), and we can instead use futures::executor::block_on wherever the test framework needs to use async functions (I haven't implemented this change yet but will do so).

I also plan to rebase this PR again once #1595 merges, to move away from the async-trait crate which I think will significantly simplify this PR.

@daira daira mentioned this pull request Oct 26, 2024
Merged
@str4d
zcash_client_backend: Switch from async-trait to RPIT for BlockCache
6f9390b 
The `BlockCache::truncate` default method is removed because the
`trait-variant` crate does not yet support default methods.
@str4d str4d force-pushed the impl_block_cache_for_fsblockdb branch from 2c32c73 to 890fd86 Compare October 28, 2024 22:35
@str4d
Copy link
Contributor

str4d commented Oct 28, 2024

Rebased on main to move away from the async-trait crate. I also did some more reworking of the commits to split them into logical changes. I almost have the test framework async-ness confined to the last commit; there is just one test in zcash_client_sqlite of FsBlockDb that needs adjusting.

The main next thing I want to figure out is whether the current state of the first commit can be paired down even further. BlockSource is made async in this PR, and I want to figure out whether that is necessary or not for the stated PR goal of implementing BlockCache for FsBlockDb.

str4d and others added 8 commits October 29, 2024 02:28
@str4d
zcash_client_backend: Generalise BlockSource::with_blocks error
36144f9 
Instead of requiring `zcash_client_backend::data_api::chain::Error`, it
now works with any error type that `BlockSource::Error` can be converted
into. We require an explicit conversion function instead of using `From`
because in the specific case of `data_api::chain::Error` it would result
in conflicting `From` impls.
@Oscar-Pepper @str4d
implement Sync for FsBlockDb and BlockDb so they can implement BlockC…
95649dc 
…ache trait

Co-authored-by: Jack Grigg <[email protected]>
@Oscar-Pepper @str4d
replace block source with block cache in scan cached blocks
d3bf952
@Oscar-Pepper @str4d
implement BlockCache for FsBlockDb
69cf2d9
@Oscar-Pepper @str4d
updated change logs
22b7714
@Oscar-Pepper @str4d
implement BlockCache trait for MockBlockSource
163dbc7
@Oscar-Pepper @str4d
implement 'read' for BlockDb so it can call 'scan_cached_blocks'
3d1c710
@Oscar-Pepper @str4d
zcash_client_backend: Migrate test framework to be async
020438c
@str4d str4d force-pushed the impl_block_cache_for_fsblockdb branch from 890fd86 to 020438c Compare October 29, 2024 02:50
@str4d
Copy link
Contributor

str4d commented Oct 29, 2024

Force-pushed to remove the dependency of BlockSource on data_api::chain::Error, and thus remove the similar complexity that this PR added to BlockCache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ec2 ec2 ec2 left review comments

@str4d str4d str4d left review comments

@daira daira daira left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Oscar-Pepper @str4d @daira @ec2