1169 sync engine with block cache trait #1217
Conversation
…` with methods for managing a block cache.
… tokio deps and moved tokio rt-multi-thread feature to dev-dependencies
- Added read error documentation - Changed name of `cache_tip` method to `get_tip_height` - Improved doc comments
This implements the necessary state machine for taking a wallet in some arbitrary synchronization status, and fully scanning (the remainder of) the chain. Closes zcash#1169.
…d implement for `MockBlockSource` zcash_client_sqlite: Implement block cache for `FsBlockDb` and `TestBlockCache`
Oscar-Pepper
force-pushed
the
1169-sync-engine_block-cache
branch
from
e9d7194 to
77d27a0
Compare
|
This needs to be rebased now that #1184 has been merged. |
Ok thanks. I think its best I separate this PR into more managable chunks. I will try to fit this in soon so it doesnt get too far behind |
|
This PR has now been blocking parts of |
| #[tracing::instrument(skip(params, block_source, data_db))] | ||
| #[tracing::instrument(skip(params, block_cache, wallet_data))] | ||
| #[allow(clippy::type_complexity)] | ||
| pub fn scan_cached_blocks<ParamsT, DbT, BlockSourceT>( | ||
| pub fn scan_cached_blocks<ParamsT, DbT, BcT>( | ||
| params: &ParamsT, | ||
| block_source: &BlockSourceT, | ||
| data_db: &mut DbT, | ||
| from_height: BlockHeight, | ||
| limit: usize, | ||
| ) -> Result<ScanSummary, Error<DbT::Error, BlockSourceT::Error>> | ||
| block_cache: &BcT, | ||
| wallet_data: &mut DbT, | ||
| scan_range: &ScanRange, | ||
| ) -> Result<ScanSummary, Error<DbT::Error, BcT::Error>> |
There was a problem hiding this comment.
As a suggestion for future PRs: part of the reason why this PR is harder to rebase is because there are commits like this one that rename all of the arguments as well as changing the parameter name, which were not necessary to do. In particular, renaming data_db to wallet_data in the same commit as switching from BlockSource to BlockCache.
| @@ -138,7 +123,7 @@ where | |||
|
|
|||
| // 7) Loop over the remaining suggested scan ranges, retrieving the requested data | |||
| // and calling `scan_cached_blocks` on each range. | |||
| let scan_ranges = db_data.suggest_scan_ranges()?; | |||
| let scan_ranges = wallet_data.suggest_scan_ranges()?; | |||
There was a problem hiding this comment.
Same thing here. The unnecessary rename from db_data to wallet_data has caused this entire file to be conflicted, but because it was done in the same commit as the BlocksCache change, I can't just blow the commit away and recreate the rename.
| pub(crate) struct BlockCache { | ||
| pub(crate) struct TestBlockCache { |
There was a problem hiding this comment.
Another unnecessary rename. It's entirely expected to encounter structs with the same name as traits. Rather than renaming this struct in the same commit, this should instead have used a combination of impl crate::data_api::chain::BlockCache for BlockCache and use crate::data_api::chain::BlockCache as _ to avoid the naming conflict and reduce the conflicting changes made in the commit.
Zingo Labs reconsidered its strategy and significantly reallocated resources with the April 30th release of Zashi. I have done my best to make my position clear and consistent. Prior to the entrance of Zashi into the competitive wallet market it was in our interest to allocate our limited resources towards librustzcash functionality that's useful across the ecosystem. We bore the significant personal cost when this bet went poorly for us with the introduction of the new Binance requirements. In spite of the fact that our primary funding stream was (and is) payment on delivery for specified results, we allocated precious developer time to unpaid contributions to community-critical resource like librustzcash, and zebrad. In the context of Zashi's release it's essential that we differentiate ourselves to survive. Pro-bono work on the common sync core is not just less efficient than more targeted efforts on our own sync engine, it is actively counter to this necessary differentiation. This inefficient allocation of librustzcash development effort is certainly not the alignment I would have preferred. Any implication that our efforts are not efficient, and consistent, should be considered both in light of the Zashi shift in our operating environment, and in light of our substantial track record of FOREGOING UNEARNED PAYMENT until such time as we complete our contracts. Thanks for your thoughtful attention, |
Had you mentioned this here or elsewhere, I would have taken over the PR much earlier. As it was, I was trying to ensure you were included in the development process, something you had at the time expressed significant interest in, and was taking pains to ensure I was not doing work in parallel that would obsolete work you were presumably (given no updates here to the contrary) working on at your own pace. I will not make this mistake in future. |
|
I can check with |
|
As I said above, I've already picked this up myself, due to its necessity for zcashd deprecation. I'm currently on PTO though and not working on it until at least end of next week, so if anyone else is likely to make progress in the next week they are more than welcome to do so. |
Hi str4d, I apologise for my part in the way this has turned out, it was not my intention to cause blockers on zcashd deprecation or cause you frustration. I was not aware of this until now as I missed your comment from three weeks ago. I think zancas has already mentioned we are limited on resources and funds. The zip317 release turned out to be a lot more work than expected as we replaced a large % of our legacy code along with a massive increase in test coverage. I had to pull off sync to prioritise this work. I agree this should have been communicated better. This PR was initally developped alongside #1192 . As i mentioned in #1192 and also on signal and discord, this PR was intended to replace #1184 so you wouldnt have to repeat the work i had already done to get #1184 building and ready for merge. At the time, this PR was complete and only draft because #1192 was still under review and had multiple change requests which lead to me rebasing and re-working this PR multiple times. Simplifying this rebasing was actually the reason why i squashed my commits which was one of your conerns. I understand you were very busy with the zashi release but I feel the lack of communication at this time is also a factor here. In this context, I hope you see that I wasnt so much renaming 'db_data', I was intending these names to be the first to merge and to better reflect the fact that this sync engine was also going to be used by wallets like zingo where the wallet data is not purely a database persistance layer like sqlite. In heinsight I should have left it alone. I am away for the next week but I will prioritise this PR when i return if you have not got onto it before hand. |
|
superseded by #1535 |
FsBlockDbwithBlockCachefor generic implementation of block caches, solving cyclic dependencies.BlockCacheforFsBlockDbandTestBlockCache(formerlyBlockCache)