Skip to content

Implement BlockCache trait for FsBlockDb#1535

Open
Oscar-Pepper wants to merge 9 commits into
zcash:mainfrom
Oscar-Pepper:impl_block_cache_for_fsblockdb
Open

Implement BlockCache trait for FsBlockDb#1535
Oscar-Pepper wants to merge 9 commits into
zcash:mainfrom
Oscar-Pepper:impl_block_cache_for_fsblockdb

Conversation

@Oscar-Pepper
Copy link
Copy Markdown
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 force-pushed the impl_block_cache_for_fsblockdb branch from e0d5339 to 4995d68 Compare September 12, 2024 09:05
@Oscar-Pepper Oscar-Pepper marked this pull request as ready for review September 12, 2024 09:06
ec2
ec2 reviewed Sep 20, 2024
View reviewed changes
Comment thread zcash_client_backend/src/data_api/chain.rs Outdated
@str4d str4d force-pushed the impl_block_cache_for_fsblockdb branch from 4995d68 to c91dc38 Compare October 25, 2024 06:07
@str4d
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <jack@electriccoin.co>
@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
Copy Markdown
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

@daira daira daira left review comments

@str4d str4d str4d left review comments

+1 more reviewer

@ec2 ec2 ec2 left review comments

Reviewers whose approvals may not affect merge requirements

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.

5 participants

@Oscar-Pepper @str4d @daira @ec2 @pacu